Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/jsc/ConsoleObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1810,6 +1810,34 @@
}
}

impl ZigFormatter<'_, '_> {
/// Format into a byte sink, propagating the real `JsError` (a throwing
/// `[util.inspect.custom]`, termination, OOM). The `Display` impl below
/// collapses that to `fmt::Error`, which `std::io::Write::write_fmt`
/// turns into a panic when the sink itself never errored; use this
/// wherever the caller can `?`-propagate instead.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn write_to(&self, writer: &mut dyn bun_io::Write) -> JsResult<()> {
let formatter: &mut Formatter<'_> = self
.formatter
.take()
.expect("ZigFormatter::write_to re-entered or used after consumption");

formatter.stack_check.update();
let one = [self.value];
formatter.remaining_values = bun_ptr::RawSlice::new(&one);

let result = (|| {
let tag = Tag::get(self.value, formatter.global_this)?;
let global = formatter.global_this;
formatter.format::<false>(tag, writer, self.value, global)
})();

formatter.remaining_values = bun_ptr::RawSlice::EMPTY;
self.formatter.set(Some(formatter));
result
}
}

Check warning on line 1839 in src/jsc/ConsoleObject.rs

View check run for this annotation

Claude / Claude Code Review

Same-class panic left unfixed at sibling write!-into-Vec sites

The same `write!`-into-infallible-sink panic this PR fixes remains at two sibling sites: `src/runtime/test_runner/expect.rs:2801` (`ExpectMatcherUtils::print_value`, reachable via `this.utils.printReceived/printExpected/stringify` in custom matchers — same crate, already returns `JsResult`, one-line swap to `write_to`) and `src/runtime/api/BunObject.rs:723` (`bun_inspect`, where the `.is_err()` branch is dead code because `write_fmt` panics before returning). Per REVIEW.md's whole-class rule, ei
Comment thread
robobun marked this conversation as resolved.
Outdated

impl core::fmt::Display for ZigFormatter<'_, '_> {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
// Move the unique `&mut Formatter` out of the cell for the body;
Expand Down
9 changes: 5 additions & 4 deletions src/runtime/test_runner/jest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -709,8 +709,10 @@ pub(crate) fn format_label(
list.extend_from_slice(owned_slice.slice());
} else {
let mut formatter = crate::test_runner::expect::make_formatter(global_this);
// formatter cleanup handled by Drop.
write!(&mut list, "{}", value.to_fmt(&mut formatter)).unwrap();
// formatter cleanup handled by Drop. `write_to` (not
// `write!`) so a throwing custom formatter surfaces as
// a JS error instead of a panic in `write_fmt`.
Comment thread
robobun marked this conversation as resolved.
Outdated
value.to_fmt(&mut formatter).write_to(&mut list)?;
}
idx = var_end;
continue;
Expand Down Expand Up @@ -787,8 +789,7 @@ pub(crate) fn format_label(
}
b'p' => {
let mut formatter = crate::test_runner::expect::make_formatter(global_this);
let value_fmt = current_arg.to_fmt(&mut formatter);
write!(&mut list, "{}", value_fmt).unwrap();
current_arg.to_fmt(&mut formatter).write_to(&mut list)?;
idx += 1;
args_idx += 1;
}
Expand Down
15 changes: 15 additions & 0 deletions test/cli/test/bun-test.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1218,6 +1218,21 @@
expect(stderr).toContain("First user: Alice with tag: admin");
});

test("surfaces a throwing custom formatter in the interpolated value as a test error", () => {
const stderr = runTest({
args: [],
expectExitCode: 1,
input: `
import { test } from "bun:test";

test.each([{ a: { b: { [Symbol.for("nodejs.util.inspect.custom")]() { throw new Error("boom from inspect.custom"); } } } }])("case $a.b", () => {});
test.each([[{ [Symbol.for("nodejs.util.inspect.custom")]() { throw new Error("boom from inspect.custom"); } }]])("case %p", () => {});
`,
});

expect(stderr).toContain("boom from inspect.custom");
});

Check warning on line 1234 in test/cli/test/bun-test.test.ts

View check run for this annotation

Claude / Claude Code Review

Test does not exercise the %p code path it claims to cover

The second `test.each` (the `%p` case) is never reached: `format_label`'s error propagates via `?` out of the `#[bun_jsc::host_fn]` at `ScopeFunctions.rs:229/251`, throwing synchronously during module evaluation, so the first `$a.b` case aborts the file before the `%p` line runs. The single `toContain("boom from inspect.custom")` is satisfied by the `$path` case alone — reverting the `%p` fix at `jest.rs:792` back to `write!(...).unwrap()` would not break this test. Pass an array of two input st
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.

test("handles missing properties gracefully", () => {
const cases = [{ a: 1 }];

Expand Down
Loading