Repository navigation
Conversation
|
Updated 12:17 PM PT - Jul 13th, 2026
❌ @robobun, your commit 2dc9a18 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33324That installs a local version of the PR into your bun-33324 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughThis PR adds test coverage for AbortController/AbortSignal DOM behavior and structuredClone semantics. It expands abort-event, static AbortSignal, and Error/DataCloneError cloning cases without changing exported APIs. ChangesAbortController/AbortSignal test coverage
structuredClone test coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 2
🤖 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/js/web/abort/abort.test.ts`:
- Around line 221-226: The abort identity test is too weak because
`toThrow(reason)` only matches the error message, not the exact thrown object.
Update the `throws the reason by identity` test around `AbortController.abort`
and `signal.throwIfAborted()` to capture the thrown value explicitly and assert
it is the same `reason` instance using identity comparison (`toBe`), so the test
verifies the exact object was thrown.
In `@test/js/web/workers/structured-clone.test.ts`:
- Around line 744-747: The typed-array transfer test in structuredClone is too
broad because it only checks for a DOMException and could pass for the wrong
error name. Update the assertion in the test that uses structuredClone(view, {
transfer: [view as any] }) so it also verifies the thrown error’s name is
"DataCloneError", matching the other cases in this describe block.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 056b91ee-af69-462f-be9f-371d4c8e1252
📒 Files selected for processing (2)
test/js/web/abort/abort.test.tstest/js/web/workers/structured-clone.test.ts
|
This PR may be a duplicate of:
🤖 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 — pure additive test coverage for AbortSignal/AbortController and structuredClone, no src/ changes.
Extended reasoning...
Overview
This PR appends ~60 new tests to two existing test files: test/js/web/abort/abort.test.ts (+32 tests covering AbortController/AbortSignal DOM spec behavior) and test/js/web/workers/structured-clone.test.ts (+28 tests covering DataCloneError rejection cases and HTML serialization semantics). All changes are purely additive — no existing tests are modified or removed, and there are zero src/ changes.
Security risks
None. This is test-only code that never ships in the runtime. No auth, crypto, permissions, or user-input handling is touched.
Level of scrutiny
Low. Test-only additions to existing files, following established patterns in those files. The tests are spec-referenced (DOM and HTML spec URLs cited), use strong assertions (.toBe identity checks, specific error names/types rather than bare .toThrow()), and avoid flaky constructs — the AbortSignal.timeout test awaits the abort event via Promise.withResolvers rather than sleeping. The one known divergence (Error cause not serialized) is correctly marked test.failing so it flips when fixed.
Other factors
The bug-hunting system found nothing. The PR description shows both files pass locally (37 pass / 240 pass). Worst-case failure mode is a test being wrong or overly strict, which CI will surface immediately and which carries no runtime risk. This is exactly the kind of mechanical, self-contained coverage expansion that doesn't need human review.
|
Thanks, all three bots. Addressed in 4346ac7. CodeRabbit: both correct.
find-issues: this PR does not fix any of the four, so no Node gives #18357 ( duplicate-prs: #25406 and #25407 overlap on |
There was a problem hiding this comment.
No issues found — the tests look solid and spec-derived. Deferring to a maintainer mainly for the open call on the AbortSignal.any overlap with #25406/#25407, and to sanity-check the five test.failing expectations before they land as pinned behavior.
Extended reasoning...
Overview
Test-only PR adding ~60 spec-derived cases to two existing files: test/js/web/abort/abort.test.ts (AbortController/AbortSignal: reason identity, event dispatch ordering, throwIfAborted, abort/timeout/any statics, brand checks) and test/js/web/workers/structured-clone.test.ts (DataCloneError rejection matrix, object-graph semantics, Error cloning). No src/ changes. Both CodeRabbit nits were addressed in 4346ac7 and the bug-hunting pass found nothing.
Security risks
None. Pure test additions with no runtime, network, or filesystem side effects beyond what the existing test files already exercise.
Level of scrutiny
Medium. While test-only, this is 60 new assertions that encode spec expectations Bun will be held to going forward, plus five test.failing cases that pin the shape of a future DOMException-stack fix (e.g. stack must contain "AbortError"). A maintainer should confirm those pinned expectations match what they actually intend to ship, since test.failing will flip red the moment the behavior changes in any direction.
Other factors
The author explicitly left a decision to a maintainer: the AbortSignal.any describe block overlaps open PRs #25406/#25407 and they offered to drop it. That call — land this as-is vs. drop the overlap and let the earlier PRs land first — is not mine to make. The tests themselves follow harness conventions (await conditions via Promise.withResolvers, no sleeps, identity assertions via toBe, test.each-style tables) and read cleanly.
4346ac7 to
f76e289
Compare
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/js/web/workers/structured-clone.test.ts`:
- Around line 731-754: Extract a shared expectDataCloneError helper for the
duplicated try/catch assertion logic in structuredClone tests. Move the repeated
“capture thrown value, assert DOMException, assert name is DataCloneError” block
into a small helper near the rejected cases, then call it from both the rejected
loop and the typed-array transfer test to keep the structured-clone test cases
concise and consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b22c4fb-a347-4b58-b3c3-64380d8b4c1d
📒 Files selected for processing (2)
test/js/web/abort/abort.test.tstest/js/web/workers/structured-clone.test.ts
There was a problem hiding this comment.
I didn't find any issues with the tests themselves — they're well-derived from spec and Node behavior — but the five test.failing cases pin expected behavior for open bugs, and the AbortSignal.any overlap with #25406/#25407 needs a maintainer call, so worth a human look before landing.
Extended reasoning...
Overview
This PR is purely additive test coverage: ~480 lines across test/js/web/abort/abort.test.ts (+36 tests) and test/js/web/workers/structured-clone.test.ts (+28 tests). No src/ changes. Tests are derived from the DOM and HTML specs and cross-checked against Node, covering throwIfAborted, abort-reason identity, event dispatch/reentrancy, AbortSignal.abort/timeout/any, and structuredClone rejection cases, object semantics, cycles, and Error cloning. The author addressed both of CodeRabbit's correctness comments (identity assertion in throwIfAborted, tightened DataCloneError name check) in commit 4346ac7; one trivial refactor nit (extract an expectDataCloneError helper) remains unaddressed but is cosmetic.
Security risks
None. Test-only changes with no production code, no network access beyond in-process APIs, no auth/crypto/permissions surface.
Level of scrutiny
Medium. The pass/fail tests themselves are low-risk — additive spec coverage that either passes or gets reverted. What warrants a maintainer's eye:
- Five
test.failingcases pin desired behavior for #17877/#25182/#21900 (DOMException stack traces) and Errorcauseserialization. These are reasonable expectations (Node and browsers agree), but they encode a commitment about what the fix should look like — e.g.,reason.stackmust contain"AbortError",new DOMException(...).stackmust be a string. A maintainer should confirm these are the invariants Bun wants pinned. - The
waitUntilAbortedpolling helper works around a Windows hang by pollingsignal.abortedin a bounded 2000×1ms loop rather than awaiting the event. It's documented and bounded, but it's a workaround for a runner bug rather than a fix.
Other factors
- The duplicate-PR bot flagged overlap with open PRs #25406/#25407 for the
AbortSignal.anyblock, and the author explicitly offered to drop that section if maintainers prefer landing those first. That's a coordination decision only a maintainer can make. - CI build #68379 was retriggered on the latest commit; I can't confirm it's green from here.
- The tests are well-structured, use identity assertions where appropriate, cite spec sections and issue URLs, and follow the file's existing conventions. I found no flakiness patterns, unbounded waits, or vacuous assertions.
|
Good review. Three things in fb6cf9c. "a workaround for a runner bug rather than a fix" is the sharpest point here, and it deserved better than a code comment. // test/js/web/abort/abort.ts:113
// FIXME: test runner hangs when this is enabled
test.skip("timeout works", done => { ... });Filed as #33334 with the evidence chain: the CodeRabbit's On the five $ bun -e 'console.log(new DOMException("x", "AbortError").stack)'
undefinedEverything downstream falls out of that: Worth noting the directionality: On the |
There was a problem hiding this comment.
I didn't find any issues — the tests are well-structured and spec-derived — but with ~470 lines of new CI assertions (including five test.failing cases that encode expected future behavior) and the open overlap with #25406/#25407 you flagged, a maintainer should sign off on which expectations to commit to.
Extended reasoning...
Overview
This PR adds ~64 new test cases across two files with no src/ changes:
test/js/web/abort/abort.test.ts: +36 tests covering AbortController/AbortSignal DOM spec behavior (throwIfAborted, reason identity, event dispatch/reentrancy, listener ordering,AbortSignal.abort/timeout/any), plus 4test.failingcases pinning the missing-stack-on-DOMException bugs (#17877, #25182, #21900).test/js/web/workers/structured-clone.test.ts: +28 tests coveringDataCloneErrorfor non-serializable values, object cloning semantics (getters, holes, prototypes, cycles, identity), and Error cloning, plus 1test.failingforError.causenot being serialized.
All three rounds of CodeRabbit feedback have been addressed (identity assertion for throwIfAborted, DataCloneError name check, extracted expectDataCloneError helper). The bug-hunting system found nothing.
Security risks
None. Test-only additions with no changes to runtime, build, or CI configuration.
Level of scrutiny
Moderate. While test-only, this is a large surface (~470 lines) of new CI assertions derived from the DOM/HTML specs. Each passing test becomes a contract Bun must continue to satisfy, and each test.failing becomes a contract that fixing the referenced issue is required (the test will start failing CI once the underlying behavior changes). The assertions look correct against the specs and Node's behavior, but a maintainer should confirm they agree with all of them — particularly the test.failing cases, which encode opinions about what Bun should do (e.g., that DOMException must capture a stack, that structuredClone must preserve Error.cause).
Other factors
- The author explicitly flagged overlap between the
AbortSignal.anydescribe block and open PRs #25406/#25407, and deferred to maintainers on whether to drop it here or land those first. That coordination decision is still open. - A bounded-poll
waitUntilAbortedhelper works around a Windows test-runner hang (filed as #33334) rather than awaiting the abort event. The workaround is safe (bounded, fails loudly), but a maintainer may want to weigh in on whether to land it or wait for the underlying fix. - The tests follow repo conventions well (no hardcoded ports, no sleeps-as-assertions except the bounded workaround, proper
test.failingusage with issue links).
Given the scale, the embedded behavioral commitments via test.failing, and the open PR-overlap question, this warrants a human look rather than auto-approval.
AbortSignal.prototype.throwIfAborted() was never called anywhere in the test suite, AbortController.abort(reason) had no test that actually passed a reason, and structuredClone's DataCloneError path only covered detached ArrayBuffers. Adds 32 AbortController/AbortSignal tests from the DOM spec (signal identity, reason propagation, dispatch reentrancy, listener ordering, throwIfAborted, AbortSignal.any composition, brand checks) and 28 structuredClone tests from the HTML serialization spec (the DataCloneError set, getter invocation, property filtering, prototype reset, cycles through objects/arrays/Maps/Sets, reference identity, Error serialization). Everything passes except Error cause, which Bun drops and which Node/V8 and Firefox both serialize; that one is marked test.failing.
Also tighten two assertions: compare the thrown reason by identity rather than by message, and check the DataCloneError name on the transfer-list case.
Awaiting the abort event of an AbortSignal.timeout() wedges the test runner on Windows, which is what the 'FIXME: test runner hangs' note in abort.ts already warned about. A bounded poll on signal.aborted fails loudly instead of hanging the whole file.
The bounded poll in abort.test.ts works around a real bug rather than a quirk of the test, so it is filed as #33334 and the helper points at it. Reverting it to await the event is the signal that the bug is fixed.
fb6cf9c to
2dc9a18
Compare
There was a problem hiding this comment.
I didn't find any correctness issues in the tests themselves, but you've left two calls open for a maintainer — whether to keep the AbortSignal.any block given the overlap with #25406/#25407, and confirming the test.failing invariants (DOMException stack shape, Error.cause serialization) — so this should get a human sign-off rather than a bot approval.
Checked: abort-event dispatch/reentrancy tests match DOM spec ordering; throwIfAborted identity assertions are strict (toBe, not toThrow); waitUntilAborted polling is bounded and the Windows hang is filed as #33334; the expectDataCloneError helper and structuredClone cycle/identity/hole tests match HTML serialization semantics and don't conflict with the existing object-pool tests in the same file.
Extended reasoning...
Overview
Test-only PR adding ~64 spec-derived tests across test/js/web/abort/abort.test.ts (+295 lines) and test/js/web/workers/structured-clone.test.ts (+178 lines). No src/ changes. Covers AbortController/AbortSignal DOM semantics (reason identity, event dispatch ordering, throwIfAborted, AbortSignal.abort/timeout/any, brand checks) and structuredClone HTML serialization semantics (DataCloneError cases, getter/enumerable/symbol-key handling, cycles, Error cloning). Five test.failing cases pin known divergences (DOMException never captures a stack; Error.cause dropped on clone).
Security risks
None. Pure test additions with no production code paths, no network/filesystem access beyond the existing harness patterns, no credentials or auth logic.
Level of scrutiny
Moderate. The tests are well-constructed, spec-cited, and follow repo conventions (bounded polling instead of sleeps, identity assertions via toBe, test.failing for known divergences). All three CodeRabbit comments were addressed and resolved. However, the author has explicitly deferred two decisions to a maintainer in the thread: (1) whether the AbortSignal.any describe block should be dropped in favor of the older #25406/#25407, and (2) confirmation that the five test.failing invariants — particularly the exact shape expected of DOMException.stack and that structuredClone must preserve Error.cause — are what Bun wants to commit to. Approving over those open questions would preempt a decision the author intentionally left to a human.
Other factors
The waitUntilAborted polling helper is a documented workaround for a filed Windows runner hang (#33334), with a comment saying to revert once fixed — reasonable, but another thing a maintainer may want to weigh in on. The PR is also fairly large (~470 lines) for a test addition, which alone nudges toward human review.
|
CI status: build 72461 settled at 283 passed, 3 failed. None of the three is this diff.
This diff adds tests only, to Both files pass locally: Ready for a maintainer on the two open calls in the thread above (the |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-13, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
Two web-platform primitives with surprisingly thin coverage. Expectations were written from the DOM and HTML specs, then cross-checked against Node, rather than read off Bun's current output.
AbortSignal / AbortController (+36 tests)
test/js/web/abort/abort.test.tshad 5 tests, three of which were process-spawn smoke checks. Notable holes:AbortSignal.prototype.throwIfAborted()was never called anywhere intest/.AbortController.abort(reason)coverage lived in a fixture that callscontroller.abort()with no argument in the loop body, so custom reasons were never actually exercised.AbortSignal.timeout()firing wastest.skip'd in that fixture with a "test runner hangs" note. It does not hang; there is now a test that awaits theabortevent.Now covered:
[SameObject]signal, non-constructibleAbortSignal, reason identity (including falsy reasons), defaultAbortErrorDOMException, secondabort()being a no-op, abort event shape, listeners added after abort, aborting from inside an abort listener, listeners added during dispatch,onabortposition in the listener list,throwIfAbortedidentity/falsy/default,AbortSignal.abort,AbortSignal.timeoutfiring and argument validation,AbortSignal.any(reason identity, already-aborted sources, empty list, composite-of-composite, any iterable, fires once, TypeError paths), and a getter brand check.structuredClone (+28 tests)
DataCloneErrorwas only exercised for detached ArrayBuffers. Now: functions, arrow functions, symbols (as values, property values and array elements),WeakMap,WeakSet,WeakRef,globalThis,Promise, aProxy, a detachedArrayBuffer, and a typed array in the transfer list.Object semantics from the serialization spec: own getters invoked exactly once and landing as data properties, non-enumerable and symbol-keyed properties dropped, explicit
undefinedkept, holes kept, clone prototype reset toObject.prototype,RegExplastIndexreset, Map/Set insertion order, reference identity across sibling properties, cycles through objects, arrays, Map values and Set members, and a Map key that is also its own value.Divergences it found
Marked
test.failing, so CI stays green and each reports the moment it is fixed.DOMExceptionnever captures a stack.Node gives
"AbortError: ...\n at new DOMException (...)"for both.AbortSignal.timeout()'s reason hasstack === "", andcontroller.abort()'s has"abort@[native code]\n..."rather than a header naming the error. This is the root cause of #17877, #25182 and #21900.structuredClonedropsError.cause.structuredClone(new Error("x", { cause: new RangeError("y") })).causeisundefined; Node/V8 and Firefox both serialize it.Everything else passes. #18357 (
structuredCloneof an error loses the stack) does not reproduce on main, and the clone's stack is now asserted.Verification
No
src/changes.[stamp-90s] gate passed · iteration 1 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file