Skip to content

feat(libtest): Add JUnit formatter#84568

Merged
bors merged 3 commits into
rust-lang:masterfrom
andoriyu:libtest/junit_formatter
May 27, 2021
Merged

feat(libtest): Add JUnit formatter#84568
bors merged 3 commits into
rust-lang:masterfrom
andoriyu:libtest/junit_formatter

Conversation

@andoriyu

@andoriyu andoriyu commented Apr 25, 2021

Copy link
Copy Markdown
Contributor

tracking issue: #85563

Add an alternative formatter to libtest. Formatter produces valid xml that later can be interpreted as JUnit report.

Caveats:

  • timestamp is required by schema, but every viewer/parser ignores it. Attribute is not set to avoid depending on chrono;
  • Running all "suits" (unit tests, doc-tests and integration tests) will produce a mess;
  • I couldn't find a way to get integration test binary name, so it's just goes by "integration";

Sample output for unit tests (pretty printed by 3rd party tool):

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="test" package="test" id="0" errors="0" failures="0" tests="13" skipped="1">
    <testcase classname="results::tests" name="test_completed_bad" time="0"/>
    <testcase classname="results::tests" name="suite_started" time="0"/>
    <testcase classname="results::tests" name="suite_ended_ok" time="0"/>
    <testcase classname="results::tests" name="suite_ended_bad" time="0"/>
    <testcase classname="junit::tests" name="test_failed_output" time="0"/>
    <testcase classname="junit::tests" name="test_simple_output" time="0"/>
    <testcase classname="junit::tests" name="test_multiple_outputs" time="0"/>
    <testcase classname="results::tests" name="test_completed_ok" time="0"/>
    <testcase classname="results::tests" name="test_stared" time="0"/>
    <testcase classname="junit::tests" name="test_generate_xml_no_error_single_testsuite" time="0"/>
    <testcase classname="results::tests" name="test_simple_output" time="0"/>
    <testcase classname="test" name="should_panic" time="0"/>
    <system-out/>
    <system-err/>
  </testsuite>
</testsuites>

Sample output for integration tests (pretty printed by 3rd party tool):

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="test" package="test" id="0" errors="0" failures="0" tests="1" skipped="0">
    <testcase classname="integration" name="test_add" time="0"/>
    <system-out/>
    <system-err/>
  </testsuite>
</testsuites>

Sample output for Doc-tests (pretty printed by 3rd party tool):

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="test" package="test" id="0" errors="0" failures="0" tests="1" skipped="0">
    <testcase classname="src/lib.rs" name="(line 2)" time="0"/>
    <system-out/>
    <system-err/>
  </testsuite>
</testsuites>

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2021
Comment thread library/test/src/formatters/junit.rs Outdated
@jyn514 jyn514 added A-libtest Area: `#[test]` / the `test` library T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 26, 2021
Comment thread library/test/src/formatters/junit.rs Outdated
@yaahc

yaahc commented Apr 29, 2021

Copy link
Copy Markdown
Member

Hey, just wanted to drop in to let you know that I've done an initial review and this looks good so far. Based on the comments it looks like you have some additional changes still planned so I'll hold off on approving until you've made the changes you mentioned.

@yaahc yaahc added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2021
@Mark-Simulacrum

Copy link
Copy Markdown
Member

Hm, I have some mild reservations about supporting junit (and other more specialized frameworks). We already have a json backend, and it seems likely that could be mapped to junit with an external crate if desired. That does hurt usability somewhat, but I'm not sure it makes sense to support all possible test output formats in-tree either.

Can you perhaps share what is expected to consume the junit output? Or what the motivation is?

@andoriyu

andoriyu commented May 1, 2021

Copy link
Copy Markdown
Contributor Author

Hm, I have some mild reservations about supporting junit (and other more specialized frameworks). We already have a json backend, and it seems likely that could be mapped to junit with an external crate if desired. That does hurt usability somewhat, but I'm not sure it makes sense to support all possible test output formats in-tree either.

Can you perhaps share what is expected to consume the junit output? Or what the motivation is?

Well, JUnit is universally supported format: a lot of test harnesses/runners support JUnit as an output format, many Continuous Integration platforms support consuming it. I think CI would be a common use case to use JUnit as output. There is no point on supporting all other formats, like you said, because JUnit format is the least common denominator across all of them.

As for JSON backend goes, well, it's not stabilized and not documented. You can map it to JUnit, like you said, I did that in cargo-suity. In my opinion, this is a task for test harness, rather than 3rd party library.

Personally, I would prefer if I could keep console output, and write JUnit report into a file in parallel.

@tschuett

tschuett commented May 1, 2021

Copy link
Copy Markdown

Hm, I have some mild reservations about supporting junit (and other more specialized frameworks). We already have a json backend, and it seems likely that could be mapped to junit with an external crate if desired. That does hurt usability somewhat, but I'm not sure it makes sense to support all possible test output formats in-tree either.

Can you perhaps share what is expected to consume the junit output? Or what the motivation is?

As @andoriyu said most CI systems can consume JUnit reports. Of course you can transform json output into anything, but it seems to indirect to me.

@andoriyu andoriyu requested a review from jyn514 May 13, 2021 23:26
@jyn514

jyn514 commented May 14, 2021

Copy link
Copy Markdown
Member

I am not an appropriate reviewer for this PR, please don't request reviews from me.

@andoriyu

Copy link
Copy Markdown
Contributor Author

I am not an appropriate reviewer for this PR, please don't request reviews from me.

Oh, you just left some comment in the past. My bad.

Comment thread library/test/src/formatters/junit.rs
@yaahc

yaahc commented May 21, 2021

Copy link
Copy Markdown
Member

Hm, I have some mild reservations about supporting junit (and other more specialized frameworks). We already have a json backend, and it seems likely that could be mapped to junit with an external crate if desired. That does hurt usability somewhat, but I'm not sure it makes sense to support all possible test output formats in-tree either.

Can you perhaps share what is expected to consume the junit output? Or what the motivation is?

@Mark-Simulacrum do you still feel reservations given @andoriyu's response?

@Mark-Simulacrum

Mark-Simulacrum commented May 21, 2021

Copy link
Copy Markdown
Member

Mostly resolved, yeah. I think we should get a tracking issue filed, particularly given the presence of several outstanding defects (as noted in the PR description). It's not clear to me what the path to stabilization for these custom test outputs would be, but it needn't block unstable addition.

@tschuett

Copy link
Copy Markdown

Mostly resolved, yeah. I think we should get a tracking issue filed, particularly given the presence of several outstanding defects (as noted in the PR description). It's not clear to me what the path to stabilization for these custom test outputs would be, but it needn't block unstable addition.

I don't believe that junit is custom at all. Allmost all CIs have support for junit, e.g.:
https://plugins.jenkins.io/junit/:

@yaahc

yaahc commented May 21, 2021

Copy link
Copy Markdown
Member

Mostly resolved, yeah. I think we should get a tracking issue filed, particularly given the presence of several outstanding defects (as noted in the PR description). It's not clear to me what the path to stabilization for these custom test outputs would be, but it needn't block unstable addition.

Sounds good. @andoriyu, can you go ahead and create a tracking issue? Once that's setup I think this PR is good to go.

@andoriyu

Copy link
Copy Markdown
Contributor Author

Mostly resolved, yeah. I think we should get a tracking issue filed, particularly given the presence of several outstanding defects (as noted in the PR description). It's not clear to me what the path to stabilization for these custom test outputs would be, but it needn't block unstable addition.

Sounds good. @andoriyu, can you go ahead and create a tracking issue? Once that's setup I think this PR is good to go.

@yaahc Done - #85563. I've never filled tracking issues before, if I made any mistakes please let me know.

@yaahc

yaahc commented May 25, 2021

Copy link
Copy Markdown
Member

Looks perfect @andoriyu. I went ahead and edited your original post in this PR to add a link to the tracking issue. Thanks again for all the work!

@bors r+

@bors

bors commented May 25, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 9f83e22 has been approved by yaahc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 25, 2021
@bors

bors commented May 25, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 9f83e22 with merge 90f23ae3440a957bc9de51e4e2e2a99330f875ac...

@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
      Memory: 14 GB
      Boot ROM Version: VMW71.00V.13989454.B64.1906190538
      Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
      SMC Version (system): 2.8f0
      Serial Number (system): VMLFRk6HhKKs

hw.ncpu: 3
hw.byteorder: 1234
hw.memsize: 15032385536

@bors

bors commented May 25, 2021

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 25, 2021
@andoriyu

Copy link
Copy Markdown
Contributor Author

@yaahc looks like crates.io was down briefly?

@yaahc

yaahc commented May 27, 2021

Copy link
Copy Markdown
Member

@yaahc looks like crates.io was down briefly?

oo, sorry about that

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2021
name=\"{}\" time=\"{}\">",
class_name,
test_name,
duration.as_secs()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if the output supports subsecond precision for the duration. This is what cargo2junit does. That way you don't loose the runtime information for fast tests.

@bors

bors commented May 27, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 9f83e22 with merge 1c6868a...

@bors

bors commented May 27, 2021

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing 1c6868a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2021
@bors bors merged commit 1c6868a into rust-lang:master May 27, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 27, 2021
@klensy

klensy commented May 28, 2021

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-libtest Area: `#[test]` / the `test` library merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.