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-6631): Implement integration tests for improved client.close() - sockets + timers #4361

Draft
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Dec 30, 2024

Description

Add socket integration tests for client.close().

What is changing?

  • Assert socket creation and clean-up through libuv tracking for each relevant driver instance of socket creation.
  • Assert timer creation by asserting timers are defined on relevant client components and assert timer clean-up through process.getActiveResourceInfo()
    • Timer existence is more finicky to test, and require some artificial delaying of promises and high timer MSs
    • ex: a high heartbeatFrequencyMS to ensure we're not in between intervals when we assert MonitorInterval's timer exits

To see more information regarding testing plan outline see the design document, specifically the dropdowns under Sockets and Timers sections, respectively.

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

Copy link

There is an existing patch(es) for this commit SHA:

Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'.

@aditi-khare-mongoDB aditi-khare-mongoDB changed the base branch from main to NODE-6620/implement-client-close-test December 30, 2024 20:39
@aditi-khare-mongoDB aditi-khare-mongoDB changed the base branch from NODE-6620/implement-client-close-test to NODE-6615/integration-client-close December 30, 2024 20:39
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title test(NODE-6620): Add Improved Client.close() Socket Resource Integration Tests test(NODE-6620): Implement integration tests for improved client.close() - Socket Dec 30, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title test(NODE-6620): Implement integration tests for improved client.close() - Socket test(NODE-6620): Implement integration tests for improved client.close() - Sockets Dec 30, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title test(NODE-6620): Implement integration tests for improved client.close() - Sockets test(NODE-6631): Implement integration tests for improved client.close() - Sockets + Timers Jan 2, 2025
@aditi-khare-mongoDB aditi-khare-mongoDB changed the base branch from NODE-6620/client-close-test-setup to NODE-6615/integration-client-close January 2, 2025 21:51
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title test(NODE-6631): Implement integration tests for improved client.close() - Sockets + Timers test(NODE-6631): Implement integration tests for improved client.close() - sockets + timers Jan 7, 2025
!originalReportAddresses.includes(resource.address) &&
resource.is_referenced && // if a resource is unreferenced, it's not keeping the event loop open
(!serverType.includes(resource.type) || resource.is_active)
!originalReportAddresses.includes(resource.address) && resource.is_referenced // if a resource is unreferenced, it's not keeping the event loop open
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up from NODE-6620: We do want to clean up inactive sockets


const servers = client.topology?.s.servers;

// note: minPoolSizeCheckFrequencyMS = 100 ms by client, so this test has a chance of being flaky
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the note on the comment. This test has a higher chance of being flaky since the client can't set minPoolSizeCheckFrequencyMS. I can this to set the variable to a higher value using sinon if preferred by reviewers.

Base automatically changed from NODE-6615/integration-client-close to main January 8, 2025 16:18
rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant