[llvm][llvm-lit] Add option to create unique result file names if results already exist #112729
Conversation
|
@llvm/pr-subscribers-testing-tools Author: David Spickett (DavidSpickett) ChangesWhen running a build like: Prior to my changes you ended up with one results file, in this specific case This would only include the last set of tests lit ran, which were for llvm. for target in targets: I want to use something like this Buildkite reporting plugin in CI, Modifying CI's build scripts for Windows and Linux is a lot of work. Now you will get: This will work for all result file types since I'm doing it in the base Report The number will increment as many times as needed until a useable name Full diff: https://github.com/llvm/llvm-project/pull/112729.diff 3 Files Affected:
diff --git a/llvm/utils/lit/lit/cl_arguments.py b/llvm/utils/lit/lit/cl_arguments.py
index 5ccae4be096796..3b11342dec2162 100644
--- a/llvm/utils/lit/lit/cl_arguments.py
+++ b/llvm/utils/lit/lit/cl_arguments.py
@@ -175,6 +175,13 @@ def parse_args():
type=lit.reports.TimeTraceReport,
help="Write Chrome tracing compatible JSON to the specified file",
)
+ execution_group.add_argument(
+ "--use-unique-output-file-name",
+ help="When enabled, lit will not overwrite existing test report files. "
+ "Instead it will modify the file name until it finds a file name "
+ "that does not already exist. [Default: Off]",
+ action="store_true",
+ )
execution_group.add_argument(
"--timeout",
dest="maxIndividualTestTime",
@@ -332,16 +339,21 @@ def parse_args():
else:
opts.shard = None
- opts.reports = filter(
- None,
- [
- opts.output,
- opts.xunit_xml_output,
- opts.resultdb_output,
- opts.time_trace_output,
- ],
+ opts.reports = list(
+ filter(
+ None,
+ [
+ opts.output,
+ opts.xunit_xml_output,
+ opts.resultdb_output,
+ opts.time_trace_output,
+ ],
+ )
)
+ for report in opts.reports:
+ report.use_unique_output_file_name = opts.use_unique_output_file_name
+
return opts
diff --git a/llvm/utils/lit/lit/reports.py b/llvm/utils/lit/lit/reports.py
index d2d719b076bc70..c2a0239e360f37 100755
--- a/llvm/utils/lit/lit/reports.py
+++ b/llvm/utils/lit/lit/reports.py
@@ -1,7 +1,9 @@
+import abc
import base64
import datetime
import itertools
import json
+import os
from xml.sax.saxutils import quoteattr as quo
@@ -14,11 +16,43 @@ def by_suite_and_test_path(test):
return (test.suite.name, id(test.suite), test.path_in_suite)
-class JsonReport(object):
+class Report(object):
def __init__(self, output_file):
self.output_file = output_file
+ # Set by the option parser later.
+ self.use_unique_output_file_name = False
def write_results(self, tests, elapsed):
+ if self.use_unique_output_file_name:
+ file = None
+ filepath = self.output_file
+ attempt = 0
+ while file is None:
+ try:
+ file = open(filepath, "x")
+ except FileExistsError:
+ attempt += 1
+ # If there is an extension insert before that because most
+ # glob patterns for these will be '*.extension'. Otherwise
+ # add to the end of the path.
+ path, ext = os.path.splitext(self.output_file)
+ filepath = path + f".{attempt}" + ext
+
+ with file:
+ self._write_results_to_file(tests, elapsed, file)
+ else:
+ # Overwrite if the results already exist.
+ with open(self.output_file, "w") as file:
+ self._write_results_to_file(tests, elapsed, file)
+
+ @abc.abstractmethod
+ def _write_results_to_file(self, tests, elapsed, file):
+ """Write test results to the file object "file"."""
+ pass
+
+
+class JsonReport(Report):
+ def _write_results_to_file(self, tests, elapsed, file):
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.
@@ -67,9 +101,8 @@ def write_results(self, tests, elapsed):
tests_data.append(test_data)
- with open(self.output_file, "w") as file:
- json.dump(data, file, indent=2, sort_keys=True)
- file.write("\n")
+ json.dump(data, file, indent=2, sort_keys=True)
+ file.write("\n")
_invalid_xml_chars_dict = {
@@ -88,21 +121,18 @@ def remove_invalid_xml_chars(s):
return s.translate(_invalid_xml_chars_dict)
-class XunitReport(object):
- def __init__(self, output_file):
- self.output_file = output_file
- self.skipped_codes = {lit.Test.EXCLUDED, lit.Test.SKIPPED, lit.Test.UNSUPPORTED}
+class XunitReport(Report):
+ skipped_codes = {lit.Test.EXCLUDED, lit.Test.SKIPPED, lit.Test.UNSUPPORTED}
- def write_results(self, tests, elapsed):
+ def _write_results_to_file(self, tests, elapsed, file):
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))
- file.write("</testsuites>\n")
+ 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))
+ file.write("</testsuites>\n")
def _write_testsuite(self, file, suite, tests):
skipped = 0
@@ -206,11 +236,8 @@ def gen_resultdb_test_entry(
return test_data
-class ResultDBReport(object):
- def __init__(self, output_file):
- self.output_file = output_file
-
- def write_results(self, tests, elapsed):
+class ResultDBReport(Report):
+ def _write_results_to_file(self, tests, elapsed, file):
unexecuted_codes = {lit.Test.EXCLUDED, lit.Test.SKIPPED}
tests = [t for t in tests if t.result.code not in unexecuted_codes]
data = {}
@@ -249,17 +276,14 @@ def write_results(self, tests, elapsed):
)
)
- with open(self.output_file, "w") as file:
- json.dump(data, file, indent=2, sort_keys=True)
- file.write("\n")
+ json.dump(data, file, indent=2, sort_keys=True)
+ file.write("\n")
-class TimeTraceReport(object):
- def __init__(self, output_file):
- self.output_file = output_file
- self.skipped_codes = {lit.Test.EXCLUDED, lit.Test.SKIPPED, lit.Test.UNSUPPORTED}
+class TimeTraceReport(Report):
+ skipped_codes = {lit.Test.EXCLUDED, lit.Test.SKIPPED, lit.Test.UNSUPPORTED}
- def write_results(self, tests, elapsed):
+ def _write_results_to_file(self, tests, elapsed, file):
# Find when first test started so we can make start times relative.
first_start_time = min([t.result.start for t in tests])
events = [
@@ -270,8 +294,7 @@ def write_results(self, tests, elapsed):
json_data = {"traceEvents": events}
- with open(self.output_file, "w") as time_trace_file:
- json.dump(json_data, time_trace_file, indent=2, sort_keys=True)
+ json.dump(json_data, time_trace_file, indent=2, sort_keys=True)
def _get_test_event(self, test, first_start_time):
test_name = test.getFullName()
diff --git a/llvm/utils/lit/tests/unique-output-file.py b/llvm/utils/lit/tests/unique-output-file.py
new file mode 100644
index 00000000000000..e0ce21aebf6950
--- /dev/null
+++ b/llvm/utils/lit/tests/unique-output-file.py
@@ -0,0 +1,28 @@
+# Check that lit will not overwrite existing result files when given
+# --use-unique-output-file-name.
+
+# Files are overwritten without the option.
+# RUN: rm -rf %t.xunit*.xml
+# RUN: echo "test" > %t.xunit.xml
+# RUN: not %{lit} --xunit-xml-output %t.xunit.xml %{inputs}/xunit-output
+# RUN: FileCheck < %t.xunit.xml %s --check-prefix=NEW
+
+# RUN: rm -rf %t.xunit*.xml
+# RUN: echo "test" > %t.xunit.xml
+# Files should not be overwritten with the option.
+# RUN: not %{lit} --xunit-xml-output %t.xunit.xml --use-unique-output-file-name %{inputs}/xunit-output
+# RUN: FileCheck < %t.xunit.xml %s --check-prefix=EXISTING
+# EXISTING: test
+# Results in a new file with "1" added.
+# RUN: FileCheck < %t.xunit.1.xml %s --check-prefix=NEW
+# NEW: <?xml version="1.0" encoding="UTF-8"?>
+# NEW-NEXT: <testsuites time="{{[0-9.]+}}">
+# (assuming that other tests check the whole contents of the file)
+
+# The number should increment as many times as needed.
+# RUN: touch %t.xunit.2.xml
+# RUN: touch %t.xunit.3.xml
+# RUN: touch %t.xunit.4.xml
+
+# RUN: not %{lit} --xunit-xml-output %t.xunit.xml --use-unique-output-file-name %{inputs}/xunit-output
+# RUN: FileCheck < %t.xunit.5.xml %s --check-prefix=NEW
\ No newline at end of file
|
|
A question might be "why can't we just use check-all in CI?". We cannot do that because sometimes in CI we need to disable the tests of a single project, but may continue building that project. Or it may be a dependency of a project we do test, but its tests do not work in CI. |
|
Right now ci does This feature will help me fix that by creating an aggregated report from which you can see right away which tests failed across the whole build. |
There was a problem hiding this comment.
Looks reasonable to me, certainly having a proper summary of all the check runs at the end is a good idea. My only caution is that I'm not a Python expert by any means, and it would be nice if someone else could confirm your statement about propagating the flag.
If they don't, ping me next week and I'll approve.
There was a problem hiding this comment.
I assume you expect the area which will contain the results files will be clean before running the tests in the first place? Otherwise you'll end up having stale results files in addition to new results files, and then sweeping them all up will result in spurious data being recorded.
Another thought I had is you could just name the results file after the specific lit suite it comes from (presumably derived from the directory path or similar somehow?). In this way, new results will overwrite old results without cleanup, but should also be uniquely named.
Yes. I think it's fair enough to expect that CI pipelines will run a pretty simple I suppose someone might want to run lit over and over then compare variance between runs and this feature allows that. Though it's not my use case.
This is what I thought of doing via the shell scripting at first. Looking at lit itself, it can take multiple paths in a single invocation and the test results are the combination of all tests discovered. So if you had lots of things in the same sub-tree we could factor that out and use that in the filename: Produces: Which is I think a different feature than the one I'm proposing but would work equally well for the CI's use case. With the advantage that you can see from the filename which one goes with which project. Though I think only a tiny minority of people would ever take advantage of that. If we wanted a path based naming feature in the future, I don't think this unique file names feature would clash with it. You'd just get My concern with it is figuring out how to condense multiple paths into a filename that's useful to the reader. |
Yes, LGTM |
This is to later allow me to handle the choosing of the filename in one place, so that we can write unique files across multiple runs.
…ults already exist
When running a build like:
ninja check-clang check-llvm
Prior to my changes you ended up with one results file, in this specific case
Junit XML:
results.xml
This would only include the last set of tests lit ran, which were for llvm.
To get around this, many CI systems will run one check target, move the file
away, then run another.
for target in targets:
ninja target
mv results.xml results-${target}.xml
I want to use something like this Buildkite reporting plugin in CI,
which needs to have all the results available:
https://buildkite.com/docs/agent/v3/cli-annotate#using-annotations-to-report-test-results
Modifying CI's build scripts for Windows and Linux is a lot of work.
So my changes instead make lit detect an existing result file and modify the file name
until it finds a unique file name to write to.
Now you will get:
results.xml results.1.xml
This will work for all result file types since I'm doing it in the base Report
class. Now you've got separate files, it's easy to collect them with `<path>/*.xml`.
The number will increment as many times as needed until a useable name
is found.
DavidSpickett
force-pushed
the
lit-reports
branch
from
e0ae853 to
020f9a4
Compare
|
Thanks for the reviews, this is much nicer than what I first proposed! |
When running a build like:
Prior to my changes you ended up with one results file, in this specific case Junit XML:
This would only include the last set of tests lit ran, which were for llvm. To get around this, many CI systems will run one check target, move the file away, then run another, somehow propgating the return code as well.
I want to use something like this Buildkite reporting plugin in CI, which needs to have all the results available:
https://buildkite.com/docs/agent/v3/cli-annotate#using-annotations-to-report-test-results
Modifying CI's build scripts for Windows and Linux is a lot of work. So my changes instead make lit detect an existing result file and modify the file name to find a new file to write to. Now you will get:
This will work for all result file types since I'm doing it in the base Report class. Now you've got separate files, it's easy to collect them with
<path>/*.xml.Note that the
<tempfile generated value>is not ordered.