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

feat(NODE-4078): allow comment with estimated doc count #3301

Merged
merged 4 commits into from Jun 30, 2022
Merged

Conversation

durran
Copy link
Member

@durran durran commented Jun 27, 2022

Description

Allows comment to be provided with estimated document count.

What is changing?

Sets the comment on the command and adds the new unified tests.

Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-4078

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

@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 27, 2022
baileympearson
baileympearson previously approved these changes Jun 27, 2022
@bajanam bajanam added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 27, 2022
@@ -0,0 +1,31 @@
import { expect } from 'chai';

describe('Collection', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to add an integration test for this, I think we should add it with the others. Adding tests for all falsy values should be as easy as adding ['estimatedDocumentCount', { } ] to the array of commands here:

['findOneAndReplace', { filter: { _id: 1 }, replacement: { x: 12 } }] as const

Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved over to that test.

Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

requesting changes for visibility

baileympearson
baileympearson previously approved these changes Jun 30, 2022
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

One small but non blocking suggestion. I confirmed that the new tests are running and passing. LGTM!

Screen Shot 2022-06-30 at 8 15 16 AM

Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
@durran
Copy link
Member Author

durran commented Jun 30, 2022

@baileympearson I committed your suggestion. Just waiting on evg run now.

Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

failing tests are unrelated to these changes. LGTM 😄

@baileympearson baileympearson merged commit bed1fe3 into main Jun 30, 2022
@baileympearson baileympearson deleted the NODE-4078 branch June 30, 2022 17:56
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
4 participants