Skip to content

Conversation

@durran
Copy link
Member

@durran durran commented Oct 23, 2025

Description

Fixes the flaky SDAM prose test for connection pooling.

Summary of Changes

Updates the test to filter out rogue server heartbeat failed events.

What is the motivation for this change?

NODE-5206

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

@durran durran requested a review from a team as a code owner October 23, 2025 11:28
const events: string[] = [];
beforeEach(async function () {
client = this.configuration.newClient({
directConnection: true,
appName: 'SDAMPoolManagementTest',
heartbeatFrequencyMS: 500
heartbeatFrequencyMS: 100
Copy link
Member Author

Choose a reason for hiding this comment

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

Spec says 500 or less, so I changed it to 100 to speed up the test.

});
utilClient = this.configuration.newClient({ directConnection: true });
Copy link
Member Author

Choose a reason for hiding this comment

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

Our general pattern in tests is to have a util client for setting fail points, so I changed this in this test.

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

Do we know why this test started failing much more frequently in the last few months and why it seems to most frequently (maybe only?) fail on latest servers with server API enabled?

@durran
Copy link
Member Author

durran commented Oct 24, 2025

Do we know why this test started failing much more frequently in the last few months and why it seems to most frequently (maybe only?) fail on latest servers with server API enabled?

I have no idea, but locally I could only get it to flake with the stable API enabled. Not limited to latest server as I'm on 8.0 locally, but I could not get the test to fail without the stable API enabled. And it was only one excessive failing hearbeat in the way no matter how much I tinkered with the heartbeat frequency.

@durran durran requested a review from baileympearson October 24, 2025 09:11
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