-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libtest: include test output in junit xml reports #110651
Conversation
I'm about to make some changes here, and it was making me uneasy to modify the output format without test coverage.
By placing the stdout in a CDATA block we avoid almost all escaping, as there's only two byte sequences you can't sneak into a CDATA and you can handle that with some only slightly regrettable CDATA-splitting. I've done this in at least two other implementations of the junit xml format over the years and it's always worked out. The only quirk new to this (for me) is smuggling newlines as 
 to avoid literal newlines in the output.
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
@bors r+ |
libtest: include test output in junit xml reports Fixes rust-lang#110336.
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#108865 (Add a `sysroot` crate to represent the standard library crates) - rust-lang#110651 (libtest: include test output in junit xml reports) - rust-lang#110826 (Make PlaceMention a non-mutating use.) - rust-lang#110982 (Do not recurse into const generic args when resolving self lifetime elision.) - rust-lang#111009 (Add `ascii::Char` (ACP#179)) - rust-lang#111100 (check array type of repeat exprs is wf) - rust-lang#111186 (Add `is_positive` method for signed non-zero integers.) - rust-lang#111201 (bootstrap: add .gitmodules to the sources) Failed merges: - rust-lang#110954 (Reject borrows of projections in ConstProp.) r? `@ghost` `@rustbot` modify labels: rollup
let escaped_output = s.replace("]]>", "]]]]><![CDATA[>"); | ||
let escaped_output = escaped_output.replace("<?", "<]]><![CDATA[?"); | ||
// We also smuggle newlines as 
 so as to keep all the output on one line | ||
let escaped_output = escaped_output.replace("\n", "]]>
<![CDATA["); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler already pulls in aho-corasick, but the test library does not. Failing test stdout could easily balloon to gigantic sizes -- should I write a PR to swap these replaces to use aho-corasick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so -- it's a higher bar to add dependencies to the standard library, including test
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussion on Zulip: Let's wait until someone does a perf measurement and finds this to blame.
Fixes #110336.