Skip to content

test(NODE-7513): relax regression test for emptyGetMore #4916

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

Open
tadjik1 wants to merge 2 commits into
base: main
Choose a base branch
from
+5 −7
Open

test(NODE-7513): relax regression test for emptyGetMore #4916

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfa3e10
test(NODE-7513): relax regression test for emptyGetMore
tadjik1 Apr 10, 2026
7fff5df
update the comment
tadjik1 Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions test/integration/crud/find.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import * as sinon from 'sinon';

import {
Code,
CursorResponse,
Long,
type MongoClient,
MongoServerError,
Expand Down Expand Up @@ -1083,18 +1082,17 @@ describe('Find', function () {
await collection.deleteMany({});
await collection.insertMany(Array.from({ length: 4 }, (_, i) => ({ x: i })));

const getMoreSpy = sinon.spy(CursorResponse, 'emptyGetMore', ['get']);

const cursor = collection.find({}, { batchSize: 1, limit: 3 });
// emptyGetMore is used internally after limit + 1 documents have been iterated
// Iterate limit + 1 times to exercise the post-limit cursor path before rewind.
await cursor.next();
await cursor.next();
await cursor.next();
await cursor.next();

// assert that `emptyGetMore` is called. if it is not, this test
// always passes, even without the fix in NODE-6878.
expect(getMoreSpy.get).to.have.been.called;
Copy link
Copy Markdown
Contributor

@dariakp dariakp Apr 10, 2026

Choose a reason for hiding this comment

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

This is the one and only assertion in this test, without it, what is it testing? The test title says that this is a regression test for NODE-6878 and is specifically about ensuring CursorResponse.emptyGetMore contains all CursorResponse fields. Based on your investigation regarding server 9.0, it would seem the test isn't relevant to servers 9.0+, so it should be skipped on just those configurations and continue being executed on the rest.

Copy link
Copy Markdown
Member Author

@tadjik1 tadjik1 Apr 10, 2026

Choose a reason for hiding this comment

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

The spy doesn't actually test the fix - it only checks that emptyGetMore was called. The actual regression guard is cursor.rewind(); await cursor.toArray() - that's what would throw if emptyGetMore were broken again (returning an object without the proper CursorResponse shape causes documents.clear() to TypeError).

So there are two scenarios:

  1. Servers < 9.0 (sharded): emptyGetMore IS still called (server returns non-zero cursorId after limit). If someone reintroduced the bug, the rewind() + toArray() would throw - the regression is caught without the spy.
  2. Servers 9.0+ (sharded): emptyGetMore is never called (server returns cursorId: 0). The bug cannot manifest on this path - there's no broken object to trip over. So there's nothing to regress against.

// On servers < 9.0, mongos returns a non-zero cursorId even after the limit
// is satisfied, so the driver uses CursorResponse.emptyGetMore to avoid a
// wasted getMore. On 9.0+, mongos returns cursorId: 0 when the limit is
// reached. In both cases, rewinding and re-iterating the cursor must work.

cursor.rewind();

Expand Down
Loading