Skip to content

test(NODE-4586): optional callback async wrappers #2

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 28 commits into from
Sep 6, 2022
Merged

test(NODE-4586): optional callback async wrappers #2

merged 28 commits into from
Sep 6, 2022

Conversation

@nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Aug 18, 2022 •
edited by dariakp
Loading

NODE-4520 / NODE-4586

  • Test all the wrapped APIs
  • Add coverage reporting
  • test latest driver on one target, its a slow task no need to dupe it
  • fixups to wrappers

@nbbeeken nbbeeken changed the base branch from main to NODE-4520/add-legacy-callback-client August 18, 2022 19:52
@nbbeeken nbbeeken force-pushed the test/NODE-4520/maybe_callback branch from 79f9d5b to b59790c Compare August 18, 2022 20:01
@nbbeeken nbbeeken force-pushed the NODE-4520/add-legacy-callback-client branch from 67e615e to 745512a Compare August 18, 2022 21:47
@nbbeeken nbbeeken force-pushed the test/NODE-4520/maybe_callback branch from b59790c to 6a11426 Compare August 18, 2022 21:47
@nbbeeken nbbeeken force-pushed the NODE-4520/add-legacy-callback-client branch 2 times, most recently from ff9ef1f to 023a4c4 Compare August 20, 2022 01:08
@nbbeeken nbbeeken force-pushed the test/NODE-4520/maybe_callback branch from 6a11426 to b1ab63e Compare August 20, 2022 01:26
@nbbeeken nbbeeken force-pushed the NODE-4520/add-legacy-callback-client branch 2 times, most recently from 93901eb to 0d0558b Compare August 22, 2022 19:18
Base automatically changed from NODE-4520/add-legacy-callback-client to main August 25, 2022 19:10
@nbbeeken nbbeeken force-pushed the test/NODE-4520/maybe_callback branch 3 times, most recently from 03fa0c5 to 6f9f4a4 Compare August 25, 2022 21:18
@nbbeeken nbbeeken changed the title test(NODE-4520): optional callback async wrappers test(NODE-4586): optional callback async wrappers Aug 25, 2022
@nbbeeken nbbeeken force-pushed the test/NODE-4520/maybe_callback branch 7 times, most recently from 3bf1999 to 25e1bec Compare August 26, 2022 20:15
@nbbeeken nbbeeken marked this pull request as ready for review August 26, 2022 20:17
baileympearson
baileympearson requested changes Aug 29, 2022
View reviewed changes
Copy link
Collaborator

@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.

This PR looks good overall, but I would like maybe_callback.test.js to have thorough comments explaining exactly what we're testing and how we're constructing the tests (like we discussed). I'll take another look once there are comments @nbbeeken

test/unit/legacy_wrappers/collection.test.js Outdated
});

it('should return legacy listIndexes cursor', () => {
expect(collection.listIndexes()).to.be.instanceOf(ListIndexesCursor);
Copy link
Collaborator

@baileympearson baileympearson Aug 29, 2022

Choose a reason for hiding this comment

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

optional - this comment applies to all the custom tests in the legacy_wrappers folder. It's confusing at first glance to read this test, because I'd expect to.be.instanceOf(LegacyListIndexesCursor) based on the test description.

I understand why it's not - because we're exporting the exact same names from the legacy driver as from the original driver. But we could consider not destructuring imports to make it more explicit:

const legacyDriver = require('../../../src/index')
...
expect(legacyDriver.collection.listIndexes()).to.be.instanceOf(legacyDriver.ListIndexesCursor);

baileympearson
baileympearson previously approved these changes Aug 30, 2022
View reviewed changes
Copy link
Collaborator

@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.

two small comment suggestions but LGTM! 🔥

dariakp
dariakp requested changes Aug 30, 2022
View reviewed changes
@nbbeeken nbbeeken force-pushed the test/NODE-4520/maybe_callback branch from ed3f688 to 4ee81c3 Compare September 6, 2022 18:05
@baileympearson baileympearson merged commit 3245fa1 into main Sep 6, 2022
@baileympearson baileympearson deleted the test/NODE-4520/maybe_callback branch September 6, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@baileympearson baileympearson baileympearson approved these changes

@dariakp dariakp dariakp approved these changes

Assignees

No one assigned

Labels

Team Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nbbeeken @baileympearson @dariakp