Skip to content
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.

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

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

Merged
merged 18 commits into from
Mar 12, 2025

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Jan 10, 2025

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

@aditi-khare-mongoDB aditi-khare-mongoDB changed the base branch from main to NODE-6620/sockets January 10, 2025 22:21
@aditi-khare-mongoDB 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 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 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 aditi-khare-mongoDB marked this pull request as ready for review January 13, 2025 20:01
@baileympearson baileympearson self-assigned this Jan 13, 2025
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 13, 2025
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.

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

baileympearson
baileympearson previously approved these changes Jan 23, 2025
@baileympearson 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
@nbbeeken nbbeeken dismissed baileympearson’s stale review January 23, 2025 17:04

The base branch was changed.

@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch 4 times, most recently from 8adfee6 to 08d8c26 Compare January 23, 2025 18:33
@nbbeeken nbbeeken self-requested a review January 23, 2025 21:49
@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-6626/server-side branch 3 times, most recently from 7348d39 to 74673f8 Compare January 28, 2025 16:03
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

"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
Copy link
Contributor Author

aditi-khare-mongoDB commented Jan 29, 2025

@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

@nbbeeken nbbeeken requested a review from baileympearson March 7, 2025 19:42
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.

one other comment

@nbbeeken nbbeeken requested a review from baileympearson March 10, 2025 18:54
@nbbeeken nbbeeken force-pushed the NODE-6626/server-side branch from b25536d to 4c1d06c Compare March 11, 2025 19:25
@baileympearson baileympearson requested a review from nbbeeken March 12, 2025 20:58
@baileympearson baileympearson merged commit 7dba9b1 into main Mar 12, 2025
30 checks passed
@baileympearson baileympearson deleted the NODE-6626/server-side branch March 12, 2025 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants