-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
fix(cli): output more detailed information for steps when using JUnit reporter #22797
fix(cli): output more detailed information for steps when using JUnit reporter #22797
Conversation
@@ -268,43 +268,77 @@ pub enum TestFailure { | |||
HasSanitizersAndOverlaps(IndexSet<String>), // Long names of overlapped tests | |||
} | |||
|
|||
impl ToString for TestFailure { | |||
fn to_string(&self) -> String { | |||
impl std::fmt::Display for TestFailure { |
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.
For reviewers: this is just a refactor; ToString
trait is automatically implemented for all types that implement std::fmt::Display
, so basically it's recommended to implement std::fmt::Display
683614a
to
145f2a1
Compare
I agree, this is a reasonable trade-off. |
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.
Great improvement. I have a couple of requests, but overall LGTM.
Pinging @littledivy for a second pair of eyes and to ensure npm_smoke_tests
repo won't suddenly break because of this change.
cli/tools/test/reporters/junit.rs
Outdated
Self { | ||
path, | ||
cwd: Url::from_directory_path(std::env::current_dir().unwrap()).unwrap(), |
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.
This will eventually fail and panic for user; can you pass down cwd
from CliOptions
so it's handled gracefully?
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 brought this from the existing code of PrettyReporter:
deno/cli/tools/test/reporters/pretty.rs
Line 40 in b6e44f9
cwd: Url::from_directory_path(std::env::current_dir().unwrap()).unwrap(), |
Should we also fix these ones as well?
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.
Yes, I think it would be better to fix both then
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.
Sure! I noticed that all the test reporters have this code, so I'll fix all of them
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.
OK, it's done 752db03
cli/tools/test/reporters/junit.rs
Outdated
|
||
impl TestNameTree { | ||
fn new() -> Self { | ||
Self(IndexMap::new()) |
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.
Maybe pre-allocate to something like 256 to avoid excessive reallocations?
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.
Good point! fixed in 14d89d2
4d5ae5a
to
752db03
Compare
This patch gets JUnit reporter to output more detailed information for test steps (subtests).
Issue with previous implementation
In the previous implementation, the test hierarchy was represented using several XML tags like the following:
<testsuites>
corresponds to the entire test (one execution ofdeno test
has exactly one<testsuites>
tag)<testsuite>
corresponds to one file, such asmain_test.ts
<testcase>
corresponds to oneDeno.test(...)
<property>
corresponds to onet.step(...)
This structure describes the test layers but one problem is that
<property>
tag is used for any use cases so some tools that can ingest a JUnit XML file might not be able to interpret<property>
as subtests.How other tools address it
Some of the testing frameworks in the ecosystem address this issue by fitting subtests into the
<testcase>
layer. For instance, take a look at the following Go test file:Running gotestsum, we can get the output like this:
This output shows that nested test cases are squashed into the
<testcase>
layer by treating them as the same layer as their parent,TestMain
. We can still distinguish nested ones by theirname
attributes that look likeTestMain/<subtest_name>
.As described in #22795, vitest solves the issue in the same way as gotestsum.
One downside of this would be that one test failure that happens in a nested test case will end up being counted multiple times, because not only the subtest but also its wrapping container(s) are considered to be failures. In fact, in the gotestsum output above,
TestMain/child_2
failed (which is totally expected) while its parent,TestMain
, was also counted as failure. As #20273 (comment) pointed out, there is a test runner that offers flexibility to prevent this, but I personally don't think the "duplicate failure count" issue is a big deal.How to fix the issue in this patch
This patch fixes the issue with the same approach as gotestsum and vitest.
More specifically, nested test cases are put into the
<testcase>
level and their names are now represented as squashed test names concatenated by>
(e.g.parent 2 > child 1 > grandchild 1
). This change also allows us to put a detailed error message as<failure>
tag within the<testcase>
tag, which should be handled nicely by third-party tools supporting JUnit XML.Extra fix
Also, file paths embedded into XML outputs are changed from absolute path to relative path, which is helpful when running the test suites in several different environments like CI.
Resolves #22795