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-3698): sync spec tests for crud operations with let option #3147

Merged
merged 11 commits into from
Feb 18, 2022

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Feb 15, 2022

Description

What is changing?

Support for the let option was added in: #2829

This PR pulls in spec changes from the following PRs:

Is there new documentation needed for these changes?

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>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

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.

@baileympearson Did you double check that our unified test runner definitely and correctly parses the specified let option input and expectations? (I know we made the runner adjustments in the original PR, but I am a bit paranoid about these things after all the misses we've discovered after the fact, so always worth a second look)

@baileympearson
Copy link
Contributor Author

Did you double check that our unified test runner definitely and correctly parses the specified let option input and expectations? (I know we made the runner adjustments in the original PR, but I am a bit paranoid about these things after all the misses we've discovered after the fact, so always worth a second look)

@dariakp yes, I've confirmed that the let option is being parsed as expected

@jyemin jyemin removed their request for review February 16, 2022 19:31
@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 18, 2022
@dariakp dariakp self-assigned this Feb 18, 2022
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.

Just a small issue with whitespaces, otherwise LGTM

test/spec/crud/unified/deleteMany-let.json Outdated Show resolved Hide resolved
test/spec/crud/unified/deleteOne-let.json Outdated Show resolved Hide resolved
test/spec/crud/unified/find-let.json Outdated Show resolved Hide resolved
test/spec/crud/unified/findOneAndReplace-let.json Outdated Show resolved Hide resolved
test/spec/crud/unified/findOneAndUpdate-let.json Outdated Show resolved Hide resolved
test/spec/crud/unified/updateMany-let.json Outdated Show resolved Hide resolved
test/spec/crud/unified/updateOne-let.json Outdated Show resolved Hide resolved
@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Feb 18, 2022
@dariakp dariakp merged commit 25d22b2 into main Feb 18, 2022
@dariakp dariakp deleted the NODE-3698-add-let-option-to-crud-commands branch February 18, 2022 16:58
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.

4 participants