test(NODE-6626): implement integration tests for improved client.close() - server-side #4367

aditi-khare-mongoDB · 2025-01-10T22:20:42Z

Description

Add server-side resource integration tests for client.close(). These tests will remain skipped until improved client.close() is implemented.

What is changing?

Assert server-side resource creation and clean-up upon client.close()

Is there new documentation needed for these changes?

No

What is the motivation for this change?

Improved client.close()

Release Highlight

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
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

test/integration/node-specific/client_close.test.ts

baileympearson

One comment about unskipping tests that pass, and some optional comments. Otherwise LGTM

test/integration/node-specific/client_close.test.ts

The base branch was changed.

author Aditi Khare <aditi.khare@mongodb.com> 1734376794 -0500 committer Aditi Khare <aditi.khare@mongodb.com> 1737656635 -0500 parent 70d476a author Aditi Khare <aditi.khare@mongodb.com> 1734376794 -0500 committer Aditi Khare <aditi.khare@mongodb.com> 1737656626 -0500 parent 70d476a author Aditi Khare <aditi.khare@mongodb.com> 1734376794 -0500 committer Aditi Khare <aditi.khare@mongodb.com> 1737656617 -0500 parent 70d476a author Aditi Khare <aditi.khare@mongodb.com> 1734376794 -0500 committer Aditi Khare <aditi.khare@mongodb.com> 1737656617 -0500 parent 70d476a author Aditi Khare <aditi.khare@mongodb.com> 1734376794 -0500 committer Aditi Khare <aditi.khare@mongodb.com> 1737656613 -0500 parent 70d476a author Aditi Khare <aditi.khare@mongodb.com> 1734376794 -0500 committer Aditi Khare <aditi.khare@mongodb.com> 1737656583 -0500 parent 70d476a author Aditi Khare <aditi.khare@mongodb.com> 1734376794 -0500 committer Aditi Khare <aditi.khare@mongodb.com> 1737656581 -0500 skeleton skeleton updates refactor for table fix remove misc file clean up Delete logs.txt requested changes: additional expectation remove extra file ready for review server selection test fix

clean up 2 clean up 2 clean up

test/integration/node-specific/client_close.test.ts

txn test fix 2 lint

nbbeeken

"the server-side ServerSession is cleaned up by client.close():"

test is failing, this one we can unskip but I think the issue is something with the namespace already/not existing?

aditi-khare-mongoDB · 2025-01-29T20:47:33Z

@nbbeeken Error should be gone now but I'll wait for tests to pass on full CI before re-requesting review.

I'm skipping the sessions tests on versions < 4.2 since that's what the $currentOp API requires to detect idleSessions

test/integration/node-specific/client_close.test.ts

nbbeeken

If CI is green, LGTM!

nbbeeken · 2025-01-30T17:10:22Z

test/integration/node-specific/client_close.test.ts

-            );
+            const isCursor = (r) => ((r.type === 'idleCursor') || (r.type === 'op' && r.desc === 'getMore'));
+            return res.cursor.firstBatch.filter(r => isCursor(r) && r.ns === 'close.cursorCleanUp');


Shouldn't we address the outstanding cursors this found? seems like client.close() isn't doing what it promises to if we are seeing cursors from other tests here.

Yep, I reverted the changes, and I'll look into the outstanding cursors

bug fixed

nbbeeken · 2025-01-31T17:52:38Z

src/cursor/abstract_cursor.ts

@@ -1061,6 +1059,8 @@ export abstract class AbstractCursor<
        if (!this.cursorSession?.inTransaction()) {
          maybeClearPinnedConnection(this.cursorSession, { error });
        }
+        this.cursorId = Long.ZERO;
+        this.isKilled = true;


Interesting that this related to fixing the server side issue. We've changed the meaning of killed include cursors that didn't actually run kill cursors

/** * A `killCursors` command was attempted on this cursor. * This is performed if the cursor id is non zero. */

I'm not sure that's a change we can make here, maybe there's another approach to whatever moving this variable addressed?

aditi-khare-mongoDB changed the base branch from main to NODE-6620/sockets January 10, 2025 22:21

aditi-khare-mongoDB changed the title ~~test(NODE-6626): Server Resource Integration Tests~~ test(NODE-6626): Implement integration tests for improved client.close() - server-side Jan 10, 2025

aditi-khare-mongoDB changed the title ~~test(NODE-6626): Implement integration tests for improved client.close() - server-side~~ test(NODE-6626): implement integration tests for improved client.close() - server-side Jan 10, 2025

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch 3 times, most recently from 34ccf67 to 804c464 Compare January 10, 2025 23:00

aditi-khare-mongoDB marked this pull request as ready for review January 13, 2025 20:01

aditi-khare-mongoDB commented Jan 13, 2025

View reviewed changes

test/integration/node-specific/client_close.test.ts Outdated Show resolved Hide resolved

baileympearson self-assigned this Jan 13, 2025

baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 13, 2025

baileympearson requested changes Jan 13, 2025

View reviewed changes

test/integration/node-specific/client_close.test.ts Outdated Show resolved Hide resolved

test/integration/node-specific/client_close.test.ts Outdated Show resolved Hide resolved

test/integration/node-specific/client_close.test.ts Outdated Show resolved Hide resolved

aditi-khare-mongoDB requested a review from baileympearson January 21, 2025 20:40

aditi-khare-mongoDB force-pushed the NODE-6620/sockets branch from ddb638d to 13a8a25 Compare January 22, 2025 23:31

baileympearson previously approved these changes Jan 23, 2025

View reviewed changes

baileympearson added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jan 23, 2025

Base automatically changed from NODE-6620/sockets to main January 23, 2025 17:04

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch 3 times, most recently from e34d285 to d36e661 Compare January 23, 2025 18:29

clean up

08d8c26

clean up 2 clean up 2 clean up

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch from 8adfee6 to 08d8c26 Compare January 23, 2025 18:33

lint

24dcee8

nbbeeken self-requested a review January 23, 2025 21:49

nbbeeken requested changes Jan 23, 2025

View reviewed changes

test/integration/node-specific/client_close.test.ts Outdated Show resolved Hide resolved

test/integration/node-specific/client_close.test.ts Show resolved Hide resolved

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch from 145c987 to ba68560 Compare January 23, 2025 23:01

txn test fix

6a70114

txn test fix 2 lint

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch from 9712d52 to 6a70114 Compare January 23, 2025 23:02

aditi-khare-mongoDB and others added 3 commits January 24, 2025 15:05

Merge branch 'main' into NODE-6626/server-side

e719fa9

test fixes

689706b

Merge branch 'main' into NODE-6626/server-side

74673f8

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch from 7348d39 to 74673f8 Compare January 28, 2025 16:03

await collection creation - fix txn test

6629343

aditi-khare-mongoDB requested a review from nbbeeken January 28, 2025 16:43

nbbeeken requested changes Jan 28, 2025

View reviewed changes

W-A-James and others added 2 commits January 29, 2025 15:44

test(NODE-4955): sync unacknowledged write spec tests (#4380)

729c269

failing test fixed

f2f8dce

currentOp requires >=4.2

35435db

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch from ecc387b to 35435db Compare January 29, 2025 21:33

Merge branch 'main' into NODE-6626/server-side

0f7d8ba

aditi-khare-mongoDB requested a review from nbbeeken January 29, 2025 22:06

nbbeeken requested changes Jan 30, 2025

View reviewed changes

requested changes

3c98214

nbbeeken previously approved these changes Jan 30, 2025

View reviewed changes

$currentOp idleCursors needs >= 4.2

f53ecc6

aditi-khare-mongoDB dismissed nbbeeken’s stale review via f53ecc6 January 30, 2025 16:31

nbbeeken requested changes Jan 30, 2025

View reviewed changes

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch 2 times, most recently from f11de93 to f53ecc6 Compare January 30, 2025 17:58

nbbeeken added the wip label Jan 31, 2025

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch from f031f94 to e6aac85 Compare January 31, 2025 17:42

aditi-khare-mongoDB requested a review from a team as a code owner January 31, 2025 17:42

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch from 5dea817 to 25fc9a6 Compare January 31, 2025 17:43

bug fixed

1d3fd91

bug fixed

aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch from 02183c2 to 1d3fd91 Compare January 31, 2025 17:44

Merge branch 'main' into NODE-6626/server-side

429a20b

nbbeeken requested changes Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(NODE-6626): implement integration tests for improved client.close() - server-side #4367

test(NODE-6626): implement integration tests for improved client.close() - server-side #4367

aditi-khare-mongoDB commented Jan 10, 2025 •

edited

Loading

baileympearson left a comment

nbbeeken left a comment

aditi-khare-mongoDB commented Jan 29, 2025 •

edited

Loading

nbbeeken left a comment

nbbeeken Jan 30, 2025

aditi-khare-mongoDB Jan 30, 2025 •

edited

Loading

nbbeeken Jan 31, 2025

test(NODE-6626): implement integration tests for improved client.close() - server-side #4367

Are you sure you want to change the base?

test(NODE-6626): implement integration tests for improved client.close() - server-side #4367

Conversation

aditi-khare-mongoDB commented Jan 10, 2025 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Double check the following

baileympearson left a comment

Choose a reason for hiding this comment

nbbeeken left a comment

Choose a reason for hiding this comment

aditi-khare-mongoDB commented Jan 29, 2025 • edited Loading

nbbeeken left a comment

Choose a reason for hiding this comment

nbbeeken Jan 30, 2025

Choose a reason for hiding this comment

aditi-khare-mongoDB Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

nbbeeken Jan 31, 2025

Choose a reason for hiding this comment

aditi-khare-mongoDB commented Jan 10, 2025 •

edited

Loading

aditi-khare-mongoDB commented Jan 29, 2025 •

edited

Loading

aditi-khare-mongoDB Jan 30, 2025 •

edited

Loading