test: add unexpected disconnect guards to more client test files#4844
Open
samayer12 wants to merge 10 commits intonodejs:mainfrom
Open
test: add unexpected disconnect guards to more client test files#4844samayer12 wants to merge 10 commits intonodejs:mainfrom
samayer12 wants to merge 10 commits intonodejs:mainfrom
Conversation
Signed-off-by: Sam Mayer <sam.mayer@datadoghq.com> Signed-off-by: Sam Mayer <sam.mayer@defenseunicorns.com>
Signed-off-by: Sam Mayer <sam.mayer@defenseunicorns.com>
…est files Signed-off-by: Sam Mayer <sam.mayer@defenseunicorns.com>
samayer12
commented
Feb 27, 2026
The first describe block used the global agent, leaving pooled connections alive after server.closeAllConnections() sent RST. On macOS, the async RST delivery could surface as ECONNRESET in the next describe block's fetch call. Give each block its own Client and await cleanup to eliminate the race. Signed-off-by: Sam Mayer <sam.mayer@defenseunicorns.com>
5c94a36 to
8e69a9f
Compare
metcoder95
reviewed
Mar 2, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4844 +/- ##
=======================================
Coverage 93.14% 93.14%
=======================================
Files 109 109
Lines 34239 34239
=======================================
Hits 31891 31891
Misses 2348 2348 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a88b414 to
8e69a9f
Compare
This reverts commit 29ddaeb.
This reverts commit 78d583d.
…ervers The chain limit tests ran concurrently (node:test uses Promise.all within a Suite), so a shared server was unsafe: an aborted connection's async RST delivery on macOS could corrupt the server state for a sibling test. Replace the shared before/after server with a setupChainServer(t) helper that creates an isolated server+client pair per test and binds cleanup to t.after, which is scoped to each individual test context. Also removes the keepalive: false no-op (undici documents this flag as a noop outside of browser context).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This relates to...
Relates to #251 and #4833.
Rationale
Adds a
guardDisconnect()helper to unit tests that previously used the more verbose pattern seen in #4833. This PR also adds the guard function to additional tests that do not already adopt the pattern. Docs are also updated for future test authors.I think that 8e69a9f also resolves a test cleanup issue in CI with node 20 tests on macos-latest, seen here.Note that
c8fails coverage in CI and is related to bcoe/c8#581 and bcoe/c8#582.Changes
This PR recommended structuring tests along these lines:
Following feedback, we prefer to make the function utility explicit and just provide a set of assertions calculated. Therefore, we'll stick with:
Here are the benchmark results from a local run:
Features
N/A
Bug Fixes
N/A
Breaking Changes and Deprecations
None
Status