Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the test suite to cover additional edge cases and previously untested branches across transports, buffering, telemetry, and metric helper APIs.
Changes:
- Add new/expanded tests for UDP DNS caching behaviors, transport newline handling, and socket-null error paths.
- Add telemetry, buffering, close(), childClient(), event(), check(), and stats function edge-case coverage.
- Introduce a new
test/constants.jssuite covering protocol constants and platform-specific error-code helpers.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/udpDnsCacheTransport.js | Adds DNS cache edge-case tests (lookup failure, IP host bypass, TTL re-resolution). |
| test/transport.js | Adds transport-focused tests (null socket path, mock transport event API, TCP/stream newline). |
| test/telemetry.js | Adds tests for no-op flush paths, counter flushing, interval start/stop, and breakdown tags. |
| test/statsFunctions.js | Adds metric API edge cases (negative/zero/large values, floats, Date timing, array stats). |
| test/send.js | Adds send/_send/sendMessage edge-case coverage and mock-mode behavior tests. |
| test/event.js | Adds coverage for empty options and special character escaping. |
| test/constants.js | New test file for constants module behavior. |
| test/close.js | Adds close() edge cases (force close after retries, telemetry-enabled close). |
| test/childClient.js | Adds coverage for inheritance/sharing (socket/buffer), nested children, tag override behavior. |
| test/check.js | Adds coverage for all CHECKS statuses being emitted. |
| test/buffer.js | Adds buffer boundary/interval/encoding edge-case coverage and parent/child buffer sharing. |
Comments suppressed due to low confidence (1)
test/send.js:18
afterEachnow callscloseAll(server, statsd, true, done), which swallows any errors thrown during client/server shutdown. That can hide real failures (e.g., socket close errors) and make this suite pass when it shouldn’t. Suggest keepingallowErrorsasfalseand instead explicitly null outserver/statsd(as you already do) for tests that intentionally bypass normal teardown.
afterEach(done => {
closeAll(server, statsd, true, done);
server = null;
statsd = null;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/check.js
Outdated
Comment on lines
+158
to
+173
| server.on('metrics', event => { | ||
| // TCP may concatenate messages, so check all status matches in the event | ||
| const statusMatches = event.match(/_sc\|[^|]+\|(\d)/g); | ||
| if (statusMatches) { | ||
| statusMatches.forEach(match => { | ||
| const status = parseInt(match.charAt(match.length - 1), 10); | ||
| const index = expectedStatuses.indexOf(status); | ||
| if (index >= 0) { | ||
| expectedStatuses.splice(index, 1); | ||
| } | ||
| }); | ||
| } | ||
| if (expectedStatuses.length === 0) { | ||
| done(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This test can call done() more than once: once expectedStatuses is empty, every subsequent metrics event will still satisfy expectedStatuses.length === 0 and re-invoke done(), which fails the test run. Add a guard (e.g., a doneCalled flag) and/or switch to server.once and remove the listener after completion.
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.
No description provided.