bun:test: stop expect() failure messages from consuming <...> spans in user data - #34343
Conversation
…not the rendered message expect() failure messages were running the `<tag>` -> ANSI/strip rewrite over the fully rendered string (template + substituted user data), so any `<...>` span in a Received/Expected value was consumed as markup. Comparing two different HTML strings printed two byte-identical strings as unequal; under FORCE_COLOR a user's `<i>` became a live `\x1b[3m` italic escape. The Zig baseline applied Output.prettyFmt to the comptime template and substituted args afterwards, so user data was never scanned. Restore that contract: - Add `expect_throw!` which applies `pretty_fmt!` to the template literal at compile time (both colour branches) and substitutes positional args via `format_args!`. Re-exported as `throw!` inside the expect module. - Replace `Expect::throw` with `throw_rendered`, a pure concat sink that never scans its input for markup. - `get_signature` now caches the ANSI/stripped header pair so the failure path does no runtime markup work at all. - Convert every matcher failure site (~130 call sites across 40+ files) from `this.throw(g, sig, format_args!(tpl, ..))` to `throw!(this, g, sig, tpl, ..)`. - Delete `JSGlobalObject::throw_pretty` and the `JSGlobalObjectTestExt` shim; the handful of template-only callers move to the compile-time `throw_pretty_static!` or plain `throw` (shell/BunObject messages had no markup to begin with). - `throw_pretty_matcher_error` and `CustomMatcherParamsFormatter` now emit matcher_name/matcher_params/message verbatim; the custom-matcher message returned from expect.extend is no longer scanned for markup. - Drop the pre-escaped `\<`/`\>` glyphs in toHaveReturned/OrderingRelation now that operands go through as verbatim args. Affects toBe, toContain, toHaveProperty, toThrow, not.*, expect.any, custom labels, .pass/.fail messages, .resolves/.rejects, expect.extend messages.
|
Updated 5:09 AM PT - Jul 16th, 2026
❌ @autofix-ci[bot], your commit 60254a3 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34343That installs a local version of the PR into your bun-34343 --bun |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (45)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a 45-file change that introduces a new cross-cutting macro abstraction (expect_throw! / __expect_throw_dispatch! with the peel/pool ident-binding pattern) and deletes JSGlobalObject::throw_pretty, so it's worth a human look at the macro design and the incidental behavior changes.
Checked: pretty_fmt! accepts concat!(...) templates (existing usage in output.rs); create_error_instance already carries the mid-format clear_exception_except_termination handling that throw_pretty had; no stray callers of the removed throw_pretty/throw_fmt/Expect::throw remain; the \\< / \\>= escapes still in toBeCloseTo/toHaveBeenCalled templates are correct (template still goes through pretty_fmt!). Noted a few incidental message-content changes: toBeArrayOfSize/toContainEqual now emit the "Received:"/"Expected to contain:" prose that the old throw_fmt shim was silently dropping, and toBeEmpty/toThrow sites that previously called global.throw_pretty directly now route through throw_rendered and so respect expect(v, label) custom labels.
Extended reasoning...
Overview
Fixes expect() failure messages consuming <...> spans in user data by moving the <tag> → ANSI rewrite from a runtime pass over the rendered string to a compile-time pass over the template literal. Touches 45 files: a new expect_throw! macro in test_runner/mod.rs, deletion of JSGlobalObject::throw_pretty, a rewrite of get_signature to cache both colour variants, and ~130 mechanical call-site conversions across the expect/to*.rs matchers plus a handful of shell/BunObject callers.
Security risks
None. This is error-message formatting; no auth, crypto, network, or filesystem paths are touched. The one arguably security-adjacent aspect — user data being interpreted as ANSI escape markup under FORCE_COLOR=1 — is what the PR fixes.
Level of scrutiny
High. While the per-file conversions are mechanical, the core is a new #[macro_export] macro with a non-trivial recursive peel/pool pattern that binds each arg once via match ($sig, &($v), ..) to feed both colour branches without double-evaluation. CLAUDE.md flags "new cross-cutting abstractions need maintainer agreement before appearing inside a feature PR"; a maintainer should sign off on the macro shape, the crate-root #[macro_export] placement, and the deletion of the shared throw_pretty API.
Other factors
Several incidental behavior changes ride along: (1) toBeArrayOfSize and toContainEqual previously called throw_fmt with a template that was documented-ignored, so their failure messages were missing the "Received:" / "Expected to contain:" prose — now emitted correctly; (2) four toBeEmpty sites and several toThrow sites that called global.throw_pretty directly (bypassing custom_label) now go through throw_rendered and will show the user's expect(v, label) label instead of the default header; (3) the deleted __get_signature! const-macro had no remaining callers I could find. These all look like improvements but are user-visible message changes not covered by the new test. Combined with the scope, deferring to a human.
|
CI build 73795: all failures are tagged |
…e labels The "Expected to contain:" / "Received:" labels were being dropped from these matchers' failure messages (the format literal was passed to a discarded parameter). #34343 fixed the underlying bug as part of migrating all matchers to the throw! macro; these tests lock the message shape so it can't regress again.
2 participants
What
expect()failure messages were running the<tag>-> ANSI/strip rewrite over the fully rendered string (template + substituted user data). Any<...>span in a Received/Expected value was parsed as markup and consumed.Under
FORCE_COLOR=1it was worse:toContain("<i>")emitted a live\x1b[3mitalic escape, interpreting user data as markup.Affects
toBe,toContain,toHaveProperty,toThrow,not.*,.resolves/.rejects,expect(v, label),.pass/.failmessages, andexpect.extendcustom-matcher messages.Cause
JSGlobalObject::throw_pretty(args: Arguments<'_>)rendered the entireargs(template literal + user-data substitutions) into one buffer, then ranpretty_fmt_rtover that buffer. The Zig baseline appliedOutput.prettyFmtto the comptime template and substituted args viacreateErrorInstanceafterwards, so user data was never scanned.Fix
Restore the template-first contract by moving the
<tag>rewrite to compile time everywhere in the matcher failure path:expect_throw!macro (re-exported asthrow!in theexpectmodule) appliespretty_fmt!to the template literal at compile time for both colour branches, then substitutes positional args viaformat_args!. User data in the args is never scanned.Expect::throwbecomesthrow_rendered, a pure concat sink that emits its input verbatim.get_signaturecaches the ANSI/stripped header pair so the failure path does no runtime markup work.this.throw(g, sig, format_args!(tpl, ..))tothrow!(this, g, sig, tpl, ..).JSGlobalObject::throw_prettyand theJSGlobalObjectTestExt::throw_prettyshim are deleted; the template-only callers move to a compile-timethrow_pretty_static!macro or plainthrow(shell/BunObject messages had no markup to begin with).throw_pretty_matcher_errorandCustomMatcherParamsFormatternow emitmatcher_name/matcher_params/messageverbatim, so a custom matcher's returned message string is no longer scanned for markup.\</\>operator glyphs intoHaveReturned/OrderingRelationnow that operands are substituted verbatim.Verification
New
test/js/bun/test/expect-failure-message-angle-brackets.test.tsspawns a childbun testand asserts, for bothNO_COLORandFORCE_COLOR:<div ...>Hello</div>appears verbatim in the Expected/Received linestoContain("<i>")shows the literal"<i>"(and under colours, does not emit"\x1b[3m")<b>/<u>/<red>/<d>/<blue>/<magenta>in property paths, thrown Error messages, custom labels,.pass()messages,.resolvesreceived values, andexpect.extendmessages all render verbatim>glyph intoBeGreaterThanfailure output still rendersFails on 1.4.0-canary; passes with this change.
test/js/bun/test/expect.test.js(406 tests) passes unchanged.[review] gate passed · iteration 0 · 45 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file