[lit] [PATCH 2/2] Add "--reduced-xunit-report" option

[gbedwell]

Summary

This option is designed to save on disk space in cases where a CI system
is running dozens of builds each day and is using the information in the
XUnit report to display information about the tests. Due to the sheer
number of tests in the LLVM repo each XUnit report is multiple MB in
size and when multiplied by large numbers of builds these reports can
consume swathes of disk space when each report is archived by CI unless
an aggressive expiration strategy is in place, meaning that historical
data about failures is lost.

In general, the most interesting results are those were a test has
failed (UNRESOLVED/TIMEOUT/FAIL/XPASS) so this new option limits the
report to only include these tests. This comes at the expense of
continuity of test data (a new failure will show as a new test for
example, and no duration data will be recorded for passing tests), but
for some users this may be a trade-off worth making.

I'm not sure whether this would be considered controversial or not as it somewhat breaks the xunit format (although only when the new flag is enabled), so if we ultimately decide not to go with this it's okay. We would definitely find it useful internally though. I've tested it with Jenkins' xunit reporting and it works as I'd expect. I can see all the failures in any given build - just not the passes, and therefore the history isn't as useful. It's an absolute fraction of the size though. This is a trade-off that seems worth it for us.

[yln]

Initially, my main question was: would this functionality make sense for reports other than the xml one too? Probably not, because the use case is pretty specific, right? Anyways, I don't think we need to implement that now and leave that work to whoever needs it.

• 3 small nits
• Is it possible to write a test for this?

I am okay with the overall motivation considering it's gated behind a flag.

@jdenny: Comments on the larger trade-off of having this feature?

llvm/utils/lit/lit/cl_arguments.py

121

Would something like "report/output failures only" be a more precise flag name?

122

I would prefer to omit the dest=... parameter and let ArgParse pick a Python-idiomatic field name.

llvm/utils/lit/lit/main.py

121

Please pass opts.reduced_report instead of the entire options object here.

[yln]

llvm/utils/lit/lit/cl_arguments.py

119

Circling back after reviewing part 1: Can we find a way to pass in the boolean in the constructor for XunitReport to avoid having to extend all write_results() functions for the reports that don't care/support the option?

[jdenny]

@gbedwell, thanks for working on this. I have also noticed that gitlab CI's web UI for browsing test results can be tough to navigate for LLVM because of the massive number of tests. This could help.

As you mentioned in the review summary, it is misleading that non-failing tests appear not to have run at all. I'm imagining a manager or some external person looking at CI results and thinking nothing ever passes. To address that issue, one possibility is to add a fake test result per non-failing test code that perhaps sums up its number of tests and elapsed times. I'm not requesting this improvement before this lands. It's just a thought.

In D143519#4111328, @yln wrote:

Initially, my main question was: would this functionality make sense for reports other than the xml one too?

It might. For lit's own test suite, I noticed the --time-trace-output output is about twice as big as the --xunit-xml-output output when all tests pass. Perhaps the filter can just be moved up a level where write_results is called. It seems like that would also shrink this patch.

@jdenny: Comments on the larger trade-off of having this feature?

Thanks for pinging me on this. Especially if the implementation is moved out of the individual reports, it's a small amount of code, and I can see how it would be useful.

llvm/utils/lit/lit/reports.py

22–23

This list is already in lit/Test.py. It can be accessed with something like test.result.code.isFailure.

[gbedwell]

Thanks all for taking a look! I haven't had a chance to get back to this yet due to other work landing on my plate, but I intend to revisit this and implement the requested changes ASAP.

llvm/utils/lit/lit/cl_arguments.py

def parse_args():
     execution_group.add_argument("--time-tests",
             help="Track elapsed wall time for each test",
             action="store_true")
     execution_group.add_argument("--no-execute",
             dest="noExecute",
             help="Don't execute any tests (assume PASS)",
             action="store_true")
     execution_group.add_argument("--xunit-xml-output",
             type=lit.reports.XunitReport,
             help="Write XUnit-compatible XML test reports to the specified file")
+    execution_group.add_argument("--reduced-xunit-report",
+            dest="reducedXUnitReport",
+            help="When writing an XUNit test report, do not include results for "
+                 "tests that completed successfully or were not run",
+            action="store_true")
     execution_group.add_argument("--resultdb-output",
             type=lit.reports.ResultDBReport,
             help="Write LuCI ResuldDB compatible JSON to the specified file")
     execution_group.add_argument("--time-trace-output",
             type=lit.reports.TimeTraceReport,
             help="Write Chrome tracing compatible JSON to the specified file")
     execution_group.add_argument("--timeout",
             dest="maxIndividualTestTime",

llvm/utils/lit/lit/main.py

def main(builtin_params={}):
 
     if opts.time_tests:
         print_histogram(discovered_tests)
 
     print_results(discovered_tests, elapsed, opts)
 
     tests_for_report = selected_tests if opts.shard else discovered_tests
     for report in opts.reports:
-        report.write_results(tests_for_report, elapsed)
+        report.write_results(tests_for_report, elapsed, opts)
 
     if lit_config.numErrors:
         sys.stderr.write('\n%d error(s) in tests\n' % lit_config.numErrors)
         sys.exit(2)
 
     if lit_config.numWarnings:
         sys.stderr.write('\n%d warning(s) in tests\n' % lit_config.numWarnings)
 

llvm/utils/lit/lit/reports.py

def by_suite_and_test_path(test):
     return (test.suite.name, id(test.suite), test.path_in_suite)
 
 
 class _ReportBase(abc.ABC):
     def __init__(self, output_file):
         self.output_file = output_file
         self.skipped_codes = {lit.Test.EXCLUDED,
                               lit.Test.SKIPPED, lit.Test.UNSUPPORTED}
+        self.failed_codes = {lit.Test.UNRESOLVED, lit.Test.TIMEOUT,
+                             lit.Test.FAIL, lit.Test.XPASS}
 
     @abc.abstractmethod
-    def write_results(self, tests, elapsed):
+    def write_results(self, tests, elapsed, opts):
         ...
 
 
 class JsonReport(_ReportBase):
-    def write_results(self, tests, elapsed):
+    def write_results(self, tests, elapsed, opts):
+        del opts
         unexecuted_codes = {lit.Test.EXCLUDED, lit.Test.SKIPPED}
         tests = [t for t in tests if t.result.code not in unexecuted_codes]
         # Construct the data we will write.
         data = {}
         # Encode the current lit version as a schema version.
         data['__version__'] = lit.__versioninfo__
         data['elapsed'] = elapsed
         # FIXME: Record some information on the lit configuration used?
def remove_invalid_xml_chars(s):
     # removes them to produce a valid XML document.
     #
     # Note: In XML 1.1 only \0 is illegal (https://www.w3.org/TR/xml11/#charsets)
     # but lit currently produces XML 1.0 output.
     return s.translate(_invalid_xml_chars_dict)
 
 
 class XunitReport(_ReportBase):
-    def write_results(self, tests, elapsed):
+    def write_results(self, tests, elapsed, opts):
+
+        reduced_report = opts.reducedXUnitReport
         tests.sort(key=by_suite_and_test_path)
         tests_by_suite = itertools.groupby(tests, lambda t: t.suite)
 
         with open(self.output_file, 'w') as file:
             file.write('<?xml version="1.0" encoding="UTF-8"?>\n')
             file.write('<testsuites time="{time:.2f}">\n'.format(time=elapsed))
             for suite, test_iter in tests_by_suite:
-                self._write_testsuite(file, suite, list(test_iter))
+                self._write_testsuite(file, suite, list(test_iter), reduced_report)
             file.write('</testsuites>\n')
 
-    def _write_testsuite(self, file, suite, tests):
+    def _write_testsuite(self, file, suite, tests, reduced_report):
         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())
 
         name = suite.config.name.replace('.', '-')
         file.write(f'<testsuite name={quo(name)} tests="{len(tests)}" failures="{failures}" skipped="{skipped}">\n')
         for test in tests:
+            if reduced_report and test.result.code not in self.failed_codes:
+                continue
             self._write_test(file, test, name)
         file.write('</testsuite>\n')
 
     def _write_test(self, file, test, suite_name):
         path = '/'.join(test.path_in_suite[:-1]).replace('.', '_')
         class_name = f'{suite_name}.{path or suite_name}'
         name = test.path_in_suite[-1]
         time = test.result.elapsed or 0.0
):
     ):
         test_data['status'] = 'SKIP'
     elif result_code == lit.Test.UNRESOLVED or result_code == lit.Test.TIMEOUT:
         test_data['status'] = 'ABORT'
     return test_data
 
 
 class ResultDBReport(_ReportBase):
-    def write_results(self, tests, elapsed):
+    def write_results(self, tests, elapsed, opts):
+        del opts
         unexecuted_codes = {lit.Test.EXCLUDED, lit.Test.SKIPPED}
         tests = [t for t in tests if t.result.code not in unexecuted_codes]
         data = {}
         data['__version__'] = lit.__versioninfo__
         data['elapsed'] = elapsed
         # Encode the tests.
         data['tests'] = tests_data = []
         for test in tests:
def write_results(self, tests, elapsed, opts):
                     )
 
         with open(self.output_file, 'w') as file:
             json.dump(data, file, indent=2, sort_keys=True)
             file.write('\n')
 
 
 class TimeTraceReport(_ReportBase):
-    def write_results(self, tests, elapsed):
-        del elapsed
+    def write_results(self, tests, elapsed, opts):
+        del elapsed, opts
 
         # Find when first test started so we can make start times relative.
         first_start_time = min([t.result.start for t in tests])
         events = [self._get_test_event(
             x, first_start_time) for x in tests if x.result.code not in self.skipped_codes]
 
         json_data = {'traceEvents': events}