test(net): expand Node 26.5 parity coverage - #6865
Conversation
📝 WalkthroughWalkthroughThe PR expands the Changesnode:net parity coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-parity/node_suite_baseline.json`:
- Around line 105-106: Update the regression checker to validate the baseline
total in addition to the existing pass-floor check: reject results when
current.total is less than floor.total. Use the baseline totals represented by
test-parity/node_suite_baseline.json so removing failing fixtures cannot make a
reduced suite pass.
In `@test-parity/node-suite/net/connection/data-roundtrip.ts`:
- Around line 4-7: Update the server callback in net.createServer so it
accumulates all data chunks received from the socket instead of responding from
the first data event. After the peer signals the receive stream has ended,
combine the buffered data, uppercase the complete input, and send the
transformed result before closing the socket.
In `@test-parity/node-suite/net/EVIDENCE.md`:
- Around line 162-164: Update the stopping-point sentence in EVIDENCE.md by
replacing “required excluded host” with “required exclusion of host”; leave the
rest of the documentation unchanged.
- Around line 47-48: Update the parity definition in EVIDENCE.md to state that
“same” means byte-for-byte equality of stdout after applying
node_suite_run.normalize(), rather than raw stdout parity. Keep the existing
description of “diff” and the repeated-run qualification unchanged.
In `@test-parity/node-suite/net/server/pause-on-connect.ts`:
- Around line 15-23: Update the synchronization in the server test around the
accepted-socket data handling so it exposes a promise or equivalent completion
signal for the expected payload, then await that signal before logging received.
Replace reliance on client.once("close") alone with the accepted socket’s
expected end/close or data completion, while preserving the existing received
payload assertion/output.
In `@test-parity/node-suite/net/socket/destroy-and-reset.ts`:
- Around line 3-10: The test loop should capture the error emitted by
resetAndDestroy() before logging the final result. Update the socket error
listener in the method loop to record the received error, then log that captured
error’s name and code after invoking the method, ensuring resetAndDestroy()’s
destroy(ERR_SOCKET_CLOSED) event is observable despite the no-op listener.
In `@test-parity/node-suite/net/validation/connect-missing-arguments.ts`:
- Around line 3-10: Synchronize asynchronous validation outcomes before logging
results: in test-parity/node-suite/net/validation/connect-missing-arguments.ts
(lines 3-10) and test-parity/node-suite/net/validation/connect-options.ts (lines
13-20), await either the socket's "error" or "close" event before reporting each
case; in test-parity/node-suite/net/validation/listen-options.ts (lines 9-17),
await "error" or "listening", close the server, then await its cleanup before
reporting. Update the affected net.connect/net.Server.listen fixture flows while
preserving their existing pass/error output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf92aa69-3905-4342-871e-e1bfe752ab6f
📒 Files selected for processing (50)
test-parity/node-suite/net/EVIDENCE.mdtest-parity/node-suite/net/README.mdtest-parity/node-suite/net/classes/block-list-identity.tstest-parity/node-suite/net/classes/block-list-ipv4.tstest-parity/node-suite/net/classes/block-list-ipv6.tstest-parity/node-suite/net/classes/block-list-json.tstest-parity/node-suite/net/classes/block-list-socket-address-inputs.tstest-parity/node-suite/net/classes/blocklist-socketaddress.tstest-parity/node-suite/net/classes/socket-address-defaults.tstest-parity/node-suite/net/classes/socket-address-parse.tstest-parity/node-suite/net/classes/socket-address-validation.tstest-parity/node-suite/net/connection/abort-signal.tstest-parity/node-suite/net/connection/address-metadata.tstest-parity/node-suite/net/connection/backpressure.tstest-parity/node-suite/net/connection/bytes-counters.tstest-parity/node-suite/net/connection/connect-overloads.tstest-parity/node-suite/net/connection/cork-uncork.tstest-parity/node-suite/net/connection/data-roundtrip.tstest-parity/node-suite/net/connection/end-callback-order.tstest-parity/node-suite/net/connection/event-order.tstest-parity/node-suite/net/connection/half-close.tstest-parity/node-suite/net/connection/ipv6-loopback.tstest-parity/node-suite/net/connection/state-transitions.tstest-parity/node-suite/net/exports/bound-socket.tstest-parity/node-suite/net/exports/class-prototypes.tstest-parity/node-suite/net/exports/create-server-and-helpers.tstest-parity/node-suite/net/exports/descriptors-and-aliases.tstest-parity/node-suite/net/exports/stream-alias.tstest-parity/node-suite/net/method-values/server-async-dispose.tstest-parity/node-suite/net/server/address-lifecycle.tstest-parity/node-suite/net/server/connection-state-limits.tstest-parity/node-suite/net/server/get-connections.tstest-parity/node-suite/net/server/initial-state-and-refs.tstest-parity/node-suite/net/server/listen-overloads.tstest-parity/node-suite/net/server/pause-on-connect.tstest-parity/node-suite/net/socket/chainable-controls.tstest-parity/node-suite/net/socket/destroy-and-reset.tstest-parity/node-suite/net/socket/initial-state.tstest-parity/node-suite/net/socket/type-of-service.tstest-parity/node-suite/net/validation/auto-select-family-defaults.tstest-parity/node-suite/net/validation/connect-missing-arguments.tstest-parity/node-suite/net/validation/connect-options.tstest-parity/node-suite/net/validation/connect-port.tstest-parity/node-suite/net/validation/create-server-options.tstest-parity/node-suite/net/validation/listen-options.tstest-parity/node-suite/net/validation/listen-port.tstest-parity/node-suite/net/validation/receiver-validation.tstest-parity/node-suite/net/validation/socket-constructor-options.tstest-parity/node-suite/net/validation/socket-set-timeout.tstest-parity/node_suite_baseline.json
💤 Files with no reviewable changes (2)
- test-parity/node-suite/net/server/connection-state-limits.ts
- test-parity/node-suite/net/classes/blocklist-socketaddress.ts
| "pass": 16, | ||
| "total": 47 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the baseline total, not only the pass floor.
The regression checker currently ignores floor.total. With this 16/47 baseline, deleting the 31 failing fixtures could produce 16/16 and still pass the regression check. Add a current.total < floor.total regression check so fixture removal cannot silently reduce coverage.
Suggested checker change
+ if cur["total"] < floor["total"]:
+ regressions.append(
+ f"{mod}: {cur['total']} fixtures < baseline {floor['total']}"
+ )
- if cur["pass"] < floor["pass"]:
+ if cur["pass"] < floor["pass"]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-parity/node_suite_baseline.json` around lines 105 - 106, Update the
regression checker to validate the baseline total in addition to the existing
pass-floor check: reject results when current.total is less than floor.total.
Use the baseline totals represented by test-parity/node_suite_baseline.json so
removing failing fixtures cannot make a reduced suite pass.
| `same` means byte-for-byte stdout parity with Node 26.5.0. `diff` means a stable | ||
| runtime difference. Every row completed three identical runs. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe parity as normalized stdout equality.
The runner applies node_suite_run.normalize() before comparing outputs, so this is not raw byte-for-byte stdout parity. Clarify that same means byte-for-byte equality after normalization.
Suggested wording
-`same` means byte-for-byte stdout parity with Node 26.5.0.
+`same` means byte-for-byte stdout parity with Node 26.5.0 after normalization.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `same` means byte-for-byte stdout parity with Node 26.5.0. `diff` means a stable | |
| runtime difference. Every row completed three identical runs. | |
| `same` means byte-for-byte stdout parity with Node 26.5.0 after normalization. `diff` means a stable | |
| runtime difference. Every row completed three identical runs. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-parity/node-suite/net/EVIDENCE.md` around lines 47 - 48, Update the
parity definition in EVIDENCE.md to state that “same” means byte-for-byte
equality of stdout after applying node_suite_run.normalize(), rather than raw
stdout parity. Keep the existing description of “diff” and the repeated-run
qualification unchanged.
| await new Promise<void>((resolve, reject) => { | ||
| server.once("error", reject); | ||
| server.listen(0, "127.0.0.1", resolve); | ||
| }); | ||
| client = net.connect((server.address() as any).port, "127.0.0.1"); | ||
| client.on("error", () => {}); | ||
| client.end("hello"); | ||
| await new Promise<void>((resolve) => client!.once("close", resolve)); | ||
| console.log("received:", received.join("")); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test-parity/node-suite/net/server/pause-on-connect.ts"
if [ -f "$file" ]; then
echo "== file exists =="
wc -l "$file"
echo "== contents =="
cat -n "$file"
else
echo "file not found: $file"
echo "candidate files:"
fd -i 'pause-on-connect|pause.*connect|server.*pause' test-parity || true
fi
echo "== related context =="
rg -n "pause-on-connect|received|server\.on|pauseOnConnect|pause connect|connect" test-parity || trueRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== node net test behavior probe =="
node - <<'JS'
const net = require('net');
let events = [];
let done = false;
async function run() {
const server = net.createServer((s) => {
events.push('accept');
s.on('data', (chunk) => {
// simulate work after data
Promise.resolve().then(() => {
events.push('data-after-microtick');
});
});
s.on('end', () => events.push('accept-end'));
s.on('pause', () => events.push('accept-pause'));
s.on('resume', () => events.push('accept-resume'));
});
const serverReady = new Promise(resolve => {
server.once('error', reject => { throw reject; });
server.listen(0, '127.0.0.1', resolve);
});
await serverReady;
const address = server.address();
const client = net.connect(address.port, '127.0.0.1');
client.once('close', () => events.push('client-close'));
client.end('hello');
// mimic test: await client close, print received, then exit
console.log('client-close, received:', 'hello');
await new Promise(resolve => setTimeout(resolve, 1));
console.log('other events:', events);
client.destroy();
server.close();
}
run();
JSRepository: PerryTS/perry
Length of output: 766
Use a stable synchronization point for the server payload.
client.once("close") is the only wait before logging, but the accepted socket’s "data" callback runs asynchronously after being resumed. Track the accepted socket’s expected "end"/"close" or expected-data promise and await it before printing received.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-parity/node-suite/net/server/pause-on-connect.ts` around lines 15 - 23,
Update the synchronization in the server test around the accepted-socket data
handling so it exposes a promise or equivalent completion signal for the
expected payload, then await that signal before logging received. Replace
reliance on client.once("close") alone with the accepted socket’s expected
end/close or data completion, while preserving the existing received payload
assertion/output.
Source: MCP tools
| for (const args of [[], [{}], [{ host: "127.0.0.1" }]] as any[][]) { | ||
| try { | ||
| const socket = (net.connect as any)(...args); | ||
| socket.on("error", () => {}); | ||
| socket.destroy(); | ||
| console.log(JSON.stringify(args), "OK"); | ||
| } catch (error: any) { | ||
| console.log(JSON.stringify(args), error.name, error.code); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked files:"
git ls-files 'test-parity/node-suite/net/validation/*.ts'
echo
echo "Connect missing:"
if [ -f test-parity/node-suite/net/validation/connect-missing-arguments.ts ]; then
cat -n test-parity/node-suite/net/validation/connect-missing-arguments.ts
fi
echo
echo "Connect options:"
if [ -f test-parity/node-suite/net/validation/connect-options.ts ]; then
cat -n test-parity/node-suite/net/validation/connect-options.ts
fi
echo
echo "Listen options:"
if [ -f test-parity/node-suite/net/validation/listen-options.ts ]; then
cat -n test-parity/node-suite/net/validation/listen-options.ts
fi
echo
echo "Search for runner/runtime context:"
rg -n "test-parity|node-suite|validation|net.connect|createServer|listen" test-parity -S --glob '*.ts' --glob '*.js' --glob '*.json' | head -200Repository: PerryTS/perry
Length of output: 27548
🌐 Web query:
Node.js net.connect emits error asynchronous connect options throws synchronously documentation
💡 Result:
In Node.js, the net.connect method (and its alias net.createConnection) functions asynchronously [1][2]. When a connection attempt encounters a problem, it does not throw an exception; instead, it emits an 'error' event on the returned net.Socket object [1][2]. Because the method is asynchronous, you must attach an 'error' event listener to the returned socket to handle connection failures [3][2]. However, the method can still throw errors synchronously if the provided arguments or options are invalid [4]. For example, passing invalid parameters (such as a negative port number) will cause Node.js to throw a synchronous exception (like a RangeError) immediately when the function is called [4]. In summary: 1. Synchronous errors: These occur immediately upon calling the function if the inputs or configuration options are invalid [4]. 2. Asynchronous errors: These occur during the actual network connection attempt and are communicated via the 'error' event on the returned socket [1][2]. To ensure robust error handling, you should use a try-catch block to wrap the net.connect call itself to catch synchronous configuration errors, and register an 'error' listener on the returned socket object to handle runtime connection errors [3].
Citations:
- 1: https://nodejs.org/dist/latest/docs/api/net.html
- 2: https://nodejs.org/api/net.html
- 3: https://stackoverflow.com/questions/14568207/why-do-i-get-a-econnrefused-error-after-trying-to-reconnect-my-tcp-connection-in
- 4: https://repo.huaweicloud.com:8443/artifactory/nodejs-local/v4.4.0/docs/api/errors.html
🌐 Web query:
Node.js net.listen emits error asynchronous listen close close callback promise documentation
💡 Result:
In Node.js, the net.Server.listen method is asynchronous but does not return a Promise [1][2]. Instead, it returns the net.Server instance itself, allowing for method chaining [1][2]. Key behavioral details for handling its lifecycle are as follows: 1. Asynchronous Execution and Callbacks: Because listen is asynchronous, it does not immediately indicate that the server is ready to accept connections [1]. When the server successfully starts listening, it emits the 'listening' event [1][2]. If a callback is passed as the final argument to listen, it is automatically registered as a listener for this 'listening' event [1][2]. 2. Error Handling: Errors occurring during the listen process (such as EADDRINUSE) are emitted via the 'error' event on the server instance [3]. Because the method does not return a promise, these errors cannot be caught using try/catch blocks or promise rejection handlers unless you explicitly wrap the method [4]. Developers must attach an 'error' event listener to the server instance to handle these cases [3]. 3. Closing the Server: The 'close' event is emitted when the server has effectively stopped accepting new connections and all existing connections have ended [3]. The server.close([callback]) method is used to initiate this shutdown [3]. Unlike socket events, if a server emits an 'error' event, it will not automatically emit a 'close' event unless server.close is manually invoked [3]. 4. Promise-based Alternatives: As of recent versions, Node.js has introduced an experimental net/promises API which provides a promise-based approach to listening [5]. This API is designed to resolve once the server is actively listening [5]. For versions prior to or outside of this experimental API, developers often manually wrap server.listen in a Promise if they require async/await syntax, typically by setting up 'listening' and 'error' event listeners within the Promise constructor [4].
Citations:
- 1: https://nodejs.org/api/net.html
- 2: https://nodejs.org/api/net.md
- 3: https://nodejs.org/docs/v22.14.0/api/net.html
- 4: Promisify server.listen nodejs/node#21482
- 5: net: add experimental net/promises API nodejs/node#63965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat package.json 2>/dev/null | jq '.type // empty, .scripts // empty, .dependencies // empty, ."devDependencies" // empty' 2>/dev/null || true
echo
fd -a 'package.json|tsx|test|verify|parity|runner' . | sed 's#^\./##' | head -120
echo
rg -n "\"node-suite\"|node-suite|\\.ts$|validation|net::|test:|parity|tsx|perry" package.json test-parity -g 'package.json' -g '*.ts' -g '*.js' -g '*.json' -S | head -250Repository: PerryTS/perry
Length of output: 7090
Synchronize validation results before reporting pass.
net.connect() and net.Server.listen() fail asynchronously via "error" after returning the socket/server, so these fixtures can log "OK" before any observed failure. Await one of "error"/"close" for sockets, and await "error"/"listening" before closing then waiting on the server’s cleanup before reporting each case.
📍 Affects 3 files
test-parity/node-suite/net/validation/connect-missing-arguments.ts#L3-L10(this comment)test-parity/node-suite/net/validation/connect-options.ts#L13-L20test-parity/node-suite/net/validation/listen-options.ts#L9-L17
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-parity/node-suite/net/validation/connect-missing-arguments.ts` around
lines 3 - 10, Synchronize asynchronous validation outcomes before logging
results: in test-parity/node-suite/net/validation/connect-missing-arguments.ts
(lines 3-10) and test-parity/node-suite/net/validation/connect-options.ts (lines
13-20), await either the socket's "error" or "close" event before reporting each
case; in test-parity/node-suite/net/validation/listen-options.ts (lines 9-17),
await "error" or "listening", close the server, then await its cleanup before
reporting. Update the affected net.connect/net.Server.listen fixture flows while
preserving their existing pass/error output.
Source: MCP tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-parity/node-suite/net/EVIDENCE.md`:
- Line 108: Update the Bun 1.2.18 aggregate row in EVIDENCE.md to match the
verified PR results: 27 of 47 identical and 20 differences, while preserving the
other aggregate columns and table formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 57b0c20d-40df-4776-af0b-2923334cf328
📒 Files selected for processing (4)
test-parity/node-suite/net/EVIDENCE.mdtest-parity/node-suite/net/connection/data-roundtrip.tstest-parity/node-suite/net/server/pause-on-connect.tstest-parity/node-suite/net/socket/destroy-and-reset.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test-parity/node-suite/net/connection/data-roundtrip.ts
- test-parity/node-suite/net/socket/destroy-and-reset.ts
| | -------------- | --------------------: | -----------------: | -------------: | -------: | | ||
| | Node.js 26.5.0 | 47 | 0 | 0 | 0 | | ||
| | Deno 2.9.3 | 42 | 5 | 0 | 0 | | ||
| | Bun 1.2.18 | 28 | 19 | 0 | 0 | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reconcile the Bun aggregate with the PR result.
The PR objective reports Bun at 27/47 identical with 20 differences, while this table reports 28/47 with 19 differences. Align the table with the verified fixture results and stated objective before merging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-parity/node-suite/net/EVIDENCE.md` at line 108, Update the Bun 1.2.18
aggregate row in EVIDENCE.md to match the verified PR results: 27 of 47
identical and 20 differences, while preserving the other aggregate columns and
table formatting.
1 participant
Summary
Expand the granular
node:netparity suite against Node 26.5.0 and document stable differences in Perry, Deno, and Bun.Changes
pauseOnConnectfocused on accepted socket state, and record the asynchronousresetAndDestroy()errorRelated issue
n/a
Test plan
cargo build --releasecleancargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windowspasses — not run; this PR changes parity fixtures and evidence onlydeno fmt --check test-parity/node-suite/netcargo fmt --all -- --checkgit diff --check./scripts/check_file_size.shstill reports the pre-existingcrates/perry-runtime/src/object/mod.rsat 2039 lines. This PR does not change that file.Screenshots / output
Not applicable; this PR has no UI changes.
Checklist
Summary by CodeRabbit