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

test(NODE-4965): unskip FLE range index spec tests and implement spec prose tests #3530

Merged
merged 17 commits into from
Jan 24, 2023

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Jan 20, 2023

Description

What is changing?

This PR adds spec tests for Decimal128 range index queries, unskips the existing range index tests and adds all the prose tests related to range indexes.

CI will be mostly red until mongodb/libmongocrypt#533 merges. Serverless and mongosh failures are failing on main too.

We can confirm that the changes work as expected on the custom dependency tests from a pinned commit - logs here.

Is there new documentation needed for these changes?

Not in the driver, no.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken changed the title test: unskip csfle tests test(NODE-4965): unskip FLE range index spec tests and implement spec prose tests Jan 20, 2023
@nbbeeken nbbeeken marked this pull request as ready for review January 20, 2023 18:33
@nbbeeken nbbeeken self-requested a review January 20, 2023 18:33
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 20, 2023
@@ -2132,7 +2132,7 @@ tasks:
- func: bootstrap kms servers
- func: run custom csfle tests
vars:
CSFLE_GIT_REF: ff9e095eaf72f9e442761f69080dae159a395d94
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pointing at your branch correct? just leaving open so we can update this to the commit on main when its there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. once my PR on libmongocrypt merges, we can update this to the latest

@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jan 23, 2023
nbbeeken
nbbeeken previously approved these changes Jan 23, 2023
@baileympearson baileympearson force-pushed the NODE-4965-implement-range-index-prose-tests branch from ad17243 to 7595e7f Compare January 23, 2023 22:15
nbbeeken
nbbeeken previously approved these changes Jan 24, 2023
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

LGTM

dariakp
dariakp previously approved these changes Jan 24, 2023
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

LGTM on the naming convention

@nbbeeken nbbeeken merged commit f7961a7 into main Jan 24, 2023
@nbbeeken nbbeeken deleted the NODE-4965-implement-range-index-prose-tests branch January 24, 2023 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants