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-2124: test that inserts and upserts respect all _id values #1696

Closed
wants to merge 1 commit into from

Conversation

nbbeeken
Copy link
Contributor

Please complete the following before merging:

  • Update changelog.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded
    clusters, and serverless).

@nbbeeken nbbeeken force-pushed the DRIVERS-2124-id-types branch 2 times, most recently from b2bb775 to ca4d811 Compare October 29, 2024 18:46
@nbbeeken nbbeeken force-pushed the DRIVERS-2124-id-types branch from ca4d811 to fcd0a58 Compare October 29, 2024 20:54
@nbbeeken nbbeeken changed the title DRIVERS-2124: test that inserts and upserts respect null _id values DRIVERS-2124: test that inserts and upserts respect all _id values Oct 29, 2024
tests:
- description: inserting _id with type double via insertOne
operations:
- {name: dropCollection, object: database, arguments: {collection: type_tests}}
Copy link
Member

Choose a reason for hiding this comment

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

Drop database and collection are expensive, slow commands. If we're adding so many tests it would be good to avoid it.

Also do we really need to test every combination of operation and BSON type?

Copy link
Member

@ShaneHarvey ShaneHarvey Oct 29, 2024

Choose a reason for hiding this comment

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

Perhaps it would be faster to put all the operations in one test? There's a lot of overhead to setting up a unified test which really adds up in runtime cost when adding 150 tests.

@@ -31,8 +32,8 @@ jobs:
python3 ./source/client-side-encryption/etc/generate-test.py ./source/client-side-encryption/etc/test-templates/*.template ./source/client-side-encryption/tests/legacy
python3 ./source/client-side-operations-timeout/etc/generate-basic-tests.py ./source/client-side-operations-timeout/etc/templates ./source/client-side-operations-timeout/tests
python3 ./source/etc/generate-handshakeError-tests.py
node ./source/etc/generate-crud-id-type-tests.mjs
Copy link
Member

Choose a reason for hiding this comment

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

Nit: since all other scripts to generate test files are written in Python, should this one really be written in a different language?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't say no to php scripts :)

@nbbeeken
Copy link
Contributor Author

Thanks for the comments :) we're just going to test the null _id cases in my other PR

@nbbeeken nbbeeken closed this Oct 30, 2024
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.

3 participants