diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index ac42b0903872..c66883855015 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -5600,14 +5600,7 @@ impl VirtualMachine { let mut top_frame_is_builtin = false; if self.hide_bun_stackframes { for (i, frame) in frames.iter().enumerate() { - if frame.source_url.has_prefix_comptime(b"bun:") - || frame.source_url.has_prefix_comptime(b"node:") - || frame.source_url.is_empty() - || frame.source_url.eql_comptime("native") - || frame.source_url.eql_comptime("unknown") - || frame.source_url.eql_comptime("[unknown]") - || frame.source_url.has_prefix_comptime(b"[source:") - { + if !frame.has_user_source() { top_frame_is_builtin = true; continue; } @@ -6123,10 +6116,7 @@ impl VirtualMachine { let mut top_frame: Option<&crate::ZigStackFrame> = frames.first(); if self.hide_bun_stackframes { for frame in frames { - if frame.position.is_invalid() - || frame.source_url.has_prefix_comptime(b"bun:") - || frame.source_url.has_prefix_comptime(b"node:") - { + if frame.position.is_invalid() || !frame.has_user_source() { continue; } top_frame = Some(frame); @@ -6549,29 +6539,26 @@ impl VirtualMachine { let name = &exception.name; let message = &exception.message; let frames = exception.stack.frames(); - let top_frame = frames.first(); + let location_frame = frames + .iter() + .find(|frame| frame.has_user_source() && !frame.position.is_invalid()); let dir = bun_core::env_var::GITHUB_WORKSPACE::get() .unwrap_or_else(|| bun_bundler::bun_fs::FileSystem::instance().top_level_dir); bun_core::Output::flush(); let writer = bun_core::Output::error_writer(); - let mut has_location = false; - if let Some(frame) = top_frame { - if !frame.position.is_invalid() { - let source_url = frame.source_url.to_utf8(); - let file = bun_paths::resolve_path::relative(dir, source_url.slice()); - let _ = write!( - writer, - "\n::error file={},line={},col={},title=", - bun_core::fmt::github_action_property(file), - frame.position.line.one_based(), - frame.position.column.one_based(), - ); - has_location = true; - } - } - if !has_location { + if let Some(frame) = location_frame { + let source_url = frame.source_url.to_utf8(); + let file = bun_paths::resolve_path::relative(dir, source_url.slice()); + let _ = write!( + writer, + "\n::error file={},line={},col={},title=", + bun_core::fmt::github_action_property(file), + frame.position.line.one_based(), + frame.position.column.one_based(), + ); + } else { let _ = writer.write_all(b"\n::error title="); } @@ -6612,7 +6599,7 @@ impl VirtualMachine { let _ = writer.write_all(b"::"); } - if top_frame.is_some() { + if !frames.is_empty() { // SAFETY: per-thread VM. let vm = VirtualMachine::get(); let origin = if vm.is_from_devserver { diff --git a/src/jsc/ZigStackFrame.rs b/src/jsc/ZigStackFrame.rs index 4909e063006a..894ba0f94614 100644 --- a/src/jsc/ZigStackFrame.rs +++ b/src/jsc/ZigStackFrame.rs @@ -75,6 +75,22 @@ impl ZigStackFrame { jsc_stack_frame_index: -1, }; + /// Whether `source_url` names a source the user can open. False for JSC's + /// JS builtins (no URL, or `native`/`unknown` once parsed back out of + /// `error.stack`), for Bun's own `bun:`/`node:` modules, and for sources + /// JSC could not attribute. The code frame and the GitHub Actions + /// annotation point at the first frame for which this is true. + pub(crate) fn has_user_source(&self) -> bool { + let url = &self.source_url; + !(url.is_empty() + || url.has_prefix_comptime(b"bun:") + || url.has_prefix_comptime(b"node:") + || url.eql_comptime("native") + || url.eql_comptime("unknown") + || url.eql_comptime("[unknown]") + || url.has_prefix_comptime(b"[source:")) + } + pub fn name_formatter(&self, enable_color: bool) -> NameFormatter { NameFormatter { function_name: self.function_name, diff --git a/test/cli/test/bun-test.test.ts b/test/cli/test/bun-test.test.ts index 4bc66e446ba0..04c131471d94 100644 --- a/test/cli/test/bun-test.test.ts +++ b/test/cli/test/bun-test.test.ts @@ -719,6 +719,78 @@ describe("bun test", () => { }); expect(stderr).toMatch(/::error title=error: Test \"time out\" timed out after \d+ms::/); }); + // The frame on top of these stacks has no file to annotate: a JS builtin + // (`reduce`), the `native` / `unknown` placeholders such frames turn into + // once error.stack has been read, or one of bun's own `node:*` modules. + // The annotation has to point at the first frame below it that is in the + // test file. The test callbacks are async so the error reaches the + // reporter with its own stack; a synchronous throw is reported with the + // frames of the throw site instead, which would not have these on top. + test.each([ + { + label: "a JS builtin", + body: `[].reduce((a, b) => a);`, + callee: "reduce", + title: "TypeError: reduce of empty array with no initial value", + }, + { + label: "a JS builtin after error.stack was read", + body: `try { [].reduce((a, b) => a); } catch (e) { void e.stack; throw e; }`, + callee: "reduce", + title: "TypeError: reduce of empty array with no initial value", + }, + { + label: "a node: module", + body: `new EventEmitter().emit("error");`, + callee: "emit", + title: "error: Unhandled error. (undefined)", + }, + { + label: "a node: module after error.stack was read", + body: `try { new EventEmitter().emit("error"); } catch (e) { void e.stack; throw e; }`, + callee: "emit", + title: "error: Unhandled error. (undefined)", + }, + ])("should annotate the first frame in the test file when the top frame is $label", ({ body, callee, title }) => { + const lines = [ + `import { test } from "bun:test";`, + `import { EventEmitter } from "node:events";`, + `test("fail", async () => {`, + ` ${body}`, + `});`, + ]; + const line = lines.findIndex(l => l.includes(body)) + 1; + const col = lines[line - 1].indexOf(callee) + 1; + const stderr = runTest({ + input: [{ filename: "top-frame-has-no-file.test.ts", contents: lines.join("\n") }], + env: { + GITHUB_ACTIONS: "true", + }, + }); + const annotation = stderr.split("\n").find(l => l.startsWith("::error")); + expect(annotation).toStartWith("::error file="); + expect(annotation!.replace(/^::error file=(?:[^,]*[\\/])?/, "")).toStartWith( + `top-frame-has-no-file.test.ts,line=${line},col=${col},title=${title}::`, + ); + }); + test("should annotate without a location when no frame has a file", () => { + const stderr = runTest({ + input: ` + import { test } from "bun:test"; + test("fail", async () => { + const err = new Error("boom"); + err.stack = "Error: boom\\n at reduce (native:1:11)"; + throw err; + }); + `, + env: { + GITHUB_ACTIONS: "true", + }, + }); + const annotation = stderr.split("\n").find(l => l.startsWith("::error")); + expect(annotation).toStartWith("::error title=error: boom::"); + expect(annotation).toContain("at reduce ("); + }); }); describe(".each", () => { test("should run tests with test.each", () => { diff --git a/test/js/bun/test/stack.test.ts b/test/js/bun/test/stack.test.ts index 63b28630a3f6..6dc8f1f19189 100644 --- a/test/js/bun/test/stack.test.ts +++ b/test/js/bun/test/stack.test.ts @@ -149,3 +149,30 @@ test("Async functions frame should be included in stack trace", async () => { at async (file:NN:NN)" `); }); + +// `reduce` throws from inside JSC's builtin, so the first frame has a +// line:column inside the builtin's source and no file. The code frame shows +// this file, so its caret has to be placed by this file's frame as well. Once +// error.stack has been read, the frames are parsed back out of that string +// and the builtin frame turns into the `native` placeholder. +test.each([ + ["fresh error", false], + ["error whose .stack has been read", true], +])("code frame caret is placed by the first frame with a file, not a builtin frame on top: %s", (_, readStack) => { + let err: unknown; + try { + const emptyArray: number[] = []; + emptyArray.reduce((a, b) => a); + } catch (e) { + err = e; + } + if (readStack) void (err as Error).stack; + + const lines = Bun.inspect(err).split("\n"); + const header = lines.indexOf("TypeError: reduce of empty array with no initial value"); + expect(header).toBeGreaterThanOrEqual(2); + const [sourceLine, caretLine] = lines.slice(header - 2, header); + expect(sourceLine).toEndWith("emptyArray.reduce((a, b) => a);"); + expect(caretLine.trim()).toBe("^"); + expect(caretLine.indexOf("^")).toBe(sourceLine.indexOf("reduce")); +});