Skip to content

Conversation

PavelSafronov
Copy link
Contributor

Description

Summary of Changes

Migrating test/integration/change-streams driver callback API style tests to promises.

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
  • Changes are covered by tests

@PavelSafronov PavelSafronov requested a review from a team as a code owner September 25, 2025 20:40
Comment on lines 639 to 641
changeStream.on('error', async () => {
await changeStream.close();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

a couple of things

first, event emitters internally do not handle async functions (they do not await them, and do not handle rejections). ex:

const ee = new EventEmitter();

ee.on('data', async () => {
  await setTimeout(1000);
  console.log('data listener finished.');
});

ee.emit('data');
console.log('data listener emitted.');

// output:
// data listener emitted.
// data listener finished.
const ee = new EventEmitter();

try {
  ee.on('data', async () => {
    throw new Error('ahh');
  });
} catch (e) {
  console.error(`caught error: ${e}`);
}

ee.emit('data');

// output:
// data listener emitted.
// /Users/bailey.pearson/dev/node-mongodb-native/example.js:8
//     throw new Error('ahh');
//           ^

// Error: ahh
//     at EventEmitter.<anonymous> (/Users/bailey.pearson/dev/node-mongodb-native/example.js:8:11)
//     at EventEmitter.emit (node:events:524:28)
//     at Object.<anonymous> (/Users/bailey.pearson/dev/node-mongodb-native/example.js:14:4)
//     at Module._compile (node:internal/modules/cjs/loader:1529:14)
//     at Module._extensions..js (node:internal/modules/cjs/loader:1613:10)
//     at Module.load (node:internal/modules/cjs/loader:1275:32)
//     at Module._load (node:internal/modules/cjs/loader:1096:12)
//     at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:164:12)
//     at node:internal/main/run_main_module:28:49

If you need to do something asynchronously after an event is emitted, it's easiest to use the async once and use promises:

const errorPromise = once(changeStream, 'error');
// later, where needed
await errorPromise;
await changeStream.close();

Copy link
Contributor

@baileympearson baileympearson Sep 29, 2025

Choose a reason for hiding this comment

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

For this specific test, it's a bit trickier because we do need an error listener on the CS so that if the CS does error and close without emitting the expected change events, the test doesn't hang. But we don't want to wait on the error listener, because we don't actually expect an error. Fortunately, the async version of on():

  1. returns an async iterator of events
  2. also attaches an error listener and rejects if an error event is emitted

So we can use it like so (something like this should work, although I haven't actually run this code):

const changes = on(changeStream, 'change');

try {
  await once(changeStream.cursor, 'init');
  await coll.insertOne({ x: 2 }, { writeConcern: { w: 'majority', j: true } });
  await coll.insertOne({ x: 3 }, { writeConcern: { w: 'majority', j: true } });

  for await (const change of changes) {
    // add the assertion switch statement here
  }
  const change1 = await changes.next();
} finally {
    await changeStream.close().catch(e => {}); // close the CS, squash errors.  if we don't do this and close throws, this will squash an error thrown from inside the try-block
}

just a note - usually we use beforeEach and afterEach hooks to ensure that resources are cleaned up instead of using try-finally blocks inside tests. Either way is fine for now, but if we were writing new tests we'd prefer the test hook approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much for such a thorough explanation! I took another go at it and rewrote the test. It's not exactly what you suggested, but I think it maintains the same behavior.

@baileympearson baileympearson self-assigned this Sep 29, 2025
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 29, 2025
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.

2 participants