test(dgram): expand Node 26.5.0 parity coverage - #6894
Open
TheHypnoo wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR expands Changesnode:dgram parity coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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: 4
🤖 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/dgram/lifecycle/bind-overloads.ts`:
- Around line 3-4: Add explicit test cases in bind-overloads.ts for the
no-argument bind() overload and the bind(port, callback) overload, alongside the
existing "port-address" and "options" modes. Ensure each case creates and binds
a UDP4 socket and preserves the suite’s existing lifecycle assertions and
cleanup.
In `@test-parity/node-suite/dgram/validation/message-views.ts`:
- Around line 37-61: The scatter test currently awaits scatterReceived before
determining whether send succeeded, which can hang when the callback reports an
error. In the scatterResult === "none" branch, await scatterCallback first,
inspect its success outcome, and only await scatterReceived when delivery
succeeded; preserve the existing logging and callback result reporting.
In `@test-parity/node-suite/dgram/validation/send-arguments.ts`:
- Around line 30-33: Update the cleanup logic in the send-argument validation
flow to call socket.bind only when the socket was not already implicitly or
explicitly bound by socket.send/socket.sendto. Track the socket’s bound or
established state during the probe, then conditionally bind before socket.close
in the finally block so ERR_SOCKET_ALREADY_BOUND cannot mask the validation
result.
In `@test-parity/node-suite/dgram/validation/sendto.ts`:
- Around line 36-39: Update the finally block in the sendto validation test to
remove the unconditional socket.bind call and close the already-bound socket
directly with socket.close(resolve), ensuring cleanup does not mask failures
from the sendto checks.
🪄 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: 555455c6-9ccf-4de8-9583-d4629f9889f8
📒 Files selected for processing (56)
test-parity/node-suite/dgram/STATUS.mdtest-parity/node-suite/dgram/api/export-shape.tstest-parity/node-suite/dgram/api/receiver-validation.tstest-parity/node-suite/dgram/api/socket-aliases.tstest-parity/node-suite/dgram/api/socket-class.tstest-parity/node-suite/dgram/api/socket-descriptors.tstest-parity/node-suite/dgram/connection/connect-ordering.tstest-parity/node-suite/dgram/connection/connect-sync.tstest-parity/node-suite/dgram/connection/state-transitions.tstest-parity/node-suite/dgram/control/ttl-validation.tstest-parity/node-suite/dgram/default-import-shape.tstest-parity/node-suite/dgram/ipv6-loopback.tstest-parity/node-suite/dgram/lifecycle/abort-signal.tstest-parity/node-suite/dgram/lifecycle/async-dispose-lifecycle.tstest-parity/node-suite/dgram/lifecycle/async-dispose-shape.tstest-parity/node-suite/dgram/lifecycle/bind-conflict.tstest-parity/node-suite/dgram/lifecycle/bind-error-details.tstest-parity/node-suite/dgram/lifecycle/bind-error-retry.tstest-parity/node-suite/dgram/lifecycle/bind-overloads.tstest-parity/node-suite/dgram/lifecycle/bind-state.tstest-parity/node-suite/dgram/lifecycle/bind-sync.tstest-parity/node-suite/dgram/lifecycle/close-arguments.tstest-parity/node-suite/dgram/lifecycle/close-ordering.tstest-parity/node-suite/dgram/lifecycle/closed-state.tstest-parity/node-suite/dgram/lifecycle/custom-lookup.tstest-parity/node-suite/dgram/lifecycle/default-lookup.tstest-parity/node-suite/dgram/metrics/buffer-sizes.tstest-parity/node-suite/dgram/metrics/create-socket-buffer-options.tstest-parity/node-suite/dgram/metrics/queue-and-ref.tstest-parity/node-suite/dgram/multicast-membership.tstest-parity/node-suite/dgram/send/blocklist.tstest-parity/node-suite/dgram/send/buffer-range.tstest-parity/node-suite/dgram/send/callback-ordering.tstest-parity/node-suite/dgram/send/callback-receiver.tstest-parity/node-suite/dgram/send/connected-overloads.tstest-parity/node-suite/dgram/send/create-socket-listener.tstest-parity/node-suite/dgram/send/default-address-values.tstest-parity/node-suite/dgram/send/default-host.tstest-parity/node-suite/dgram/send/empty-and-multiple.tstest-parity/node-suite/dgram/send/empty-array.tstest-parity/node-suite/dgram/send/error-routing.tstest-parity/node-suite/dgram/send/implicit-bind-state.tstest-parity/node-suite/dgram/send/ipv6-default-host.tstest-parity/node-suite/dgram/send/overloads.tstest-parity/node-suite/dgram/send/scatter-copy.tstest-parity/node-suite/dgram/socket-controls.tstest-parity/node-suite/dgram/unicast-loopback.tstest-parity/node-suite/dgram/validation/address-arguments.tstest-parity/node-suite/dgram/validation/buffer-bounds.tstest-parity/node-suite/dgram/validation/connected-send-arguments.tstest-parity/node-suite/dgram/validation/create-socket-advanced-options.tstest-parity/node-suite/dgram/validation/create-socket-options.tstest-parity/node-suite/dgram/validation/message-views.tstest-parity/node-suite/dgram/validation/send-arguments.tstest-parity/node-suite/dgram/validation/sendto.tstest-parity/node_suite_baseline.json
Comment on lines
+3
to
4
| for (const mode of ["port-address", "options"] as const) { | ||
| const socket = dgram.createSocket("udp4"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether default/port-only bind overloads are covered elsewhere in the dgram suite
rg -n '\.bind\(\)' test-parity/node-suite/dgram -g '*.ts'
rg -n '\.bind\(0\)' test-parity/node-suite/dgram -g '*.ts'
rg -n '\.bind\(0,\s*(undefined)?\)' test-parity/node-suite/dgram -g '*.ts'Repository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
printf 'Files under dgram suite:\n'
git ls-files 'test-parity/node-suite/dgram/*.ts' | sed -n '1,120p'
printf '\nAll bind calls in dgram TypeScript fixtures:\n'
rg -n --color=never -C 3 '\.bind\s*\(' test-parity/node-suite/dgram -g '*.ts' || true
printf '\nRelevant fixture snippets:\n'
for f in test-parity/node-suite/dgram/bind-overloads.ts test-parity/node-suite/dgram/bind-sync.ts test-parity/node-suite/dgram/bind-state.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f"
fi
doneRepository: PerryTS/perry
Length of output: 38448
🌐 Web query:
Node.js dgram Socket bind port-only overload behavior bind(port callback)
💡 Result:
In Node.js, the dgram.Socket.bind method provides an overloaded signature, socket.bind([port][, address][, callback]), where parameters are optional [1][2]. When you use the overload that accepts only a port and a callback, specifically socket.bind(port, callback), the behavior is as follows: 1. Port Binding: The socket attempts to listen for datagram messages on the specified port [1]. If the port is 0, the operating system assigns a random available port [1][2]. 2. Address Assignment: Because the address parameter is omitted in this overload, the operating system defaults to listening on all available network interfaces (typically 0.0.0.0 for IPv4 or:: for IPv6) [1][2]. 3. Callback Execution: The callback function is invoked once the binding process is complete [1]. It receives no parameters [1]. 4. Event Emission: Upon successful binding, the socket also emits a 'listening' event [1][2]. Node.js documentation notes that while you can use both a 'listening' event listener and the callback, doing so is generally considered redundant or not very useful [1][2]. If the binding operation fails—for example, if the port is already in use—an 'error' event is generated on the socket object [1][2]. In rare circumstances, such as attempting to bind a socket that has already been closed, the method may throw an Error [1][2].
Citations:
🏁 Script executed:
#!/bin/bash
set -u
printf 'All non-bind helper bind calls in dgram TypeScript fixtures:\n'
python3 - <<'PY'
import subprocess, pathlib, re
files = subprocess.check_output(["git", "ls-files", "test-parity/node-suite/dgram/*.ts"], text=True).splitlines()
patterns = {
'no_args': re.compile(r'bind\s*\(\s*\)'),
'port_only': re.compile(r'bind\s*\(\s*-?\d+|0\s*\)') if False else re.compile(r'bind\s*\(\s*-?\d+\b[^,\n]*\)'), # broad filter only
}
for f in files:
for i, line in enumerate(pathlib.Path(f).read_text(errors='replace').splitlines(), 1):
if '.bind(' in line or 'bind(' in line and not line.strip().startswith('//') and not line.strip().startswith('#'):
print(f"{f}:{i}:{line.strip()}")
PY
printf '\nRelevant lifecycle bind fixture snippets:\n'
for f in test-parity/node-suite/dgram/lifecycle/bind-sync.ts test-parity/node-suite/dgram/lifecycle/bind-state.ts test-parity/node-suite/dgram/lifecycle/bind-overloads.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,180p' "$f"
fi
doneRepository: PerryTS/perry
Length of output: 11008
Add coverage for the omitted bind overload shapes.
bind-overloads.ts now only exercises bind(port, address, callback) and bind(options, callback). The suite needs explicit fixtures or cases for bind() and bind(port, callback) so bind() overload coverage is not reduced.
🤖 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/dgram/lifecycle/bind-overloads.ts` around lines 3 - 4,
Add explicit test cases in bind-overloads.ts for the no-argument bind() overload
and the bind(port, callback) overload, alongside the existing "port-address" and
"options" modes. Ensure each case creates and binds a UDP4 socket and preserves
the suite’s existing lifecycle assertions and cleanup.
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/dgram/validation/send-arguments.ts`:
- Line 16: Update the Promise around socket.bind in the validation test to
resolve on the one-shot listening event and reject on the one-shot error event,
removing both listeners after either outcome. Ensure bind failures settle the
Promise so the existing finally cleanup can close the socket.
🪄 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: 99d34f0d-52c9-4e2d-97b6-362970b69568
📒 Files selected for processing (3)
test-parity/node-suite/dgram/validation/message-views.tstest-parity/node-suite/dgram/validation/send-arguments.tstest-parity/node-suite/dgram/validation/sendto.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test-parity/node-suite/dgram/validation/message-views.ts
- test-parity/node-suite/dgram/validation/sendto.ts
|
|
||
| await new Promise<void>((resolve) => socket.close(() => resolve())); | ||
| try { | ||
| await new Promise<void>((resolve) => socket.bind(0, "127.0.0.1", resolve)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'send-arguments\.ts$' . || true
file="$(fd 'send-arguments\.ts$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "FILE=$file"
wc -l "$file"
echo "---"
cat -n "$file"
fi
echo "--- tests around bind ---"
rg -n "bind\\(|error\\(|listen|finally|cleanup|close" test-parity/node-suite/dgram/validation/send-arguments.ts test-parity/node-suite/dgram -S || trueRepository: PerryTS/perry
Length of output: 36526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'send-arguments\.ts$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "FILE=$file"
wc -l "$file"
echo "---"
cat -n "$file"
fi
echo "--- tests around bind ---"
rg -n "bind\\(|error\\(|listen|finally|cleanup|close" test-parity/node-suite/dgram/validation/send-arguments.ts test-parity/node-suite/dgram -S || trueRepository: PerryTS/perry
Length of output: 36362
Reject the bind Promise on socket errors.
If socket.bind() fails, Node emits an 'error' event rather than invoking the listening callback. The current Promise therefore never settles, and the finally cleanup may not close the socket. Use one-shot listening/error listeners and remove them after either outcome.
🤖 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/dgram/validation/send-arguments.ts` at line 16, Update
the Promise around socket.bind in the validation test to resolve on the one-shot
listening event and reject on the one-shot error event, removing both listeners
after either outcome. Ensure bind failures settle the Promise so the existing
finally cleanup can close the socket.
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
1 participant
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.
Summary
Expands granular
node:dgramparity coverage from 38 to 54 deterministic fixtures using Node 26.5.0 as the oracle and pinned Deno/Bun sources as comparison evidence.Changes
finallyacross the suite and remove persistent listeners after each contract settles.Related issue
n/a
Test plan
cargo build --release -p perrydeno fmt --checkon all 54 fixturesgit diff --checkcargo build --releasefor the whole workspace was not run; the affectedperrypackage build passescargo fmt --all -- --checkstops on unchangedmainformatting incollect_modules.rs./scripts/check_file_size.shstops on unchanged oversizedobject/mod.rsandreadline.rsScreenshots / output
Checklist
Summary by CodeRabbit