Skip to content

Conversation

PavelSafronov
Copy link
Contributor

Description

Summary of Changes

Migrating node-mongodb-native/test/unit/assorted driver callback API style tests to promises.

Notes for Reviewers

Repro/test steps:

  1. comment out line 229 in test/mongodb.ts (this disables MongoDB legacy)
  2. run npm run check:unit
  3. note failures in test/unit/assorted/sessions_client.test.ts
  4. apply fix
  5. run npm run check:unit
  6. note that none of the failures are coming from test/unit/assorted/sessions_client.test.ts
  7. uncomment line 229 in test/mongodb.ts
  8. run npm run check:unit
  9. note all tests are passing

What is the motivation for this change?

https://jira.mongodb.org/browse/NODE-4979

We want to modify all tests to no longer use callbacks as the driver is a promise only API now. This is a requirement for deprecating the legacy driver.

Double check the following

  • Lint is passing (npm run check:lint)
  • 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

@PavelSafronov PavelSafronov requested a review from a team as a code owner September 17, 2025 15:54
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.

From the test refactor ticket:

Update the client import in each updated file to use src instead of legacy

Can we also update each file in this directory to import from src, instead of from ../../mongodb?

Additionally, going forward, can we put any JS->TS conversions into their own commit? That helps with review, because you can then view the diff on subsequent commits to see the substantive changes. The diff in this PR is unhelpful

@baileympearson baileympearson self-assigned this Sep 17, 2025
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 17, 2025
@dariakp dariakp changed the title fix(NODE-7160): migrate node-mongodb-native/test/unit/assorted tests test(NODE-7160): migrate node-mongodb-native/test/unit/assorted tests Sep 17, 2025
@dariakp
Copy link
Contributor

dariakp commented Sep 17, 2025

@PavelSafronov Just a note that I updated the conventional commit tag in the PR title to "test". We reserve "fix" for bug fixes of source code.

@PavelSafronov
Copy link
Contributor Author

@baileympearson Thanks for the comments here and offline!

As discussed, we are removing one of the lint rules that blocks importing of src/ files. That removal resulted in the removal of a lot of eslint suppressions (after running npm run fix:eslint), so that's why we're seeing so many changes outside of node-mongodb-native/test/unit/assorted dir.

Aside from that, the node-mongodb-native/test/unit/assorted changes have been mostly to update imports and rewrite some of the callback-based tests to use promises.

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.

Just a few small comments. Looks good! And thanks for removing our lint rule that we no longer need.

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 also failing on other branches. LGTM

@baileympearson baileympearson merged commit e8a91a9 into mongodb:main Sep 19, 2025
23 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Primary Review In Review with primary reviewer, not yet ready for team's eyes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants