Libtest generated junit report includes extra newline at the start.

[last-partizan]

rust/library/test/src/formatters/junit.rs

Line 36 in 24ae996

self.out.write_all(b"\n")?;

Hello, is there a reason to include this newline at the start?

@yaahc i'm tagging you, looks like it was your commit.

7779eb7

I'm trying to use junit format in github actions, and with this line, generated output could not be parsed by python junitparser. Sure it could be fixed there, but i don't think of any particular reason why this newline is needed.

cargo test -- -Z unstable-options --format junit > results.xml
...
2022-01-29 10:26:06 +0000 - publish-unit-test-results -  INFO - reading results.xml
2022-01-29 10:26:06 +0000 - publish - ERROR - Error processing result file: XML or text declaration not at start of entity: line 2, column 0

Also, those ending newlines isn't neccessary (but hopefully they aren't breaking anything).

Or maybe I doing something wrong here?

[yaahc]

Looking back at #89235 I think the newline was added to make the output consistent with other test output formats when dumped via cargo. I think the issue here being that cargo expects the extra newlines to be generated by libtest, but it may be that we need to move that around so that libtest only outputs the test report with no extra whitespace and cargo handles padding it appropriately for a nice cohesive report.

I think it's fine if we remove the newlines here but I think we will instead want to update cargo to insert the newlines, make sure that the output you can capture via redirect doesn't include those newlines, and then make sure all the other test outputs don't regress as a result, probably by updating them as well.

[last-partizan]

I think, newlines here actually aren't necessary.

In #89235 you're comparing machine-readable junit to human-readable output, and if you compare it to json - json has not extra padding.

Of course, we can remove newlines here and add them in cargo, but in my opinion this adds no real value and adds some complexity, sure it looks nice without redirection, but output is supposed to be redirected, and if redirected, it will look like this:

> cargo test -- -Z unstable-options --format junit > /tmp/j.xml
    Finished test [unoptimized + debuginfo] target(s) in 0.00s
     Running unittests (target/debug/deps/rust_tests-ef3ded103bf341f0)

You've added ~E-help-wanted, and i can make this small change. I think, instead of reverting commit, i should remove starting newline, and leave just one ending newline.

Is there some example project with doctests so i can test how it would look?

[yaahc]

I think, newlines here actually aren't necessary.

In #89235 you're comparing machine-readable junit to human-readable output, and if you compare it to json - json has not extra padding.

Of course, we can remove newlines here and add them in cargo, but in my opinion this adds no real value and adds some complexity, sure it looks nice without redirection, but output is supposed to be redirected, and if redirected, it will look like this:

Sounds reasonable. 👍

You've added ~E-help-wanted, and i can make this small change. I think, instead of reverting commit, i should remove starting newline, and leave just one ending newline.

Is there some example project with doctests so i can test how it would look?

Oh, lovely! As for example doctests, If you wanted to setup a permanent / repeatable test the best place for that would probably be in library/test. If you just want to verify the output real quick manually I would setup a custom toolchain with rustup and just make a random cargo new --lib project and set it to use your custom toolchain with something like rustup override set stage1.

[last-partizan]

"real quick" for me taking longer than expected :)

I've followed this guide: https://rustc-dev-guide.rust-lang.org/getting-started.html

At the ./x.py setup i selected library, and then

./x.py build --stage 1 library/test
rustup toolchain link stage1 build/x86_64-unknown-linux-gnu/stage1

Then in my rust-example-lib i'm running rustup override set stage1, and cargo test -- -Z unstable-options --format junit

But output doesn't seem to be affected by my changes to junit formatter.

Am i missing something?

[yaahc]

That all sounds correct. It might be that you're selecting too small of a subset of the build to properly use as an override. Does it work if you run a full build with ./x.py build?

Also, incase it helps the zulip might be a better venue for getting help debugging your issues: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs

[last-partizan]

I managed to build and test it, and it is working as expected. Thanks for help!

But, junit reporter is still can be used in very limited cases: only when there is one type of tests, becouse, when there is unit/integration and doctests, every category (and every file in tests/) produces new xml. And with current architecture i don't see a way to fix this.

Luckily, using json formatter with cargo2junit gives great result, as it can take output from all test runs and compile it into single file.

To achive the same result with libtest/cargo-test we need a way to write header at the run start, and by "run" i mean "running all tests", and to write footer after all tests are completed.

[yaahc]

That's unfortunate. Does the json output that goes into cargo2junit have the same issue where it is outputting multiple smaller bits of json that the cargo subcommand then has to combine and translate into junit? If so I imagine the solution for cargo-test would look pretty similar except the input would be junit.

[last-partizan]

json output does not have this issue, becouse it does not have "header" and "footer", it just prints single events line by line as json, like this:

{ "type": "suite", "event": "started", "test_count": 1 }
{ "type": "test", "event": "started", "name": "tests::it_works" }
{ "type": "test", "name": "tests::it_works", "event": "ok" }
{ "type": "suite", "event": "ok", "passed": 1, "failed": 0, "allowed_fail": 0, "ignored": 0, "measured": 0, "filtered_out": 0, "exec_time": 0.000127229 }
{ "type": "suite", "event": "started", "test_count": 1 }
{ "type": "test", "event": "started", "name": "tests::it_works" }
{ "type": "test", "name": "tests::it_works", "event": "ok" }
{ "type": "suite", "event": "ok", "passed": 1, "failed": 0, "allowed_fail": 0, "ignored": 0, "measured": 0, "filtered_out": 0, "exec_time": 0.000227738 }
{ "type": "suite", "event": "started", "test_count": 2 }
{ "type": "test", "event": "started", "name": "tests::it_fails" }
{ "type": "test", "event": "started", "name": "tests::it_works" }
{ "type": "test", "name": "tests::it_works", "event": "ok" }
{ "type": "test", "name": "tests::it_fails", "event": "failed", "stdout": "thread 'tests::it_fails' panicked at 'assertion failed: `(left == right)`\n  left: `4`,\n right: `5`', tests/test_something.rs:12:9\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace\n" }
{ "type": "suite", "event": "failed", "passed": 1, "failed": 1, "allowed_fail": 0, "ignored": 0, "measured": 0, "filtered_out": 0, "exec_time": 0.000585028 }

And then, cargo2json converts it into actual junit/xml based on those events.

I think, yes, in theory we can let cargo-test command take those parts of xml and combine it into actual xml, but i think this is bad idea, becouse that would be splitting one code between two projects.

I will think about it.