[DRIVERS-348] Resync CRUD spec tests organized by minServerVersion and maxServerVersion Created: 10/Jan/17  Updated: 15/Apr/19  Resolved: 17/Jan/18

Status: Closed
Project: Drivers
Component/s: None
Fix Version/s: None

Type: Task Priority: Major - P3
Reporter: Jeremy Mikola Assignee: Unassigned
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on CSHARP-1897 Resync CRUD spec tests organized by m... Closed
depends on PHPLIB-231 Implement test runner for CRUD spec t... Closed
depends on PYTHON-1218 Resync CRUD spec tests organized by m... Closed
depends on RUBY-1196 Resync CRUD spec tests organized by m... Closed
depends on NODE-912 Resync CRUD spec tests organized by m... Closed
depends on JAVA-2428 Resync CRUD spec tests organized by m... Closed
Related
related to DRIVERS-347 Support deprecated "modifiers" FindOp... Closed
is related to DRIVERS-332 Resync CRUD spec tests Closed
Driver Compliance:
Key Status/Resolution FixVersion
PHPLIB-231 Done 1.1.1
CSHARP-1897 Done 2.4.3
SCALA-292 Done
PYTHON-1218 Done 3.5
JAVA-2428 Done 3.5.0
RUBY-1196 Done 2.4.2
PERL-719 Done 2.0.0
NODE-912 Done

 Description   

CRUD tests are now organized by minServerVersion and maxServerVersion.

Relevant tag: crud-tests-2017-01-10



 Comments   
Comment by Jeremy Mikola [ 13/Jan/17 ]

The lack of the "upsertedId" field in "result" would seem to imply that the driver should not return a value of "upsertedId" field to the user, but the relevant versions of the server do generate an _id for the inserted document for this operation, so the C++ driver does return a value of "upsertedId".

I imagine all drivers report the server-generated ObjectID in the upsertedId field. According to the CRUD spec, the type of this field is "any" and there is no mention of what to do in the absence of a value. I presume this means drivers should leave it as null. That said, it's possible the upsert a document and use BSON null as its ID, so upsertedCount is really the source of truth as to whether or not we should consider the contents of upsertedId.

Given this observation, I'm led to believe that (as of this resync) drivers should now be ignoring the presence or absence of fields in their user replies that aren't mentioned in the "result" document from the tests? That doesn't seem like a great direction to go in, since it weakens the strength of the assertions that the spec test can make. I wonder if, in the test files, we should be encoding "the upsertedId field should not be present in the user reply" differently from "any value of upsertedId in the user reply is acceptable"?

My understanding was that fields not present in the outcome.result field within the CRUD spec tests are simply not asserted. I'm happy to clarify that in the test's README.rst, as it currently only states that outcome.result is "The return value from the operation."

Consider that before 5242505 many tests were not asserting the value of upsertedCount at all. Also, we omit assertions for modifiedCount in pre-2.6 tests because drivers are free to omit the field, leave it as null, or throw exceptions when it is accessed (per the Bulk Update spec).

Comment by J Rassi [ 13/Jan/17 ]

When resyncing these tests to the C++ driver repository, we ran into an issue with the "ReplaceOne with upsert when no documents match without an id specified" test in replaceOne-pre_2.6.json. The lack of the "upsertedId" field in "result" would seem to imply that the driver should not return a value of "upsertedId" field to the user, but the relevant versions of the server do generate an _id for the inserted document for this operation, so the C++ driver does return a value of "upsertedId". This issue didn't affect any previous tests (this test was previously blacklisted on 2.4 and earlier), so I figured I'd bring it up here, in case other folks run into this issue.

Given this observation, I'm led to believe that (as of this resync) drivers should now be ignoring the presence or absence of fields in their user replies that aren't mentioned in the "result" document from the tests? That doesn't seem like a great direction to go in, since it weakens the strength of the assertions that the spec test can make. I wonder if, in the test files, we should be encoding "the upsertedId field should not be present in the user reply" differently from "any value of upsertedId in the user reply is acceptable"? As a short-term fix, we could also remove this test (the spec test repo doesn't have any other operations that return non-deterministic values as far as I can tell, like insertions of documents that have a generated _id, but we probably should add those eventually).

Comment by Jeremy Mikola [ 12/Jan/17 ]

Validating PHPLIB (per PHPLIB-231). Not relevant to HHVM and PHPC.

Comment by Jeremy Mikola [ 12/Jan/17 ]

Note: although maxServerVersion may be changed to be exclusive by SPEC-834 (see: pull request), this should not require any changes to the tests in the crud-tests-2017-01-10 tag.

That said, drivers will still want to use ">=" and "<" comparisons for minServerVersion and maxServerVersion, respectively.

Generated at Thu Feb 08 08:21:19 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.