[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: |
|
||||||||||||||||||||||||||||||||||||||||
| Driver Compliance: |
|
||||||||||||||||||||||||||||||||||||||||
| 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 ] |
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.
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 |
| 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. |