[llvm][llvm-lit] Add total time for each testsuite in JUnit XML output

[DavidSpickett]

Currently we write out a time taken to run all test suites:

<testsuites time="8.28">

And one for each test:

<testcase classname="lldb-shell.Breakpoint" name="breakpoint-command.test" time="2.38"/>

However, the schema says there should be one for each suite and test, but none for testsuites:
https://github.com/windyroad/JUnit-Schema/blob/cfa434d4b8e102a8f55b8727b552a0063ee9044e/JUnit.xsd#L216

I'm leaving the testsuites time in though because no one has complained so far, and someone out there probably has a script relying on it by now. Most XML tools handle unknown attributes quite well anyway.

I'm adding a per testsuite time to comply with the schema and maybe be more compatible with other JUnit tools.

<testsuite name="lldb-shell" ... time="12.34">

The test suite time is the sum of the time taken for all tests in the suite. This will ignore some overhead in setting up the suite, and means that the sum of the times for all individual suites may not equal the testsuites time.

As we're usually focusing on the execution time of particular tests, not lit's book keeping, I think this is a reasonable choice.

[llvmbot]

@llvm/pr-subscribers-testing-tools

Author: David Spickett (DavidSpickett)

Changes

Currently we write out a time taken to run all test suites:

<testsuites time="8.28">

And one for each test:

<testcase classname="lldb-shell.Breakpoint" name="breakpoint-command.test" time="2.38"/>

However, the schema says there should be one for each suite and test, but none for testsuites:
https://github.com/windyroad/JUnit-Schema/blob/cfa434d4b8e102a8f55b8727b552a0063ee9044e/JUnit.xsd#L216

I'm leaving the testsuites time in though because no one has complained so far, and someone out there probably has a script relying on it by now. Most XML tools handle unknown attributes quite well anyway.

I'm adding a per testsuite time to comply with the schema and maybe be more compatible with other JUnit tools.

<testsuite name="lldb-shell" ... time="12.34">

The test suite time is the sum of the time taken for all tests in the suite. This will ignore some overhead in setting up the suite, and means that the sum of the times for all individual suites may not equal the testsuites time.

As we're usually focusing on the execution time of particular tests, not lit's book keeping, I think this is a reasonable choice.

---

Full diff: https://github.com/llvm/llvm-project/pull/112230.diff

3 Files Affected:

• (modified) llvm/utils/lit/lit/reports.py (+11-3)
• (modified) llvm/utils/lit/tests/shtest-format.py (+1-1)
• (modified) llvm/utils/lit/tests/xunit-output.py (+1-1)

diff --git a/llvm/utils/lit/lit/reports.py b/llvm/utils/lit/lit/reports.py
index 2ac44b0c0ce86e..d2d719b076bc70 100755
--- a/llvm/utils/lit/lit/reports.py
+++ b/llvm/utils/lit/lit/reports.py
@@ -105,12 +105,20 @@ def write_results(self, tests, elapsed):
             file.write("</testsuites>\n")
 
     def _write_testsuite(self, file, suite, tests):
-        skipped = sum(1 for t in tests if t.result.code in self.skipped_codes)
-        failures = sum(1 for t in tests if t.isFailure())
+        skipped = 0
+        failures = 0
+        time = 0.0
+
+        for t in tests:
+            if t.result.code in self.skipped_codes:
+                skipped += 1
+            if t.isFailure():
+                failures += 1
+            time += t.result.elapsed
 
         name = suite.config.name.replace(".", "-")
         file.write(
-            f'<testsuite name={quo(name)} tests="{len(tests)}" failures="{failures}" skipped="{skipped}">\n'
+            f'<testsuite name={quo(name)} tests="{len(tests)}" failures="{failures}" skipped="{skipped}" time="{time:.2f}">\n'
         )
         for test in tests:
             self._write_test(file, test, name)
diff --git a/llvm/utils/lit/tests/shtest-format.py b/llvm/utils/lit/tests/shtest-format.py
index 4a3d65b7bce4fd..3a1959549e5d01 100644
--- a/llvm/utils/lit/tests/shtest-format.py
+++ b/llvm/utils/lit/tests/shtest-format.py
@@ -107,7 +107,7 @@
 
 # XUNIT: <?xml version="1.0" encoding="UTF-8"?>
 # XUNIT-NEXT: <testsuites time="{{[0-9.]+}}">
-# XUNIT-NEXT: <testsuite name="shtest-format" tests="22" failures="8" skipped="3">
+# XUNIT-NEXT: <testsuite name="shtest-format" tests="22" failures="8" skipped="3" time="{{[0-9.]+}}">
 
 # XUNIT: <testcase classname="shtest-format.external_shell" name="fail.txt" time="{{[0-9]+\.[0-9]+}}">
 # XUNIT-NEXT: <failure{{[ ]*}}>
diff --git a/llvm/utils/lit/tests/xunit-output.py b/llvm/utils/lit/tests/xunit-output.py
index 67d99849fe36d9..392cded4653fe1 100644
--- a/llvm/utils/lit/tests/xunit-output.py
+++ b/llvm/utils/lit/tests/xunit-output.py
@@ -9,7 +9,7 @@
 
 # CHECK:      <?xml version="1.0" encoding="UTF-8"?>
 # CHECK-NEXT: <testsuites time="{{[0-9.]+}}">
-# CHECK-NEXT: <testsuite name="test-data" tests="5" failures="1" skipped="3">
+# CHECK-NEXT: <testsuite name="test-data" tests="5" failures="1" skipped="3" time="{{[0-9.]+}}">
 # CHECK-NEXT: <testcase classname="test-data.test-data" name="bad&amp;name.ini" time="{{[0-1]\.[0-9]+}}">
 # CHECK-NEXT:   <failure><![CDATA[& < > ]]]]><![CDATA[> &"]]></failure>
 # CHECK-NEXT: </testcase>

[pogo59]

There are other "required" elements in that schema that we don't emit; should we be emitting them? Or are you just particularly interested in the per-testsuite statistic?

[DavidSpickett]

For context I'm working on merging result files so we can report all tests in one go at the end of the Buildkite CI runs that build PRs.

There are other "required" elements in that schema that we don't emit; should we be emitting them?

I will see what else is missing that is low friction to add. I suspect a lot of junit consumers are tolerant to missing fields.

Or are you just particularly interested in the per-testsuite statistic?

I was using https://pypi.org/project/junitparser/ to merge result files and I noticed that the testsuites time= was not the sum of the input files. Which made sense given it's not in the schema, so I went looking for where the times are expected to be.

I'm not fixing a specific use case with a specific junit consumer, just speculating that some tools out there may do a better job with this time field added.

It may also be useful for manual analysis every so often, instead of having to load the file in Python and sum all the times.

[DavidSpickett]

I will see what else is missing that is low friction to add. I suspect a lot of junit consumers are tolerant to missing fields.

I used a validator with the schema and found the following differences (aside from the testuite time I'm already adding here):

• We shouldn't be adding time to <testsuites>, but I don't see a reason to remove it. It is useful to have a total time to report.
• We do not add id to <testsuite>. This is an incrementing number from 0 that as far as I can tell, isn't that important. For example common tools to merge result files don't renumber this.
• We do not add <system-out> and <system-err> after each <testcase>. I think we bypass this by putting the output in the failure if the test does fail. We could improve this but it's not critical as long as failures are described.
• We do not add <properties> after <testsuite>. This is any environment variables and the like. We could try to add this but I think most people would look to the build system logs to find these anyway.
• We do not add timestamp to each <testsuite>, which is when the suite began. Could add, but not that useful compared to time taken.
• We do not add hostname to each <testsuite>. Lit is probably always on one host so this is not needed.
• We do not add package to each <testsuite>. This feels like a Java thing "Full class name of the test for non-aggregated testsuite documents.". I'm not sure what that would even be in our context.
• We do not add errors to testsuite. This is because JUnit draws a distinction between a failure and an error, an error might be more akin to lit's UNRESOLVED. However lit already considers UNRESOLVED to be a failure and that seems like a breaking change if we did that.

I could try to add <testsuite timestamp= but the value of that is low, especially for my use cases. The rest are not relevant to LLVM or risk being breaking changes.

[pogo59]

Thanks for reviewing the schema and giving a detailed rationale. This change LGTM.