SWIFT-1229, SWIFT-1216, SWIFT-1243 Vendor libmongoc patch and sync command monitoring tests #643
Conversation
| @@ -155,7 +169,7 @@ private struct CMTest: Decodable { | |||
|
|
|||
| case "insertMany": | |||
| let documents = (self.op.args["documents"]?.arrayValue?.compactMap { $0.documentValue })! | |||
| let options = InsertManyOptions(ordered: self.op.args["ordered"]?.boolValue) | |||
| let options = InsertManyOptions(ordered: self.op.args["options"]?.documentValue?["ordered"]?.boolValue) | |||
There was a problem hiding this comment.
the test file was updated at some point to nest this value
| @@ -247,20 +261,12 @@ private struct CommandStartedExpectation: ExpectationType, Decodable { | |||
| private func normalizeCommand(_ input: BSONDocument) -> BSONDocument { | |||
| var output = BSONDocument() | |||
| for (k, v) in input { | |||
| // temporary fix pending resolution of SPEC-1049. removes the field | |||
There was a problem hiding this comment.
SPEC-1049 was long ago resolved, but we never got a SWIFT ticket created for it so the changes are just now being synced
| @@ -0,0 +1,659 @@ | |||
| { | |||
| "description": "redacted-commands", | |||
| "schemaVersion": "1.0", | |||
There was a problem hiding this comment.
I manually edited this from 1.5 to 1.0. there are technically some things in this file that require test runner updates -- use of "auth": false and "observeSensitiveCommands": true -- but fortunately at least right now we don't actually need to do anything about them.
the reason those options are included is so that drivers who explicitly ignore sensitive commands in their runner implementation know to pay attention to them here, and so that the test isn't run with any extraneous auth-related events published (that would happen if the test is run against a deployment with auth).
however, somewhat conveniently libmongoc is currently non-spec-compliant and does not publish events for any auth commands it runs itself (though it will publish them for any run manually via runCommand, as this test does). this means that we can actually run the test with auth on after all since there won't be extra events.
as a result of libmongoc not publishing the auth events, I missed the unified test runner requirement to explicitly ignore sensitive commands since we never saw events for them to begin with, so we also already have the "observeSensitiveCommands" behavior required by this test file by default.
in my forward-port PR I will add logic for actually parsing and obeying these options correctly, but it doesn't seem worth the work to do that here too given that they would have no effect on Swift 1.1.x + libmongoc 1.17.x, and I don't think we'll be maintaining this branch much longer anyway.
There was a problem hiding this comment.
Is omitting such events definitely not spec compliant? It's not actually explicitly stated in the Command Monitoring spec (from my reading) that handshakes should be included in the events, though it also doesn't explicitly state that they shouldn't. IMO, given how easy it is to leak stuff and how noisy the handshake commands can be, I actually think it would be beneficial to mandate that drivers not emit events pre or during handshake. Rust is like libmongoc/Swift in this way btw.
There was a problem hiding this comment.
You're right, it's actually still ambiguous. I think I was thinking that it wasn't because of DRIVERS-1809, but we haven't actually done that ticket yet. FWIW that ticket originally proposed events should not be published, but after a conversation in leads triage - which I missed due to PTO so I don't have much context on - it was edited to propose the opposite.
| @@ -178,6 +178,7 @@ extension MongoCollection_IndexTests { | |||
| static var allTests = [ | |||
| ("testCreateIndexFromModel", testCreateIndexFromModel), | |||
| ("testIndexOptions", testIndexOptions), | |||
| ("testTextIndex", testTextIndex), | |||
There was a problem hiding this comment.
this and the changes for the index tests were just cherry-picked from a commit I made on main a while back to fix these tests against 5.0/latest: dfa2ca9
…fs going forward (#614) * ignore lines starting with a slash when checking diff * make exports and linuxmain
| @@ -231,9 +231,10 @@ functions: | |||
| SWIFT_VERSION=${SWIFT_VERSION} \ | |||
| bash ${PROJECT_DIRECTORY}/.evergreen/install-tools.sh sourcery | |||
| make linuxmain SOURCERY=${PROJECT_DIRECTORY}/opt/sourcery/bin/sourcery | |||
| git diff --exit-code Tests/LinuxMain.swift | |||
| # use regex to ignore lines starting with a / | |||
There was a problem hiding this comment.
this and related sourcery changes cherry-picked in from main: dbc65f3
Vendors in libmongoc 1.17.7 and syncs the recent changes to the command monitoring tests.
Evergreen patch (waiting on macOS, but all passed locally anyway): https://evergreen.mongodb.com/version/60e78b0157e85a0b2b120d28
[update: the sourcery task there failed because the versions don't match and I did not include the changes from dbc65f3 in this PR. I have now cherry-picked that in and am running a new patch here. also, I had to restart a few tasks because installing Swift hung forever and they failed]
I will make a forward-port PR to sync/run the new tests on
mainonce we have the corresponding C changes available in their 1.18 release