Fix process abort when expect matcher utils inspect a value whose custom inspect throws - #36912
Fix process abort when expect matcher utils inspect a value whose custom inspect throws#36912robobun wants to merge 1 commit into
Conversation
…ue throws printReceived/printExpected/stringify formatted the value through the Display adapter into a std::io::Write sink. When inspecting the value threw a JS exception (e.g. a throwing [util.inspect.custom]), the Display impl returned fmt::Error while the Vec sink reported no error, so io::Write::write_fmt panicked with "a formatting trait implementation returned an error when the underlying stream did not", aborting the process and bypassing the user's try/catch. Add Formatter::format_value, a fallible single-value entry point that propagates the JsError, and use it in ExpectMatcherUtils::print_value so the exception surfaces as a catchable JS error.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesFormatter Error Propagation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Note: #36911 fixes the same panic mechanism at a different site (test.each title formatting) and adds an equivalent helper, ZigFormatter::write_to, to ConsoleObject.rs. The two helpers do the same thing from different entry points. Whichever PR lands second should drop its copy and reuse the other's; happy to rebase this one onto #36911 if that merges first. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate / same-class question raised above: I went through every
These were filed and picked up as separate reports, which is why they ended up as separate PRs. If you would rather review a single PR for the whole class, say the word and I will pull the |
…36911) ### Problem A `test.each` / `describe.each` title interpolating a value via `$path` (or `%p`) aborts the test runner when formatting that value throws, e.g. a throwing `[Symbol.for("nodejs.util.inspect.custom")]`: ```js import { test } from "bun:test"; test.each([{ a: { b: { [Symbol.for("nodejs.util.inspect.custom")]() { throw 1 } } } }])("case $a.b", () => {}); ``` ``` panic: a formatting trait implementation returned an error when the underlying stream did not ``` ### Cause `format_label` in `src/runtime/test_runner/jest.rs` formats non-string interpolated values with `write!(&mut list, "{}", value.to_fmt(&mut formatter))` through `std::io::Write`. `ZigFormatter`'s `Display` impl collapses the pending JS exception to `fmt::Error`, and `std::io::Write::write_fmt` panics when the `Display` impl errors while the sink (`Vec<u8>`) did not. The `%p` branch in the same function has the identical bug. ### Fix Add `Formatter::format_value`, a fallible single-value entry point that propagates the `JsError` (thrown exception, termination, OOM) instead of collapsing it to `fmt::Error`, and use it at both `format_label` sites. The thrown exception now surfaces as a test error and `bun test` exits 1 instead of crashing. The `Formatter::format_value` hunk is byte-identical to the one in #36912 so the two merge cleanly in either order. Sibling sites with the same `write!`-through-`Display` pattern that are intentionally not touched here because they already have their own PRs: `ExpectMatcherUtils::print_value` in `expect.rs` (#36912) and `bun_inspect` in `BunObject.rs` (#30980). ### Verification New test in `test/cli/test/bun-test.test.ts` covers `test.each` and `describe.each` for both the `$path` and `%p` sites, one fixture file per variant with a distinct error message (the declaration throw aborts module evaluation, so a single file can only exercise the first one). It aborts with the panic on a build without the fix and passes with it. The full `bun-test.test.ts` file passes. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/bun-test.test.ts <!-- robobun:evidence:end -->
…ven-sh#36911) ### Problem A `test.each` / `describe.each` title interpolating a value via `$path` (or `%p`) aborts the test runner when formatting that value throws, e.g. a throwing `[Symbol.for("nodejs.util.inspect.custom")]`: ```js import { test } from "bun:test"; test.each([{ a: { b: { [Symbol.for("nodejs.util.inspect.custom")]() { throw 1 } } } }])("case $a.b", () => {}); ``` ``` panic: a formatting trait implementation returned an error when the underlying stream did not ``` ### Cause `format_label` in `src/runtime/test_runner/jest.rs` formats non-string interpolated values with `write!(&mut list, "{}", value.to_fmt(&mut formatter))` through `std::io::Write`. `ZigFormatter`'s `Display` impl collapses the pending JS exception to `fmt::Error`, and `std::io::Write::write_fmt` panics when the `Display` impl errors while the sink (`Vec<u8>`) did not. The `%p` branch in the same function has the identical bug. ### Fix Add `Formatter::format_value`, a fallible single-value entry point that propagates the `JsError` (thrown exception, termination, OOM) instead of collapsing it to `fmt::Error`, and use it at both `format_label` sites. The thrown exception now surfaces as a test error and `bun test` exits 1 instead of crashing. The `Formatter::format_value` hunk is byte-identical to the one in oven-sh#36912 so the two merge cleanly in either order. Sibling sites with the same `write!`-through-`Display` pattern that are intentionally not touched here because they already have their own PRs: `ExpectMatcherUtils::print_value` in `expect.rs` (oven-sh#36912) and `bun_inspect` in `BunObject.rs` (oven-sh#30980). ### Verification New test in `test/cli/test/bun-test.test.ts` covers `test.each` and `describe.each` for both the `$path` and `%p` sites, one fixture file per variant with a distinct error message (the declaration throw aborts module evaluation, so a single file can only exercise the first one). It aborts with the panic on a build without the fix and passes with it. The full `bun-test.test.ts` file passes. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/bun-test.test.ts <!-- robobun:evidence:end -->
Problem
In an
expect.extendmatcher,this.utils.printReceived(v),printExpected(v), andstringify(v)abort the whole process when inspectingvthrows, and the user's own try/catch never runs:Cause
ExpectMatcherUtils::print_valueformatted the value through theDisplayadapter (ZigFormatter) into astd::io::Writesink. When inspecting the value throws a JS exception, theDisplayimpl returnsfmt::Errorwhile the in-memory Vec sink reported no error, soio::Write::write_fmtpanics with "a formatting trait implementation returned an error when the underlying stream did not". Thelet _ =around the write cannot swallow a panic.Fix
Added
Formatter::format_value, a fallible single-value entry point that propagates theJsErrorinstead of flattening it tofmt::Error, and switchedprint_valueto it. The exception from the user's inspect method now surfaces as a normal catchable JS error fromstringify/printExpected/printReceived(matching how the rest of the matcher pipeline propagates exceptions).Verification
New test
test/js/bun/test/expect-extend-matcher-utils-throw.test.tscrashes the process on bun 1.4.0-canary (1498d7b) and passes with this change. Also ranexpect.test.js,expect-extend.test.js,expect-extend-asymmetric-match-throw.test.ts,expect-extend-preload.test.ts, andjest-extended.test.js(503 pass, 0 fail).