DRIVERS-1995: Do not error when parsing change stream event documents #1164
Conversation
| document: { | ||
| "_id": 1, | ||
| "a": 1 | ||
| } |
There was a problem hiding this comment.
I'm not aware of any other YAML tests that use braces for multiple lines. That said, we do have plenty that use this to be more concise and express a document on a single line, so I'd suggest the following:
| document: { | |
| "_id": 1, | |
| "a": 1 | |
| } | |
| document: { _id: 1, a: 1 } |
Quoting fields also isn't required so I removed that.
| expectResult: { | ||
| "operationType": "addedInFutureMongoDBVersion", | ||
| "ns": { | ||
| "db": "database0", | ||
| "coll": "collection0" | ||
| } | ||
| } |
There was a problem hiding this comment.
We typically do not hard-code database and collection names. Since these are already defined as *database0 and *collection0 in the entity definitions, we can reuse those here.
| expectResult: { | |
| "operationType": "addedInFutureMongoDBVersion", | |
| "ns": { | |
| "db": "database0", | |
| "coll": "collection0" | |
| } | |
| } | |
| expectResult: | |
| operationType: "addedInFutureMongoDBVersion" | |
| ns: { db: *database0, coll: *collection0 } |
Other tests would typically only define &database0 for the entity identifier and then define another &databaseName0 anchor for its name, but that wasn't done in this test file for reasons unknown to me.
There was a problem hiding this comment.
As you can probably tell, I haven't written a lot of unified spec tests. Thanks for the pointers. Updating the tests as suggested.
| arguments: | ||
| # using $project to simulate future changes to ChangeStreamDocument structure | ||
| pipeline: [ { $project: { operationType: 1, ns: 1, newField: "newFieldValue" } } ] | ||
| saveResultAsEntity: &changeStream0 changeStream0 |
There was a problem hiding this comment.
| saveResultAsEntity: &changeStream0 changeStream0 | |
| saveResultAsEntity: *changeStream0 |
No need to redefine the anchor unless you expect this test might be copied to another file. That said, YAML also allows redefinition (last one wins) so feel free to ignore this suggestion.
There was a problem hiding this comment.
I'm going to leave the tests self-contained in case they are re-ordered or split into separate files in the future. As well having an anchor defined in a previous test seems messy to me.
| document: { | ||
| "_id": 1, | ||
| "a": 1 | ||
| } |
There was a problem hiding this comment.
| document: { | |
| "_id": 1, | |
| "a": 1 | |
| } | |
| document: { _id: 1, a: 1 } |
| expectResult: { | ||
| "operationType": "insert", | ||
| "ns": { | ||
| "db": "database0", | ||
| "coll": "collection0" | ||
| }, | ||
| "newField": "newFieldValue" | ||
| } |
There was a problem hiding this comment.
| expectResult: { | |
| "operationType": "insert", | |
| "ns": { | |
| "db": "database0", | |
| "coll": "collection0" | |
| }, | |
| "newField": "newFieldValue" | |
| } | |
| expectResult: | |
| operationType: "insert" | |
| ns: { db: *database0, coll: *collection0 } | |
| newField: "newFieldValue" |
| document: { | ||
| "_id": 1, | ||
| "a": 1 | ||
| } |
There was a problem hiding this comment.
| document: { | |
| "_id": 1, | |
| "a": 1 | |
| } | |
| document: { _id: 1, a: 1 } |
| object: *collection0 | ||
| arguments: | ||
| pipeline: [ { $project: { optype: "$operationType", ns: 1, newField: "value" } } ] | ||
| saveResultAsEntity: &changeStream0 changeStream0 |
There was a problem hiding this comment.
| saveResultAsEntity: &changeStream0 changeStream0 | |
| saveResultAsEntity: *changeStream0 |
| document: { | ||
| "_id": 1, | ||
| "a": 1 | ||
| } |
There was a problem hiding this comment.
| document: { | |
| "_id": 1, | |
| "a": 1 | |
| } | |
| document: { _id: 1, a: 1 } |
| expectResult: { | ||
| "optype": "insert", | ||
| "ns": { | ||
| "db": "database0", | ||
| "coll": "collection0" | ||
| }, | ||
| "newField": "value" | ||
| } |
There was a problem hiding this comment.
| expectResult: { | |
| "optype": "insert", | |
| "ns": { | |
| "db": "database0", | |
| "coll": "collection0" | |
| }, | |
| "newField": "value" | |
| } | |
| expectResult: | |
| optype: "insert", | |
| ns: { db: *database0, coll: *collection0 } | |
| newField: "value" |
|
|
||
| - description: "Test unknown operationType MUST NOT err" | ||
| runOnRequirements: | ||
| - minServerVersion: "3.6" |
There was a problem hiding this comment.
It doesn't look like any of the tests in this file specify a minServerVersion below 3.6 (some use a greater version). If you'd like to clean this up, I'd propose removing minServerVersion: "3.6" throughout the file (both in your new tests and the few existing ones that shared it) and just adding this to the top-level runOnRequirements.
|
|
||
| - description: "Test unknown operationType MUST NOT err" | ||
| runOnRequirements: | ||
| - minServerVersion: "3.6" |
| document: { | ||
| "_id": 1, | ||
| "a": 1 | ||
| } |
| expectResult: { | ||
| "operationType": "addedInFutureMongoDBVersion", | ||
| "ns": { | ||
| "db": "database0", | ||
| "coll": "collection0" | ||
| } | ||
| } |
There was a problem hiding this comment.
As you can probably tell, I haven't written a lot of unified spec tests. Thanks for the pointers. Updating the tests as suggested.
| arguments: | ||
| # using $project to simulate future changes to ChangeStreamDocument structure | ||
| pipeline: [ { $project: { operationType: 1, ns: 1, newField: "newFieldValue" } } ] | ||
| saveResultAsEntity: &changeStream0 changeStream0 |
There was a problem hiding this comment.
I'm going to leave the tests self-contained in case they are re-ordered or split into separate files in the future. As well having an anchor defined in a previous test seems messy to me.
| document: { | ||
| "_id": 1, | ||
| "a": 1 | ||
| } |
| expectResult: { | ||
| "operationType": "insert", | ||
| "ns": { | ||
| "db": "database0", | ||
| "coll": "collection0" | ||
| }, | ||
| "newField": "newFieldValue" | ||
| } |
|
Squashed and merged manually due to merge conflicts with change stream updates that got in before me. |
Of the two new unified tests,
Test server error on projecting out _id, already passes on the .NET/C# driver. The other unified test,Test projection in change stream returns expected fields, fails but I haven't investigated whether this is a test runner bug or an impl bug. When the change stream is run in the shell, it produces the asserted document, but the unified test in the .NET/C# driver does not.The remaining 3 tests assert compatibility with potential future changes that the server could make. Therefore they have to be written as prose tests asserting that the change stream response parsing machinery can tolerate new operation types, new fields, and changes to the returned
nsdocument.Thanks in advance for any feedback and suggestions.