Conversation
A test file that fails to load (syntax error, top-level throw, unresolved import), or a describe() body that throws, is counted in the console summary and exit code but was absent from the JUnit XML: the reporter only ever wrote a <testcase> per executed test entry. A run where every file fails to load produced no outfile at all. CI dashboards that parse junit.xml would report such a run as green. on_uncaught_exception now captures ShowUnhandledErrorBetweenTests / ShowUnhandledErrorInDescribe into the JUnit reporter and emits a synthetic <testcase><error> for the current file (with a fallback for BuildMessage / ResolveMessage, which bypass the ZigException formatter). Each <testsuite> and <testsuites> now carries an errors= attribute, and write_to_file always writes the outfile.
WalkthroughChangesJUnit error reporting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… open; AggregateError fallback Address review feedback: - merge_junit_fragments now reads and emits errors= so the merged <testsuites> root matches its inner suites under --parallel; the crashed-worker synthetic suite also carries errors="0" for schema consistency. - write_unhandled_error only pops open describe suites when switching files, so a between-tests error mid-describe no longer splits the suite into siblings. - When a load error arrives as an AggregateError wrapping BuildMessage / ResolveMessage children, fall back to its .message for the <error> text. - Add a --parallel variant of the load-error test.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/test_runner/bun_test.rs`:
- Around line 1409-1410: Replace the panicking expect call on
junit.write_unhandled_error in the unhandled-error path with the established
handle_oom or unwrap_or_oom mechanism, ensuring allocation failures are handled
gracefully rather than causing a process panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2103104-d9a2-44e9-84f2-1de63c2e2116
⛔ Files ignored due to path filters (1)
test/js/junit-reporter/__snapshots__/junit.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
src/runtime/cli/test/parallel/aggregate.rssrc/runtime/cli/test_command.rssrc/runtime/test_runner/bun_test.rstest/js/junit-reporter/junit.test.js
| self.contents | ||
| .extend_from_slice(b"<testcase name=\"(load error)\" classname=\""); |
There was a problem hiding this comment.
🟡 The synthetic <testcase> is hardcoded to name="(load error)", but write_unhandled_error is now also invoked for ShowUnhandledErrorBetweenTests during Phase::Execution — e.g. a late setTimeout throw between tests in a file that loaded fine and already has passing testcases in the same suite. The label is misleading in that case; consider threading the HandleUncaughtExceptionResult (or a &'static [u8] label) into write_unhandled_error so only collection/load-phase errors say (load error) and the between-tests path says (unhandled error). Cosmetic — counts and XML well-formedness are unaffected.
Extended reasoning...
What the bug is
write_unhandled_error at src/runtime/cli/test_command.rs:887-888 unconditionally emits:
self.contents
.extend_from_slice(b"<testcase name=\"(load error)\" classname=\"");But the function's own doc comment (line 854-858) enumerates four call paths: "file failed to load, top-level throw, throwing describe body, rejection between tests". The last of these is not a load error — the file loaded successfully, tests already ran, and the error surfaced from a stale timer/rejection between test entries during Phase::Execution.
The specific code path
In src/runtime/test_runner/bun_test.rs, on_uncaught_exception computes is_unhandled as:
let is_unhandled = matches!(
handle_status,
HandleUncaughtExceptionResult::ShowUnhandledErrorBetweenTests
| HandleUncaughtExceptionResult::ShowUnhandledErrorInDescribe
);and calls junit.write_unhandled_error(file) whenever is_unhandled && !junit_ctx.is_null(). ShowUnhandledErrorBetweenTests is returned by Execution::handle_uncaught_exception during Phase::Execution for a user_data that fails the sequence lookup (a late setTimeout throw from a prior test, a stale unhandled rejection), and by the Phase::Done arm unconditionally. In neither case did the file fail to load.
Why existing code doesn't prevent it
write_unhandled_error receives only the file path — it has no way to distinguish which HandleUncaughtExceptionResult variant triggered it, so the label is baked in. The new tests only exercise collection-phase errors (syntax error, top-level throw, missing import, throwing describe body), all of which genuinely are load/collection failures, so the between-tests path is uncovered.
Step-by-step proof
Given:
import { test } from "bun:test";
test("a", () => { setTimeout(() => { throw new Error("late") }, 0); });
test("b", () => {});- File loads and parses successfully;
Phase::Collectioncompletes. - Test
aruns and passes →maybe_print_junit_lineemits<testcase name="a" .../>inside the file's<testsuite>. - The queued
setTimeoutfires; the throw reacheson_uncaught_exceptionwith aRefDataValuethat no longer matches the active sequence →Execution::handle_uncaught_exceptionreturnsShowUnhandledErrorBetweenTests→is_unhandled = true. write_unhandled_error(file)runs and emits<testcase name="(load error)" ...><error type="Error" message="late">...</error></testcase>in the same suite as the passingaandbtestcases.- Test
bruns and emits<testcase name="b" .../>.
Result: the JUnit report shows a suite for a successfully-loaded file containing two passing tests plus a testcase labelled (load error) — a CI dashboard reader would reasonably conclude the file failed to load, contradicting the sibling passing testcases.
Impact
Cosmetic only. The XML remains well-formed, errors= counts are correct, and the exit code is unaffected. Before this PR the between-tests error was entirely absent from the JUnit report, so this is strictly better. Only the human-readable name= attribute is inaccurate for one of the four documented call paths. REVIEW.md's "Name things truthfully" applies.
Fix
Add a label: &'static [u8] parameter to write_unhandled_error (or pass handle_status) and have the caller in bun_test.rs supply b"(load error)" for ShowUnhandledErrorInDescribe / Phase::Collection and b"(unhandled error)" for ShowUnhandledErrorBetweenTests. That keeps the console banner ("Unhandled error between tests") and the JUnit label consistent.
…it report (#37483) ### Problem `scripts/runner.node.mjs` runs the parallel-safe bucket as one `bun test --parallel --reporter=junit` (#36175) and reads the report to decide which files failed, what to re-run alone, and the `(X.XXs)` it prints per file. It collected the suites like this: ```js for (const [, attrs] of xml.matchAll(/<testsuite\b([^>]*)>/g)) { const file = /\bfile="([^"]+)"/.exec(attrs)?.[1]; ... if (file) suites.set(file, { failures, seconds, cases: [] }); } ``` bun's reporter writes one `<testsuite name="<path>" file="<path>">` per file and, nested inside it, one `<testsuite name="<describe>" file="<path>">` per describe block (`begin_test_suite_with_line` in `src/runtime/cli/test_command.rs` puts `file=` on both kinds). Every suite of a file therefore overwrote the previous one, and a file with describe blocks ended up represented by whichever describe suite came last in the report. Report written by the released bun for a file with a failing top-level test, a failing `describe("first")` and a passing `describe("second")`, plus a file with a `describe.concurrent` of three 100ms tests: ```xml <testsuite name="test/a.test.ts" file="test/a.test.ts" tests="4" failures="2" time="0.002066623" ...> ... <testsuite name="first" file="test/a.test.ts" tests="1" failures="1" time="0" ...> <testsuite name="second" file="test/a.test.ts" tests="2" failures="0" time="0" ...> </testsuite> <testsuite name="test/b.test.ts" file="test/b.test.ts" tests="3" failures="0" time="0.100953121" ...> <testsuite name="conc" file="test/b.test.ts" tests="3" failures="0" time="0.3" ...> </testsuite> ``` The runner's loop turned that into `a.test.ts: { failures: 0, seconds: 0 }` and `b.test.ts: { seconds: 0.3 }`: * `a.test.ts` is not added to `failed`, and because the report exists (`evidence` is true) nothing is re-run: the file is printed as passed and pushed to `okResults`, so the bucket's non-zero exit is swallowed and the job goes green. Any bucket file whose failures are at top level or in a describe block other than the last one is affected. * The per-file time is a describe block's sum of test durations instead of the file suite's wall clock (`end_test_suite` times file suites from `file_start_ns` to `file_end_ns` and describe suites by summing their tests), so `describe.concurrent` files over-report and files with several describes under-report. These lines feed `scripts/ci-slowest-tests.ts` and `scripts/update-test-durations.mjs`. * The failing cases are attached to that same entry, and since the file is not classified as failed they are never printed in the "failing in the parallel batch" group or the flaky annotation. ### Fix The parsing moves to `parseJunitFileSuites()` in `scripts/utils.mjs` (the runner's existing helper module; `runner.node.mjs` itself runs on import, so it cannot be unit tested) and only keeps suites whose `name` is their `file`, which is how the reporter writes file suites. That is the right suite to read on its own terms: `end_test_suite` adds each closed suite's metrics to its parent, so the file suite's `failures` includes every nested describe block, and its `time` is the only wall clock measurement in the report. The synthetic suite the `--parallel` coordinator writes for a file whose worker crashed or was interrupted (`merge_junit_fragments` in `src/runtime/cli/test/parallel/aggregate.rs`) has the same `name == file` shape, so crashed and hung files are still classified as failed. The `<testcase>` loop is unchanged: it keys on each case's own `file` attribute. The runner's classification, printing and annotation code is unchanged apart from calling the helper. #36218 (in flight) keeps the same file-suite shape, so it is unaffected. The earlier draft of the batch runner, #29654, applied this same `name === file` rule in its `parsePassedFilesFromJunit()`; the rule did not make it into the version that landed in #36175. #29654 is a rewrite of the whole batch mechanism and conflicts with main, so this PR only restores the rule in the code that shipped. ### Tests `test/internal/runner-junit.test.ts`: * a hand-written report in the reporter's shape (top-level failure, a failing describe, a passing describe with a nested describe last, a second plain file): the file is reported with `failures: 2`, the file suite's `time`, and both failing cases with their names and messages unescaped; * Windows-style backslash paths and escaped characters are keyed the way the runner looks files up; * the coordinator's crashed-file suite is reported as one failure with no cases; * a real `bun test --parallel=2 --reporter=junit` run over a file with describe blocks, a plain file and a file that calls `process.exit()`: the report contains four suites carrying the describe file's path, and the parsed result has the file suite's counts, its `time`, the two failing cases, and the crash marker. With the previous behaviour (every suite with a `file` attribute) the first and last of these fail (`failures: 0` and the last describe block's time for the file with describe blocks); with this change all five pass under `bun bd test` and with the released bun. `node scripts/runner.node.mjs --exec-path ./build/debug/bun-debug js/web/encoding` still forms the 8-file bucket, prints the file suites' times, and re-ran the one file the interrupted batch marked as failed.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-28 and it conflicts with main. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
Problem
A test file that fails to load (syntax error, top-level
throw, unresolved import), or adescribe()body that throws, is counted on the console and in the exit code but is absent from the JUnit XML. The only<testcase>producer ismaybe_print_junit_line, reached solely from the executed-test-entry result path; load errors go through theStatus::Rejectedbranch ofrun_test_fileand describe/between-tests errors throughon_uncaught_exceptionwithShowUnhandledError*, neither of which ever touched the junit reporter. Becausewrite_to_fileearly-returned whencontentswas empty, a run where every file fails to load produced no outfile at all.Net effect: the artifact CI dashboards parse reports
tests="1" failures="0"(or nothing) for a run whose console is red and whose exit code is 1.Fix
JunitReporter::write_unhandled_error(file)emits a synthetic<testcase name="(load error)" file="..."><error type=".." message="..">..</error></testcase>inside the file's<testsuite>, counted in a newerrorsfield onMetrics.on_uncaught_exceptionnow installs theZigExceptioncapture callback forShowUnhandledErrorBetweenTests/ShowUnhandledErrorInDescribeas well, then callswrite_unhandled_error. Any pending handled-errorlast_failureis stashed/restored so it still reaches its ownwrite_test_case.BuildMessage/ResolveMessage(syntax errors, unresolved imports) bypass theZigExceptionformatter entirely, so their message text is extracted directly viarecord_failure_textwhen the callback never fired.end_test_suite/write_to_filenow emiterrors="N"on every<testsuite>/<testsuites>;write_to_filealways writes the outfile (thecontents.is_empty()gate is replaced with header emission).After:
Tests
Two new cases in
test/js/junit-reporter/junit.test.js:records files that fail to load as <error> testcases: mixed run (one good file, syntax error, top-level throw, missing import, throwing describe body). Asserts every file has a<testsuite>, each error file carries an<error>with the actual message, and the<testsuites>aggregate reconciles (tests=5 failures=0 errors=4).writes the junit outfile even when every file fails to load: all-error run producestests=2 errors=2instead of no file.Both fail on current
bunand pass with this change. The existing snapshot is updated to includeerrors="0"on suites.[review] gate passed · iteration 0 · 4 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