SPEC-173: Document tests conditional on server version #126
Conversation
There was a problem hiding this comment.
Per the comment, the sort option had no purpose here other than creating an inconsistent result value for pre-3.0 servers. It seemed prudent to simply remove it and make the test more compatible, rather than create test variants.
There was a problem hiding this comment.
The two tests "without id specified" above previously existed in findOneAndReplace.yml but these "with id specified" variants are new. This makes the test cases consistent with what we already had in replaceOne.yml for the update command.
There was a problem hiding this comment.
Using "X.Y" for minServerVersion isn't really an issue, as we can assume the minor version is zero. That doesn't work as nicely for maxServerVersion, so we can either use the next minor version or a sufficiently high patch version in the same "X.Y" series. The CRUD test README does not specify whether minServerVersion and maxServerVersion are inclusive or exclusive, so I opted to use a high patch version instead of "2.5" in this case.
On the inclusive/exclusive front, I suppose that's only an edge case when comparing a test's minServerVersion against a server with a "0" patch version (e.g. "2.6.0"). To that end, we should probably document that these ranges are inclusive. I opened SPEC-834 to track this.
There was a problem hiding this comment.
These two tests were moved to findOneAndReplace-upsert.yml.
There was a problem hiding this comment.
Previously, no tests were asserting upsertedCount. It seems like a freebie, since the field is available for all server versions.
There was a problem hiding this comment.
I'll note that without the addition of upsertedCount here, this test wouldn't really be asserting anything meaningful due to SERVER-5289.
There was a problem hiding this comment.
Splitting off replaceOne-pre_2.6.yml allowed us to relocate the upsert tests to the base replaceOne.yml file and remove this entirely.
| I only have time this week for a quick skim, but what I saw seemed sensible. |
| Looks great Jeremy. lgtm. |
Update results do not include modifiedCount for pre-2.6 servers. Additionally, SERVER-5289 prevents us from verifying the ID for a document upserted by a replaceOne operation if the ID was only specified in the filter document.
This splits the findOneAndReplace upsert tests into their own file so that we can work around SERVER-5289 for pre-2.6 servers. Additionally, we remove the sort option from all findAndModify upsert tests where no document would match to work around SERVER-17650 for pre-3.0 servers.
https://jira.mongodb.org/browse/SPEC-173
Note: I'll commit the generated JSON files once the YAML tests are reviewed. I ran these tests against the PHP library (without my PHPLIB-95 work-around in place) on server versions 2.4, 2.6, and 3.4 without issue.