Repository navigation
fix(reporters): junit to strip ANSI sequences from all XML attributes - #11407
Conversation
escapeXML removed only the ESC byte, so attribute values such as the failure message kept the rest of each sequence (e.g. "[36m"). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
AriPerkkio
left a comment
There was a problem hiding this comment.
Should JUnit reporter escape these? It seems there's no standarized answer for this.
🤖 says:
ANSI escape handling in JUnit reporters
Legend: ✅ the whole ANSI sequence is removed, ❌ it is not (something is left in the XML), — the reporter does not write that field.
Reporter testcasename/classnamefailuremessagefailurebodysystem-out/system-errWhat ❌ fields contain Vitest (main) ❌ ❌ ✅ ❌ [31mcargo-nextest ✅ ✅ ✅ ✅ Mocha built-in xunit✅ — ✅ — Playwright (default) ❌ ✅ ✅ ❌ [31mPlaywright ( stripANSIControlSequences)✅ ✅ ✅ ✅ jest-junit ❌ — ✅ ❌ see notes mocha-junit-reporter ✅ ❌ ❌ ✅ [31mPHPUnit ❌ — ❌ ❌ [31mpytest ❌ ❌ ❌ ❌ #x1B[31mMaven Surefire ❌ ❌ ❌ ❌ &#27;[31mgo-junit-report ❌ — ❌ ❌ �[31m(U+FFFD)rspec_junit_formatter ❌ ❌ ❌ ❌ Ruby-style escape + [31m
We should probably add a reporters: [['junit', { stripAnsi: boolean }]] option for this. As we are already removing ANSI codes from body in writeErrorElement, it might be tricky to decide what the default value is. Maybe the type should be stripAnsi: true | false | 'error-bodies-only', and the default would be 'error-bodies-only'. 🤔
|
@AriPerkkio Thanks. I read #4958 and #5007, where always stripping was declined. I think the reasons for stripping everything are different now, so I want to suggest it once more before I add an option.
So I suggest keeping this PR as it is: strip ANSI everywhere. I will also update the description, because it lists only some of the places this change affects. If you still want an option, how about using the Playwright default as our default? It strips only the parts built from the error: the |
AriPerkkio
left a comment
There was a problem hiding this comment.
So
removeInvalidXMLCharactersalways removes ESC.
Ohh I wasn't aware of that. In that case, it's not even possible to preserve colors in these reports (like it was thought in #5007 (comment)).
Removing all ANSI escapes in escapeXML sounds good to me. It's bug fix, not breaking change in my opinion. @hi-ogawa thoughts?
Description
The JUnit reporter strips ANSI sequences from the
<failure>body, but not from attribute values.escapeXMLremoves the ESC byte because XML cannot hold it, but the rest of each sequence stays. An error message that has colors (for example, the DOM dump that Testing Library adds by default) gives:CI tools that show the
messageattribute then print[36mas text.This PR strips ANSI sequences in
escapeXML. Themessageattribute, thenameof unhandled-error test cases, andsystem-out/system-errnow get the same treatment as the<failure>body.AI disclosure: I used Claude Code to investigate and write this change.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Locally, 8 tests fail, and the same 8 fail on
mainwithout this change.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.🤖 Generated with Claude Code