bun test --reporter=junit: fix broken XML, double-escaped classname, and empty <failure> - #34975
Conversation
…and empty <failure> escape_xml previously wrote a numeric reference for every C0 control character but neither flushed the pending run nor advanced last, so the raw byte was also emitted and the reference appeared in the wrong position. The result was not well-formed XML 1.0 (and a literal � would not be legal anyway). escape_xml now passes TAB/LF/CR through and drops every other C0 control character. The describe-scope names used for classname were XML-escaped while being joined and then escaped again inside write_test_case, so a describe title like 'suite <a> & "b"' rendered as '&lt;a&gt;' in the report. The join now concatenates the raw names with a raw ' > ' separator and leaves the single escape to write_test_case. <failure> for a thrown error was always emitted as <failure type="AssertionError" /> with no message or stack. on_uncaught_exception now records the error name, message, and a colourless rendering on the JunitReporter; write_test_case emits them as the failure's type and message attributes and text body. Timeouts also get a message attribute.
WalkthroughChangesAdds a JUnit failure reporting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This runs the exception formatter twice. Can we instead wire writing the log through run_error_handler, like we already do for GitHub Actions and similar?
|
Good call. I'll switch to the GitHub Actions pattern: add a fn-ptr slot on |
…of re-formatting Addresses review feedback: the previous approach ran to_zig_exception and print_errorlike_object a second time for every failed test. Now the stderr render path populates a single ZigException and the junit reporter reads from it via a fn-ptr slot on VirtualMachine (same lifecycle as the GitHub Actions annotation hook and the debugger lifecycle reporter). The <failure> body is built from the remapped name/message/stack frames, matching the standard Error.stack shape CI ingesters expect.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…y attempt record_failure was routing the computed relative path through SourceURLFormatter as root_path, which that formatter ignores on the no-colour no-origin path, so the raw absolute source_url was written instead. Emit the relative path (with forward slashes on Windows) and line:col directly so the failure body is platform-stable and needs no temp-dir substitution in the snapshot. Also clear last_failure at the start of each top-level on_uncaught_exception so an exhausted retry reports the final attempt rather than accumulating every attempt's stack under the first attempt's message.
escape_xml drops ESC, so a colour leak would surface as bare [Nm sequences; the previous assertion could never fail.
|
CI build 77656 on 49cf823:
Ready for another look. |
…boundary escape_xml now emits 	/ / instead of the literal bytes so attribute values survive XML 1.0 §3.3.3 attribute-value normalisation in compliant parsers. Moved the last_failure reset from on_uncaught_exception to the retry/repeat boundary in advance_sequence: a test body that throws followed by a throwing afterEach (or an unhandled rejection drained in the same attempt) now keeps the primary failure as the <failure> type/message and appends the secondary error to the body, while an exhausted retry still reports only the final attempt. New test covers the afterEach case.
There was a problem hiding this comment.
49cf823 addresses both remaining points — TAB/LF/CR now emit numeric references, and the last_failure clear moved to the retry/repeat boundary in advance_sequence with the new afterEach test covering the within-attempt accumulation. No new findings this round. Leaving the final call to a maintainer given the new VM-level on_print_error_zig_exception hook and the acknowledged test.concurrent interleaving deferral.
What was reviewed:
escape_xmlC0 handling against XML 1.0 §2.2/§3.3.3; verified the flush/advance now brackets both new arms.discard_junit_failureplacement — retry and repeat both clear,write_test_casealso clears at the tail so a passing repeat after a failing attempt won't carry stale state into a later failure.- Hook install/uninstall around
run_error_handleris symmetric on both branches;junit_ctxpoints into theBox<JunitReporter>heap so the intermediatereporter.as_ref()inon_before_printdoesn't invalidate it. - Snapshot now uses cwd-relative forward-slash frame paths, no temp-dir substitution.
Extended reasoning...
Overview
Three-part fix to the JUnit XML reporter: (1) escape_xml now flushes+advances around C0 bytes, dropping illegal ones and emitting 	/ / for TAB/LF/CR; (2) describe-scope classname joining no longer pre-escapes before write_test_case's own escape; (3) a new VirtualMachine::on_print_error_zig_exception fn-ptr hook lets on_uncaught_exception capture the already-remapped ZigException name/message/stack into JunitReporter::last_failure, which write_test_case then serialises into <failure type= message=>body</failure>. Supporting changes: discard_junit_failure at retry/repeat boundaries, cwd-relative forward-slash frame paths, timeout message=.
Security risks
None. Output-only reporter path; no untrusted-input parsing, no auth/crypto/network. The new VM hook is a plain fn-ptr + ctx set/cleared around a single synchronous call, gated on the junit reporter being active.
Level of scrutiny
Medium-high. The XML escaping and classname changes are mechanical and well-covered. The failure-capture plumbing is more involved: it adds two fields to VirtualMachine, threads a raw *mut JunitReporter through a VM callback, and coordinates state across on_uncaught_exception → print_error_instance_body → write_test_case with retry/repeat/afterEach lifecycles. This went through three review rounds with a substantive fix each time (Windows path snapshot, retry accumulation → afterEach clobber → boundary clear), which is a signal the state machine is subtle.
Other factors
- Four new targeted tests plus a strengthened retry assertion and a snapshot update; author confirmed all pass on every CI lane.
- The VM hook mirrors the existing GitHub Actions annotation / debugger lifecycle-reporter pattern at the same call site (
allow_side_effectsguard), so it's not a novel mechanism — but adding fields toVirtualMachineis the kind of surface a maintainer should ack. - The
test.concurrentsame-microtask interleaving is explicitly deferred; the fallback is the pre-PR bare<failure type="Error" />, so not a regression, but worth a maintainer nod that the follow-up scope is acceptable. - The
record_failure_cbunsafecast is sound (ctx is set to&mut JunitReporterfor the duration of one synchronousrun_error_handler, single-threaded, cleared afterward; the pointer targets theBoxheap soon_before_print's shared reporter borrow doesn't invalidate it), but it's the kind of raw-ptr threading a human should glance at.
|
@robobun fix clippy |
…from captured message
record_failure now strips CSI sequences from the error message before
storing it, so a matcher message built with colours does not surface as
bare [Nm residue in the report. When the stripped message begins with
'expect(' and the error name is the generic 'Error', the <failure> type
is reported as AssertionError.
Also splits the SAFETY comment in discard_junit_failure so each unsafe
block has its own (clippy::undocumented_unsafe_blocks).
![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 `@test/js/junit-reporter/junit.test.js`:
- Around line 552-554: Extend the JUnit report test around the existing ANSI
assertion with fixture messages containing parameterized SGR such as ESC[1;31m
and non-SGR CSI such as ESC[2K sequences. Assert the generated xmlContent
excludes their post-escape residue, while retaining the existing simple SGR
coverage.
🪄 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: f3215b39-ff36-4b2d-bd06-898d41101d6b
⛔ 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_command.rssrc/runtime/test_runner/Execution.rssrc/runtime/test_runner/bun_test.rstest/js/junit-reporter/junit.test.js
5d7ad8b to
a93ab8b
Compare
2 participants
What does this PR do?
Fixes three problems with the JUnit XML reporter that, together, made the report unparseable and stripped of any useful failure information.
Repro
1. Control characters in test names produce malformed XML
escape_xmlwrote&#N;for every C0 control byte but neither flushed the pending run nor advancedlast, so the raw byte was also emitted, and the references appeared at the start of the string instead of in place. Since bytes 0x00..=0x08 / 0x0B / 0x0C / 0x0E..=0x1F are not legal XML 1.0Chars even as numeric references, the fix passes TAB/LF/CR through and drops every other C0 byte.Before:
name="�ctrl ^@nul^[esc"(parse error).After:
name="ctrl nulesc"(parses).2.
classnameis double-escapedThe describe-scope names were XML-escaped while being joined with
" > ", then escaped again insidewrite_test_case, sodescribe('suite <a> & "b"')producedclassname="suite &lt;a&gt; &amp; &quot;b&quot;". The inner<testsuite name="">was only escaped once, so the two disagreed. The join now concatenates the raw names with a raw" > "separator and leaves the single escape towrite_test_case.3.
<failure>carries no message, stack, or correct typeEvery thrown error was reported as
<failure type="AssertionError" />with no message or stack.on_uncaught_exceptionnow records the error's name, message, and a colourlessprint_errorlike_objectrendering on theJunitReporter;write_test_caseemits them as thetypeandmessageattributes and the element body:TypeError,RangeError, etc. now report their real name intype. Timeouts also gainmessage="test timed out".How did you verify your code works?
New tests in
test/js/junit-reporter/junit.test.js:produces well-formed XML when test names contain control characters: asserts the raw bytes contain no illegal C0 characters, no�/, the report parses with a strict XML parser, and TAB/LF survive.escapes the classname attribute exactly once: asserts no&lt;/&amp;etc. appear and the decodedclassnameround-trips to the original describe titles.includes the error type, message and stack in <failure>: assertstype/messageand body text for a plainError, aTypeError, and anexpect().toBe()failure, and that no ANSI escapes leak into the report.All three fail against
mainand pass with this change. The existingjunit reportertests and the--parallel --reporter=junittests still pass.[review] gate passed · iteration 5 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 1 rejected · iteration 5
evidence per changed file