DRIVERS-1713 Convert change stream tests to unified format #1156

abr-egn · 2022-03-16T16:58:53Z

This PR pulls in the test conversion that was done earlier in mongodb/mongo-rust-driver#570. There was a little bit of merging needed in change-streams.yml because some new tests had been added since the conversion, but otherwise it's all a direct copy.

benjirewis

Thanks for taking on this conversion! 🧑‍🔧 Haven't looked at all the converted tests yet; just a few initial comments.

source/change-streams/tests/unified/change-streams-errors.yml

source/change-streams/tests/unified/change-streams-resume-allowlist.yml

source/change-streams/tests/unified/change-streams-resume-errorLabels.yml

source/change-streams/tests/unified/change-streams.yml

source/change-streams/tests/unified/change-streams-errors.yml

source/change-streams/tests/unified/change-streams-resume-allowlist.yml

source/change-streams/tests/unified/change-streams-resume-errorLabels.yml

jmikola · 2022-03-31T23:57:46Z

source/change-streams/tests/unified/change-streams.yml

+                aggregate: *collection0
+                cursor: {}
+                pipeline:
+                  - $changeStream: {}


Note: drivers that have yet to implement DRIVERS-2231 will fail this assertion. I don't think we need to fix that here, but we should note that here, but it would be good to bring it up in the downstream changes for this issue. I'll also add a "has to be done after" relationship between the issues.

Previously, the POC change stream tests used $$unsetOrMatches for fullDocument here. I opened DRIVERS-2261 to clean that up.

jmikola · 2022-04-01T17:55:02Z

Just POC-ed these tests in PHPLIB, and apart from the comment tests lumped in here (likely from another PR) they passed after fixing a bug in our test runner with $$exists nested within $$unsetOrMatches (see: PHPLIB-831).

Will wait on suggested fixes above to be merged and this PR rebased, and then take a final review pass.

jmikola

I just noticed this PR introduces global entities, which I think may be redundant. Please check that out and report back.

Also left various formatting suggestions that might make the test files more readable/maintainable (disregard as you wish, since those aren't functional changes). I did point out one place where you used a literal string instead of a YAML reference, so that might be worth looking into even if the style suggestions are ignored.

source/change-streams/tests/unified/change-streams-errors.yml

source/change-streams/tests/unified/change-streams-resume-allowlist.yml

source/change-streams/tests/unified/change-streams-errors.yml

source/change-streams/tests/unified/change-streams-resume-allowlist.yml

jmikola · 2022-04-04T17:26:37Z

source/change-streams/tests/unified/change-streams-resume-allowlist.yml

+          pipeline: []
+        saveResultAsEntity: &changeStream0 changeStream0
+      - name: insertOne
+        object: *globalCollection0


I realize the tests in poc-change-streams.yml did use separate collections for running insertOne, but I don't recall why that was done. Interestingly, the current unified tests in change-streams.yml don't do this and share the same collection object used to create the change stream.

I'd see if you can make do without this and, if so, remove all of the global entities added in this PR. I wouldn't object to stripping them from the POC file as well, but we can always punt on that.

The globalClient and associated data entities aren't strictly necessary AFAICT; the tests can be edited to only use client0 and add the insert to the expected command events. I'm not sure that's an improvement on the current setup, though, since it's adding noise to the expected output (these are testing change streams, not insert).

Ah, I re-read the POC spec tests again and it looks like those separate collections did have their own client. That was likely done for ignoring insert in the APM events, as you pointed out.

The tests in change-streams.yml vary. Some don't expect events at all, and others include the insert command.

I suppose a third option is just to add insert to ignoreCommandMonitoringEvents if you'd like to avoid the extra entities, but I won't push for that. Totally up to you and I'm OK with leaving this as-is (thanks for indulging my questions).

Whoops, forgot to actually reply here :) I'd prefer to leave it as-is - none of the other options seem worth the churn.

source/change-streams/tests/unified/change-streams-resume-allowlist.yml

jmikola · 2022-04-04T17:36:19Z

source/change-streams/tests/unified/change-streams.yml

+        - killCursors
+      useMultipleMongoses: false
+  - client:
+      id: &globalClient globalClient


Per my comments above, I'll note that none of the existing tests in this file required global clients, databases, or collections.

That said, if these tests will operate on multiple collections you may want to explicitly initialize those in initialData (as I did in poc-change-streams.yml).

abr-egn · 2022-04-06T20:16:01Z

Still working on the style updates but wanted to continue the discussion of client entities while I do that :)

source/change-streams/tests/unified/change-streams-errors.yml

jmikola · 2022-04-07T15:42:31Z

source/change-streams/tests/unified/change-streams-resume-allowlist.yml

-          - replicaset
-          - sharded
-          - load-balanced
+        topologies: [ replicaset, sharded, load-balanced ]


Is there a particular reason that load-balanced is included here but omitted in most later test requirements?

Separate question: if the main difference in test requirements is server versions, perhaps we can add a top-level runOnRequirements block with `minServerVersion: "3.6.0" and relevant topologies and reduce duplication in this file? Then, test-level requirements can just specify version requirements as needed.

All of the constraints here were just ported straight from the legacy tests; I don't know why they were that way. I consolidated everything to use a top-level list that includes load-balanced (and validated that they pass on the rust driver in that configuration).

source/change-streams/tests/unified/change-streams-resume-errorLabels.yml

jmikola

Tested in PHPLIB. Various tests were skipped due to my local server version and there were a few failures due to us not having implemented comment, but everything looks OK otherwise.

benjirewis

These look great! They seem to work in the Go driver with additional logic for rename and ignoreExtraEvents. Thanks for the careful review responses. There are some merge conflicts that I think you'll have to handle, though.

abr-egn requested a review from a team as a code owner March 16, 2022 16:58

abr-egn requested review from benjirewis and removed request for a team March 16, 2022 16:58

benjirewis reviewed Mar 17, 2022

View reviewed changes

abr-egn requested a review from benjirewis March 21, 2022 14:45

benjirewis reviewed Mar 21, 2022

View reviewed changes

jmikola mentioned this pull request Mar 31, 2022

DRIVERS-1343 Include the "to" field in ChangeStreamDocument #1138

Merged

jmikola reviewed Mar 31, 2022

View reviewed changes

jmikola mentioned this pull request Apr 1, 2022

PHPLIB-652: Convert change streams to unified test format mongodb/mongo-php-library#908

Merged

abr-egn force-pushed the DRIVERS-1713/convert-changestream-tests branch from 4c9112c to 82a6db2 Compare April 4, 2022 15:48

abr-egn requested review from jmikola and benjirewis April 4, 2022 15:50

jmikola reviewed Apr 4, 2022

View reviewed changes

jmikola mentioned this pull request Apr 4, 2022

DRIVERS-1343 Fix topology and delete rename target #1169

Merged

jmikola reviewed Apr 7, 2022

View reviewed changes

jmikola approved these changes Apr 11, 2022

View reviewed changes

benjirewis approved these changes Apr 11, 2022

View reviewed changes

abr-egn added 10 commits April 11, 2022 15:48

import converted tests

a79a7ed

fix schema version

10be007

review updates

c29d3d0

embed options

0e1cb27

newlines

0748138

Fix number formatting and comments

5ee2451

review tweaks

7d41761

spacing

6056a54

avoid $numberInt

51734ed

reflow exists

b414325

abr-egn added 7 commits April 11, 2022 15:48

reflow a lot more things

a34835d

remove a few more $numberInt

450df75

fix globalClient references

002ce69

condense $$unsetOrMatches

4a51919

s/sharded/sharded-replicaset/

79f4093

consolidate topologies for allowlist

3e4bb25

consolidate topologies for errorLabels

e9017ce

abr-egn force-pushed the DRIVERS-1713/convert-changestream-tests branch from 16c0cb8 to e9017ce Compare April 11, 2022 19:48

abr-egn merged commit bb7a784 into mongodb:master Apr 12, 2022

baileympearson mentioned this pull request May 10, 2022

test(NODE-3248): sync new unified change stream tests & enhance runner mongodb/node-mongodb-native#3233

Merged

5 tasks

DRIVERS-1713 Convert change stream tests to unified format #1156

DRIVERS-1713 Convert change stream tests to unified format #1156

Uh oh!

Conversation

abr-egn commented Mar 16, 2022

Uh oh!

benjirewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmikola Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

jmikola commented Apr 1, 2022

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmikola Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abr-egn Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

jmikola Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

abr-egn Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

abr-egn commented Apr 6, 2022

Uh oh!

Uh oh!

Uh oh!

jmikola Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

abr-egn Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

benjirewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola Apr 4, 2022 •

edited

Loading

abr-egn Apr 7, 2022 •

edited

Loading