Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRIVERS-2294 Add createCollection and collMod tests for changeStreamPreAndPostImages #1206

Merged
merged 4 commits into from
May 5, 2022
Merged

DRIVERS-2294 Add createCollection and collMod tests for changeStreamPreAndPostImages #1206

merged 4 commits into from
May 5, 2022

Conversation

benjirewis
Copy link
Contributor

@benjirewis benjirewis commented May 3, 2022

DRIVERS-2294

Adds spec tests for the new changeStreamPreAndPostImages option for the createCollection and modifyCollection operations. Updates the collection management README to not mention schema version 1.0 (new tests require > 1.0).

Here's a POC in Go and a patch with passing tests.

@benjirewis benjirewis requested a review from jmikola May 3, 2022 18:04
@benjirewis benjirewis marked this pull request as ready for review May 3, 2022 18:04
@benjirewis benjirewis requested a review from a team as a code owner May 3, 2022 18:04
@benjirewis benjirewis requested review from kmahar and removed request for a team May 3, 2022 18:04
arguments:
databaseName: *database0Name
collectionName: *collection0Name
- name: modifyCollection
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Go driver has no modifyCollection helper, so if someone else could POC this test that'd be great 🙇

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do any drivers actually have collMod helpers? (Swift and Rust do not)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmikola It sounds like it might just be PHP. Any thoughts on making this an integration test within the PHP driver?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not too much trouble, I think leaving it in place and having drivers skip it would be a good exercise for ensuring each driver has some mechanism to skip unified spec tests as needed.

But no objections if you'd rather remove this. I can test in PHP easily.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not in general opposed to requiring drivers update their runners to be able to easily skip these kinds of tests if they can’t already. But I don’t know if we should bundle that with required 6.0 changes everyone is on a tight timeline for. I’d be more inclined to make the skipping capability a unified test runner requirement and add sample tests drivers need to skip to prove they can do it, or something like that.

If we do leave it, could we at least put it in a separate file? In Swift (and I believe also Rust) we error while decoding a JSON file into our native test representation type if any operation names are unrecognized, since we have a concrete type modeling each recognized operation.
We can easily skip decoding a file altogether. But currently to skip a test with an unsupported operation within a file of tests we otherwise want to run, I think we’d need to do something like add dummy modifyCollection operations to allow decoding to succeed, or do some kind of larger refactor to more gracefully support this type of thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that, I propose we split these tests into two files:

  • createCollection-pre_and_post_images
  • modifyCollection-pre_and_post_images

Then, drivers can skip as needed.

Additionally, I asked @benjirewis to add a DRIVERS ticket to add a valid-fail test for operation-unsupported (similar to operation-failure, except that test runs a non-existent server command).

@kmahar: Separately, I think you'll want to address the decoding issue in Swift/Rust ASAP. @benjirewis mentioned that the CSOT tests will likely include a whole bunch of operations in one file (some likely unimplemented in Swift). If you think this warrants a DRIVERS ticket first to require all drivers be capable of selective skipping (vs. entire files), feel free to mandate the work through that -- I think it'd be easy for already-compliant drivers to mark downstream tickets as "works as designed" if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split into two different files and created DRIVERS-2313 to add a valid-fail test for unsupported operations.

Copy link
Member

@ShaneHarvey ShaneHarvey May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, the python test runner will skip tests that use helpers pymongo does not support. For example:

        # Some tests need to be skipped based on the operations they try to run.
        for op in spec["operations"]:
            name = op["name"]
            if name == "count":
                self.skipTest("PyMongo does not support count()")
            if name == "listIndexNames":
                self.skipTest("PyMongo does not support list_index_names()")
            if client_context.storage_engine == "mmapv1":
                if name == "createChangeStream":
                    self.skipTest("MMAPv1 does not support change streams")
                if name == "withTransaction" or name == "startTransaction":
                    self.skipTest("MMAPv1 does not support document-level locking")

When we sync these tests we would just need to add "modifyCollection" to the skip condition which I'm fine with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go does not yet have the logic you've described above for Python, but I'll add it as part of GODRIVER-2396. We currently have a global list of tests that need to be skipped, which I've been adding to locally in recent changes that have tests with unsupported operations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for splitting the tests; and good to know @jmikola, I think we'll be able to add some kind of workaround for that without a ton of trouble

@benjirewis benjirewis merged commit d967891 into mongodb:master May 5, 2022
@benjirewis benjirewis deleted the createCollCSImages.2294 branch May 5, 2022 22:29
baileympearson added a commit to mongodb/node-mongodb-native that referenced this pull request May 17, 2022
- changes included in this PR - mongodb/specifications#1206
- also skips the modifyCollection helper test because node doesn't implement a
  modify collection helper
baileympearson added a commit to mongodb/node-mongodb-native that referenced this pull request May 18, 2022
- changes included in this PR - mongodb/specifications#1206
- also skips the modifyCollection helper test because node doesn't implement a
  modify collection helper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants