diagnostics_channel: vendor the two http channel tests (+2) - #34641
Conversation
Both pass on this branch as-is; copied verbatim from upstream. Between them they cover http.server.request.start, http.server.response.created, http.server.response.finish, http.client.request.created, http.client.request.start, http.client.request.error and http.client.response.finish. Verified 3/3 and tamper-checked two ways each -- a mutated value assertion and a mutated mustCall count -- with the same tampers run against the node v26.3.0 binary as a control. All four exit non-zero on both runtimes, so neither test is vacuous.
|
Updated 2:51 PM PT - Jul 23rd, 2026
@cirospaciari, your commit 9246cb9 is building: |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — two upstream Node v26.3.0 test files vendored verbatim into test/js/node/test/parallel/, no source changes. The one CI failure (test-worker-message-port-transfer-terminate.js SIGABRT on x64-asan) is unrelated to this diff.
Extended reasoning...
Overview
This PR adds exactly two files to test/js/node/test/parallel/: test-diagnostic-channel-http-response-created.js and test-diagnostics-channel-http.js, both copied verbatim from Node.js v26.3.0's own test/parallel/. Together they exercise the seven http.client.* / http.server.* diagnostics_channel publish points. There are zero source changes in the diff — the underlying channel-publishing work landed in the base branch (#32628); this PR just wires in the upstream conformance tests.
Security risks
None. Test-only additions with no runtime, build, or config changes. The tests bind to ephemeral ports on localhost and make one intentionally-failing lookup to an RFC 2606 .invalid host.
Level of scrutiny
Low. Per this directory's own CLAUDE.md, these files are official Node.js tests that Bun does not author or modify — the review question reduces to "are they faithfully copied and do they run?" The PR description documents 3/3 green plus tamper checks against both runtimes to prove the assertions are load-bearing, and the bug-hunting pass independently confirmed one file is byte-for-byte identical to upstream. The only finding is an informational nit about addresses.INVALID_HOST triggering a real getaddrinfo (which Node itself does in parallel/, not internet/), with a documented no-edit mitigation (NODE_TEST_INVALID_HOST) if it ever flakes — not a merge blocker.
Other factors
The lone CI failure in Build #75433 is test-worker-message-port-transfer-terminate.js on x64-asan, which this PR does not touch and which cannot be affected by adding two unrelated test files. No outstanding human review comments; no prior automated review to avoid duplicating.
There was a problem hiding this comment.
LGTM — two verbatim upstream Node v26.3.0 tests plus two well-documented ASAN-only quarantine entries; no source changes. The inline DNS-lookup note stands as a heads-up only, not a blocker.
Extended reasoning...
Overview
Three files touched, none of them source code:
- Two new files in
test/js/node/test/parallel/— verbatim copies of Node v26.3.0'stest-diagnostics-channel-http.jsandtest-diagnostic-channel-http-response-created.js. This is the standard upstream-test-vendoring pattern for that directory (which per its own CLAUDE.md forbids local modification). test/expectations.txt— two[ ASAN ]-scoped[ CRASH ]entries fortest-worker-message-port-transfer-terminate.jsandworker-transfer-terminate-stress.test.ts, each with a multi-line comment naming the JSC assertion, the tracking issue (#34095 / #34690), the fix PRs (#33966 / #33418), example build numbers, and explicit removal criteria. The commit message says these match main.
Security risks
None. Test-only additions and CI expectations metadata; nothing touches auth, crypto, network handling in production code, or any runtime path.
Level of scrutiny
Low. Vendored upstream tests are byte-for-byte imports whose correctness is Node's responsibility — Bun's job is to pass them, and the PR description documents tamper-checking against both runtimes to confirm they're not vacuous. The expectations.txt additions follow the file's own header guidance (crash-the-runner only, ASAN-scoped, with a comment naming the observed failure) and exceed REVIEW.md's bar for skip justification by a wide margin. Both quarantined files exist in the tree.
Other factors
A prior review pass already left one inline nit on test-diagnostics-channel-http.js:80 about the real getaddrinfo() call to something.invalid — it's the first file in this directory to hit the wire, but upstream ships it in parallel/ (not internet/), .invalid is RFC 2606-reserved, and the mitigation (NODE_TEST_INVALID_HOST env var) requires no file edit. Not a reason to hold the PR. The PR is stacked on #32628, so merge order is already gated externally.
Fast-exit-intermediate race tracked in PR #33965; hits darwin lanes across unrelated branches (~1 in 2 builds today), including both darwin-14 lanes of one build on this branch.
expectations.txt entries stay owned by main; the macOS fast-exit race (#33965) is absorbed by CI retries instead. [allow size]
There was a problem hiding this comment.
LGTM — both earlier nits are resolved (the darwin no-orphans quarantine was dropped in ddfb3d3, and the .invalid DNS lookup is kept verbatim per the vendored-test rule).
What was reviewed:
- The two new test files are verbatim Node v26.3.0 ports;
common/internetis already vendored, andaddresses.INVALID_HOSThonorsNODE_TEST_INVALID_HOSTif it ever flakes. - The two new
[ ASAN ]expectations entries quarantine whole-process JSC assertion crashes (not single-case failures), are ASAN-gated only, and each names a tracked issue + fix PR + build numbers — matches the file header's stated policy. - Confirmed both quarantined test files already exist on
main, so this is quarantining pre-existing crashes, not hiding something introduced by the stack.
Extended reasoning...
Overview
Three files touched, all under test/:
- Two new vendored Node v26.3.0 tests in
test/js/node/test/parallel/covering the sevenhttp.*diagnostics_channel events. Both are copied verbatim per the directory's vendored-test convention. test/expectations.txt: two new[ ASAN ]entries quarantiningtest-worker-message-port-transfer-terminate.js(#34095, fix PRs #33966/#33418) andworker-transfer-terminate-stress.test.ts(#34690). The earlier[ DARWIN ] no-orphans.test.tsentry I flagged was removed in ddfb3d3.
No source files changed. The PR is stacked on #32628, but the diff under review here is purely tests + expectations.
Security risks
None. Test-only additions with no production code paths. The one network-touching line (http.get({ host: 'something.invalid' })) is a DNS lookup for an RFC 2606-reserved TLD, identical to upstream Node's parallel/ copy — flagged and acknowledged in the prior review round.
Level of scrutiny
Low. Vendored upstream tests are mechanical additions, and the directory convention explicitly forbids modifying them. The only judgment call is the two ASAN quarantines, which I checked against the expectations.txt header policy: both describe whole-process JSC assertion crashes (ASSERTION FAILED: !scope.exception() || !result and ExceptionScope::assertNoException), which is exactly the "crashes the runner" case the header reserves the file for. Both are ASAN-only (release lanes keep coverage), cite specific build numbers, and name the removal condition. I verified both quarantined files exist on origin/main, so these are pre-existing crashes rather than regressions from this stack.
Other factors
- Both inline nits from the 2026-07-20 review round are marked resolved with author responses; the over-broad no-orphans quarantine was removed as suggested.
- The PR description documents tamper-checking both new tests (mutated assertion + mutated
mustCallcount) against both Bun and the Node v26.3.0 oracle, satisfying the "prove the test fails for the right reason" requirement. - The expectations comments explicitly distinguish the two quarantines' removal conditions (the stress test entry is "NOT removable with the one above" because #33966 reports it still reproduces post-fix), which is the kind of detail that prevents stale entries.
[allow size]
There was a problem hiding this comment.
LGTM — test-only, both vendored files are verbatim Node v26.3.0, and expectations.txt now byte-matches origin/main so the merge is a no-op there.
What was reviewed:
- Confirmed
git diff origin/main -- test/expectations.txtis empty after ba33a8b — the removed quarantine entries are already gone on main, so nothing is being un-quarantined. - Both prior nits (
.invalidDNS lookup, no-orphans whole-file quarantine) resolved by the author; the latter was dropped in ddfb3d3. - Trailing-newline concern on expectations.txt ruled out — main's copy is identical.
Extended reasoning...
Overview
Two new files under test/js/node/test/parallel/ vendored verbatim from Node v26.3.0 (test-diagnostics-channel-http.js, test-diagnostic-channel-http-response-created.js), plus a sync of test/expectations.txt with main. No source files are touched.
Security risks
None. Test-only additions exercising diagnostics_channel over a localhost http.createServer; the one non-local operation is a DNS lookup for the RFC 2606-reserved something.invalid, which was already discussed and accepted as verbatim-upstream behavior.
Level of scrutiny
Low. Vendored upstream tests are copied byte-for-byte per the test/js/node/test/parallel/ policy and aren't authored code to review for correctness — the review question is only whether they belong here and whether anything else was disturbed. The expectations.txt hunk looked large in the three-dot diff, but a direct check against origin/main shows zero delta, so it's rebase noise from the stacked base (#32628), not a substantive change. REVIEW.md's "never silently weaken an existing test" concern doesn't apply: every removed quarantine line is already absent on main.
Other factors
I previously left two nits on this PR; both threads are resolved. The author dropped the branch-local no-orphans darwin quarantine in ddfb3d3 and then synced expectations.txt to main in ba33a8b, addressing the over-quarantine concern. The remaining .invalid DNS note is informational and was acknowledged as intentional per the vendored-verbatim rule. The bug-hunting pass this run found nothing; the one candidate (missing trailing newline on expectations.txt) was ruled out because main's copy is identical.
c5313fb
into
claude/diagnostics-channel-node26
There was a problem hiding this comment.
LGTM — two upstream Node v26.3.0 diagnostics_channel http tests vendored verbatim, no source changes.
What was reviewed:
- Confirmed both files land in
test/js/node/test/parallel/unmodified per that directory's vendored-verbatim rule, and neither is listed inexpectations.txt(so they'll actually run). - The earlier
expectations.txtno-orphans entry was dropped in ddfb3d3; the diff is now purely the two test files. - The
.invalidDNS-lookup concern was acknowledged and resolved as intentional (matches upstreamparallel/placement;NODE_TEST_INVALID_HOSTis the escape hatch if it ever flakes).
Extended reasoning...
Overview
Adds two new files under test/js/node/test/parallel/: test-diagnostic-channel-http-response-created.js and test-diagnostics-channel-http.js, both copied verbatim from Node.js v26.3.0. Together they exercise the seven http.client.* / http.server.* diagnostics_channel events. Zero source files changed. This PR is stacked on #32628 (which implements the channel publishes); this PR only adds the upstream coverage.
Security risks
None. Test-only additions in the vendored Node parallel suite; no runtime, build, or dependency changes.
Level of scrutiny
Low. This is the canonical pattern for expanding Node compat coverage: drop the upstream file in unmodified, confirm it passes, confirm it isn't quarantined. test/js/node/test/parallel/CLAUDE.md explicitly says these files are not to be modified, so byte-for-byte fidelity to upstream is the correctness bar — which the PR description documents (tamper-checked against the Node v26.3.0 binary as a control).
Other factors
Both prior review threads are resolved: the author dropped the unrelated expectations.txt whole-file quarantine entry (ddfb3d3), and acknowledged the addresses.INVALID_HOST real-DNS-lookup nit as an intentional verbatim carry-over from upstream (where the same file lives in parallel/, not internet/). Neither file appears in expectations.txt, so they'll run in CI. The PR description records 3/3 green runs plus negative tamper checks on both runtimes, satisfying the "fails for the right reason" bar for vendored tests.
2 participants
Vendors the two upstream
diagnostics_channelhttp tests, copied verbatim. Both pass on this branch as-is. Between them they coverhttp.server.request.start,http.server.response.created,http.server.response.finish,http.client.request.created,http.client.request.start,http.client.request.errorandhttp.client.response.finish.Stacked on #32628. No source changes.
Verification
3/3 green each, and tamper-checked two ways — a mutated value assertion and a mutated
mustCallcount — with the same tampers run against the node v26.3.0 binary as a control. All four exit non-zero on both runtimes, so neither test is vacuous.No regressions (nothing to regress — zero source files changed). Confirmed anyway: the full diagnostics_channel suite is 67/67, and a 40-file
node:httpsample is 40/40.The gap after this: essentially closed
A real per-file check found 17 upstream
test-diagnostic*-channel*files missing, not the ~67 agit ls-treediff suggested. Of those 17:node:quicabsent in Bun and not compiled into the node oracle — no ceiling to reachprocess.permission; the whole permission model is absentconsole.*channels never published, plus autil.inspectline-breaking differencetracing:node.test:*channels; also needs 2 fixtures and ALSbindStoreinto test bodiesnavigator.locksmodule.importSo 67 of the 73 non-quic upstream tests are now in the tree and green, and each of the 6 remaining is blocked by a missing subsystem rather than a missing channel publish. I did not convert anything by publishing a channel at a convenient moment.
One finding worth passing on
test-diagnostic-channel-module-importmatched node on every field — event order, same-object identity across all four events, theresultpayload — excepturl, where Bun reports'node:http'and node reports'http'.The cause is not in diagnostics_channel: Bun's transpiler rewrites the literal
import("http")intoimport("node:http")at parse time. With a specifier opaque to the transpiler (['h','t','t','p'].join('')) Bun reports"http"and matches node exactly; relative and already-prefixed specifiers match too. So it's a real fidelity bug, but the fix belongs in bare-builtin specifier normalization in the parser — global module-resolution semantics, not something to patch from here.