DRIVERS-2630: add e2e testing for search index commands and clarifications to search index spec #1442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
baileympearson
changed the title
GromNaN
approved these changes
Jul 17, 2023
baileympearson
requested review from
dariakp and
GromNaN
and removed request for
a team
5 tasks
GromNaN
reviewed
Jul 18, 2023
There was a problem hiding this comment.
Tests implemented with success in PHP: tests/Collection/SearchIndexFunctionalTest.php
To improve the coverage, I suggest adding this e2e tests:
- Call to
createSearchIndexeswith an empty list of indexes. - Call
createSearchIndexwithoutname. It fallbacks to the "default" name.
GromNaN
approved these changes
Jul 20, 2023
dariakp
requested changes
Jul 24, 2023
dariakp
reviewed
Jul 25, 2023
joshbsiegel
reviewed
Jul 25, 2023
baileympearson
commented
Jul 25, 2023
Comment on lines
416
to
420
| * or language/implementation equivalent. | ||
| * | ||
| * If drivers are unable to make the IndexView iterable, they MAY opt to provide the options for | ||
| * listing search indexes via the `list` method instead of the `Collection.listSearchIndexes` method. | ||
|
|
There was a problem hiding this comment.
After discussion with @dariakp on slack, we decided that a more consistent API is to include the name in an options object. There were two reasons for this change:
- It's odd to have a set of search index management methods that have a name as the first parameter and an optional second parameter in others. It makes more sense to include an optional name in an "options" object.
- This is more closely aligned with the regular index management API.
There was a problem hiding this comment.
I understand this was already allowed, this is how it is already implemented in PHP.
dariakp
approved these changes
Jul 25, 2023
kevinAlbs
reviewed
Jul 26, 2023
Co-authored-by: Kevin Albertson <kevin.albertson@mongodb.com>
kevinAlbs
approved these changes
Jul 27, 2023
dariakp
approved these changes
Jul 27, 2023
dariakp
reviewed
Jul 27, 2023
baileympearson
commented
Jul 27, 2023
kevinAlbs
added a commit
to kevinAlbs/mongo-c-driver
that referenced
this pull request
Jul 31, 2023
kevinAlbs
added a commit
to kevinAlbs/mongo-c-driver
that referenced
this pull request
Jul 31, 2023
kevinAlbs
added a commit
to kevinAlbs/mongo-c-driver
that referenced
this pull request
Jul 31, 2023
Do not intend to merge. This was used to help review PR: mongodb/specifications#1442
kevinAlbs
added a commit
to mongodb/mongo-c-driver
that referenced
this pull request
Aug 1, 2023
* add index-management tests From PR mongodb/specifications#1442 * add test search operations and implement without helpers * document management of search indexes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does three things:
The third bullet can be separated into a different PR if necessary.
Node POC: mongodb/node-mongodb-native#3736
Notes
Why have unified tests and prose tests?
The initial plan was to enhance the unified test runner to support the new search index management commands. There ended up being complications integrating support for the new commands into the UTR (the largest issue was that the commands are asynchronous, and return from the server before the changes have finished). The unified tests were already written and expected errors when run against non-Atlas deployments, so they effectively serve as unit tests for drivers without writing another specialized test format. The new prose tests add end-to-end functionality for drivers' implementations.
Why setup/teardown a cluster for each test?
CSOT
These commands should work with CSOT as per the CSOT spec, but looking through the specs repo we don't seem to have tests for every driver helper that supports
timeoutMS. Consequently, I chose not to add tests for these helpers withtimeoutMS.Please complete the following before merging: