test(DRIVERS-3034): remove/update 4.0 related tests #1796
Conversation
|
Dead link errors on the lint task exist on previous commit. |
| @@ -15,7 +15,7 @@ phases: [ | |||
| setVersion: 1, | |||
| setName: "rs", | |||
| minWireVersion: 0, | |||
| maxWireVersion: 7 | |||
| maxWireVersion: 16 | |||
There was a problem hiding this comment.
Wire version 16 is server 5.3
There was a problem hiding this comment.
The Java and C drivers are able to pass these tests still, I think because it's only in server selection where the maxWireVersion is checked. Do you expect other drivers to fail these tests, and if so, what causes the failure?
There was a problem hiding this comment.
My expectation is that no drivers would fail these tests currently as they are for pre 6.0 and this sets the max wire version for 5.3. If the drivers remove support for 4.0, I expect these tests to fail without this change on drivers that do wire version checks outside of server selection as the maxWireVersion would then be lower than the minWireVersion. (For example, Node does do this check outside of server selection in https://github.com/mongodb/node-mongodb-native/blob/fcbc6edfc3d54f5dbd3b142c06ef205ecd2729b8/src/cmap/connect.ts#L60)
There was a problem hiding this comment.
OK, this seems like a harmless change to make, even for drivers using submodules. There is no loss of test coverage, and the tests should pass all drivers.
But why stop at 5.3? Any reason not to just bump it up the MongoDB 8.0 wire version?
There was a problem hiding this comment.
Oh, as you explained in Slack, this is for pre-6.0
durran
requested review from
W-A-James and
ShaneHarvey
and removed request for
a team
| @@ -19,7 +19,7 @@ phases: [ | |||
| # servers from a primary isWritablePrimary are added to the working server set | |||
| me: "a:27017", | |||
| minWireVersion: 0, | |||
| maxWireVersion: 7 | |||
| maxWireVersion: 25 | |||
There was a problem hiding this comment.
These I just updated to latest to not need to continually update them.
|
@durran, given that many drivers are using git submodules to pull in the specification tests, we actually have to wait until all drivers have implements DRIVERS-3034 before we actually delete these tests. Otherwise any drivers that have not yet implemented DRIVERS-3034 will be missing test coverage once they update their git submodules. So I don't think we should merge this PR as is. |
Fine with that. I just wanted exposure that I needed to do this to do the work for Node. Happy to close and create a DRIVERS ticket if you think that's the better approach. Also as a note, for drivers that do have this repo as a submodule, this change is not going to break anything. It's basically a setup for when they do remove 4.0 support. |
|
What to do with this PR depends in part on the answer to my question here. It's easy for drivers to start skipping a bunch of tests, but tests that need modification are a problem. |
|
Link check fixed in #1797 |
Removes SDAM tests that were testing pre-4.2 functionality due to drivers removing support for 4.0. Also updates other SDAM spec tests testing pre 6.0 server versions to the wire version for 5.3.
Driver test with these changes in Node: https://spruce.mongodb.com/version/6819065e84440c0007073813/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC
Related PR: mongodb/node-mongodb-native#4534
Please complete the following before merging:
clusters, and serverless).