fix(tests): close sockets to remove a Jest warning about resources outliving their tests#2279
fix(tests): close sockets to remove a Jest warning about resources outliving their tests#2279danwkennedy wants to merge 1 commit intomainfrom
Conversation
…tliving their tests
There was a problem hiding this comment.
Pull request overview
Updates a couple of test cases to ensure HTTP response sockets are closed, addressing a Jest warning about resources outliving their tests.
Changes:
- Close the response stream in an
http-clientHEAD request test to prevent lingering sockets. - Drain/close the response stream in a
coreOIDC HTTP GET test to avoid open-handle warnings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/http-client/tests/basics.test.ts | Ensures the HEAD response stream is closed so the socket doesn’t outlive the test. |
| packages/core/tests/core.test.ts | Drains/closes the OIDC endpoint response to prevent Jest open-handle warnings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| expect(res.message.statusCode).toBe(200) | ||
| // Consume the response to close the socket | ||
| res.message.destroy() |
There was a problem hiding this comment.
The comment says the response is being consumed, but res.message.destroy() actually aborts the stream. For a HEAD request you can drain/consume cleanly by awaiting res.readBody() (it should resolve to an empty string) and avoid calling destroy(), which keeps the intent clear and matches the pattern used in other tests in this file.
| res.message.destroy() | |
| await res.readBody() |
| expect(res.message.statusCode).toBe(200) | ||
| // Consume the response to close the socket | ||
| await res.readBody() | ||
| res.message.destroy() |
There was a problem hiding this comment.
await res.readBody() should already fully drain the response and allow the underlying socket to close; calling res.message.destroy() afterwards is redundant and makes the intent a bit unclear. Consider dropping the destroy() (or updating the comment if you intentionally want to abort instead of just consuming).
| res.message.destroy() |
1 participant
Description
I noticed this while I was upgrading the packages. This should silence a warning from Jest.