bun:test: throw the formatter's own error from snapshot matchers instead of "Failed to pretty format value: " - #37334
Conversation
…value match_and_fmt_snapshot discarded the error from jest_snapshot_pretty_format and threw a new "Failed to pretty format value: ..." error instead. The original exception was still pending at that point, so rendering the value for the new message failed as well and create_error_instance cleared the pending exception, leaving the matcher throwing an Error whose message ends right after the colon. Every Err from jest_snapshot_pretty_format already has the real exception pending (or is OutOfMemory/Terminated, which the host function wrapper handles), so return it as is: the getter's own error now comes out of toMatchSnapshot / toMatchInlineSnapshot.
|
Status: reproduced on the released build ( Self-review done. The only change it produced is in comments (a79f8cf and the commit before it): the two test workarounds now name #3521 and #37331, and the Checked the PRs flagged as possibly related: #36911 (merged) and #36912 fix the Waiting on CI. |
WalkthroughSnapshot formatting now propagates original JavaScript exceptions. Tests cover direct and nested getters, ChangesSnapshot error propagation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — straightforward error-propagation fix that stops swallowing the user's exception.
What was reviewed:
- Confirmed
jest_snapshot_pretty_formatreturnsJsResult<()>and everyErrpath already has a pending JS exception (formatter?chain + explicitthrow_erroron flush), so the removed wrapper was only ever discarding the real error and re-throwing while an exception was pending. - Both callers of
match_and_fmt_snapshot(snapshotand the inline path) already?the result, so behavior on success is unchanged and no snapshot is written on failure. - Tests cover the five throwing entry points the formatter reads (
$$typeoftop-level/nested, Map/Setsize, DatetoJSON) across both matchers, assert error identity (not a wrapper), and guard the inline cases with a literal so a non-throwing build fails on mismatch instead of writing into the test file.
Extended reasoning...
Overview
The PR removes a 7-line error-wrapping branch in Expect::match_and_fmt_snapshot (src/runtime/test_runner/expect.rs) that caught the Err from jest_snapshot_pretty_format, discarded it, and threw a fresh "Failed to pretty format value: {value}" error while the original JS exception was still pending on the VM. It replaces that with a direct return of the JsResult<()>, matching how the jest_deep_match call three lines above already propagates via ?. The comment next to the flush-error throw_error in mod.rs is updated to reflect that callers now ? the result directly. Twelve test cases are added to bun-snapshots.test.ts.
Security risks
None. This only changes which error object is thrown from a test-runner matcher when user code inside a snapshotted value throws during formatting. No parsing, auth, filesystem, or network code is touched.
Level of scrutiny
Low. The runtime change is a net -6 lines that turns if x.is_err() { throw new-error } Ok(()) into x. I verified in mod.rs that jest_snapshot_pretty_format's only two error sources — JestPrettyFormat::format(...)? and out.flush().map_err(|e| global.throw_error(...))? — both leave a JS exception pending, so there is no case where the old wrapper was providing a message that would now be missing. I also confirmed via grep that the removed string was the only occurrence and that match_and_fmt_snapshot has exactly two callers, both of which ? the result and consult the snapshot store only after formatting succeeds.
Other factors
The old code was itself an exception-handling bug (throwing while an exception is pending, then formatting the value again through a formatter that bails on pending exceptions — hence the truncated "Failed to pretty format value: "). The tests are well-constructed per the repo's review rules: it.each over the variant matrix, exact error identity asserted (toBe(error), not just message match), inline-snapshot cases pass a literal so a non-throwing build fails on mismatch rather than writing into the test file, and the property-matcher case builds a fresh object per call to account for the in-place write documented in #35452. The PR body confirms the tests fail on the released build and on a debug build without the src change, and pass with it under BUN_JSC_validateExceptionChecks=1.
There was a problem hiding this comment.
LGTM — removes an error-swallowing wrapper so the getter's own exception surfaces from snapshot matchers.
What was reviewed:
- Confirmed
jest_snapshot_pretty_formatreturnsJsResult<()>and throws the flush writer error itself, so there is no non-JS error the removed wrapper needed to convert. - Checked that no test in the tree asserts on the old
"Failed to pretty format value"string. - Tests cover the variant matrix ($$typeof/size/toJSON × toMatchSnapshot/toMatchInlineSnapshot, identity check, property-matcher path) and were verified to fail on the released build.
Extended reasoning...
Overview
Three files: a 7-line deletion in src/runtime/test_runner/expect.rs (match_and_fmt_snapshot) that stops catching and replacing the error from jest_snapshot_pretty_format with a fresh "Failed to pretty format value: {value}" error; a comment-only trim in src/runtime/test_runner/mod.rs (the flush comment referenced the removed branch); and ~100 lines of new tests in bun-snapshots.test.ts.
The PR description explains why the wrapper was net-negative: at the point it fired, the getter's exception was already pending on the VM, so re-formatting {value} for the new message failed too and create_error_instance cleared the pending exception, leaving only the truncated prefix. The Rust port's jest_snapshot_pretty_format already returns JsResult<()> where every Err is a thrown JS exception (or OOM/Terminated, which the host wrapper handles), and the one non-JS failure mode — the writer flush — is explicitly thrown inside the trait impl. So direct propagation is correct, matching how jest_deep_match a few lines above already uses ?.
Security risks
None. This is error-message plumbing in the test runner; no untrusted-input parsing, allocation sizing, or privileged operations involved.
Level of scrutiny
Low-to-moderate. The functional change is a one-line simplification (remove wrapper, return the inner result). The only correctness question is whether any Err variant needed the wrapper's conversion — the PR body enumerates the cases and the flush path in mod.rs confirms the writer error is already thrown as a JS error. I grepped for the old message string to confirm no existing test depended on it.
Other factors
- Tests are thorough: five distinct user-code entry points into the formatter ($$typeof top-level and nested, Map.size, Set.size, Date.toJSON), both
toMatchSnapshotandtoMatchInlineSnapshot, an identity check that the thrown value is the getter's own Error object (not a wrapper), and the property-matcher-first path. Inline cases pass a snapshot argument so a non-throwing build fails on mismatch rather than writing into the file. - Author verified the tests fail on
USE_SYSTEM_BUN=1and on a debug build without the source change, and pass with it including underBUN_JSC_validateExceptionChecks=1. - The comment-cop bot's feedback (paragraph-long comment in mod.rs) was addressed in a79f8cf — the comment is now one line.
- The PR body flags a known one-line conflict with #37331 (which asserts the old message in a new test); whichever lands second updates it. That's a merge-order note, not a defect in this PR.
|
Updated 6:25 PM PT - Aug 10th, 2026
✅ @alii, your commit 519f50961ca4305be05e529c3532b8b388e1f948 passed in 🧪 To try this PR locally: bunx bun-pr 37334That installs a local version of the PR into your bun-37334 --bun |
There was a problem hiding this comment.
LGTM — removing the error-swallowing wrapper so the formatter's own exception propagates is the correct fix.
What was reviewed:
- Confirmed
match_and_fmt_snapshotnow returns theJsResultdirectly and the sole caller (snapshot) already propagates via?; no other site depends on the removed "Failed to pretty format value" message (grepped src/ and test/). - Checked that
jest_snapshot_pretty_formatin mod.rs already throws on flush failure, so everyErrvariant carries a real pending exception or is handled byto_js_host_call— nothing is left uncovered by dropping the wrapper. - Tests cover both snapshot matchers, five distinct getter paths into the formatter, exact error identity, and the property-matchers-matched-first branch; inline cases pass a snapshot arg so a non-throwing build fails on mismatch instead of writing into the test file.
Extended reasoning...
Overview
The PR deletes a 7-line error-rewrapping branch in Expect::match_and_fmt_snapshot (src/runtime/test_runner/expect.rs) so that when jest_snapshot_pretty_format returns Err, the original JS exception propagates instead of being replaced by a truncated "Failed to pretty format value: " message. A stale multi-line comment in src/runtime/test_runner/mod.rs that referenced the removed branch is trimmed to one line. 104 lines of new tests in test/js/bun/test/snapshot-tests/bun-snapshots.test.ts cover the fix.
Security risks
None. This is test-runner error-propagation code; no untrusted input parsing, auth, crypto, or filesystem writes are affected. The only behavioral change is which error object surfaces from a snapshot matcher when the user's own getter throws.
Level of scrutiny
Low-to-medium. The native change is a pure deletion of a broken error handler that was actively swallowing the real exception (and clearing it via the pending-exception path in create_error_instance). The PR body traces every Err variant jest_snapshot_pretty_format can return (Thrown, OutOfMemory, Terminated) and shows each is already handled correctly by the host-fn wrapper, so no case is left unhandled. I verified in mod.rs that the flush error is explicitly thrown, and the sole caller (snapshot()) propagates via ?. Grepping src/ and test/ confirmed nothing else references the removed message string.
Other factors
The tests are well-constructed per the repo's review rules: they exercise five distinct entry points into the formatter ($$typeof top-level and nested, Map/Set size, Date toJSON), both toMatchSnapshot and toMatchInlineSnapshot, assert exact error identity (toBe(error)) not just message substring, cover the property-matchers-matched path, and are verified to fail on the unfixed build (release + debug ASAN) and pass with the fix under BUN_JSC_validateExceptionChecks=1. The inline-snapshot cases pass a snapshot argument so a build that doesn't throw fails on mismatch rather than writing into the test file. The comment-cop bot's feedback about the multi-line comment was addressed in a79f8cf. The known interaction with #37331 (whichever lands second must update one assertion) is documented in the PR body.
Repro
Before (release and debug builds alike), the matcher throws an error that says nothing about what went wrong:
After,
Error: boomwith the getter's stack comes out of the matcher. The snapshot formatter reads$$typeofoff every object it visits (React element detection),sizeoff Maps and Sets, and JSON-stringifies Dates, so a throwing getter ortoJSONon any of those reaches this path.Cause
Expect::match_and_fmt_snapshot(src/runtime/test_runner/expect.rs) ignored theErrfromjest_snapshot_pretty_formatand threw a freshFailed to pretty format value: {value}error instead. At that point the getter's exception is still pending on the VM, so formatting{value}for the new message fails too (the console formatter bails when an exception is pending), andcreate_error_instancehandles that by clearing the pending exception and throwing whatever prefix had been written. The user's error is lost, and the message is cut off after the colon.The wrapper dates from the Zig version, where
jestSnapshotPrettyFormathad an inferred error set that also carried plain writer errors with no JS exception behind them. The Rust port returnsJsResult<()>and throws the flush error itself, so everyErrit returns is one of:Thrown: the real exception is pending. Every?insidepretty_format.rsis on a throwing JSC call, and the fourwrite_formatbranches (Response/Request/Blob/BuildArtifact) explicitly throw when nothing is pending.OutOfMemory/Terminated: the host function wrapper (to_js_host_call) turns these into anOutOfMemoryError/ leaves the termination pending.So there is nothing left for the wrapper to convert; it only ever replaced the real error with an empty one.
Fix
Return the result of
jest_snapshot_pretty_formatdirectly, the same way the property matcher check a few lines above already propagates exceptions fromjest_deep_match. All four snapshot matchers (toMatchSnapshot,toMatchInlineSnapshot,toThrowErrorMatchingSnapshot,toThrowErrorMatchingInlineSnapshot) go through this function. Formatting happens before the snapshot store is consulted, so nothing is written in either the old or the new behavior; the only change is which error is thrown.This matches Jest, where an error thrown while pretty-format serializes the value propagates out of the matcher with its original message (plugin
test()errors are rewrapped asPrettyFormatPluginError, keeping the message and stack).The comment in
mod.rsnext to the flush error pointed at the removed branch, so it is replaced with a one-liner.Tests
test/js/bun/test/snapshot-tests/bun-snapshots.test.ts, newdescribe("when formatting the received value throws"):toMatchSnapshot()andtoMatchInlineSnapshot(...)with a throwing$$typeofgetter on the received value (theTag::getcall at the top of the formatter), on a nested value (the property walk), a throwingsizegetter on a Map and on a Set, and a throwingtoJSONon a Date, each asserting the thrown message is the getter'sThe inline snapshot cases pass a snapshot argument, so a build that does not throw fails on the mismatch instead of writing into the test file. The 12 new cases fail on the released build and on a debug build without the
src/change (all withReceived message: "Failed to pretty format value: ") and pass with it, including underBUN_JSC_validateExceptionChecks=1. The rest oftest/js/bun/test/snapshot-tests/andci-restrictions.test.tspass with the change.Related: #37331 makes the ordered property walk stop at a throwing property; without it a throwing nested getter that does not sort last is dropped from the output before reaching this code, which is why the nested test case uses a single property. Its added test in this same file currently asserts the old
Failed to pretty format valuemessage, so whichever of the two lands second needs to update that assertion to the getter's message. The property matcher test builds a fresh object per call because matched matchers are written into the received object (#3521, being removed in #35452).[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file