[lit] Add --report-failures-only option for lit test reports #115439

rpatel321 · 2024-11-08T08:02:07Z

Add option (--report-failures-only) to generate a reduced report for lit tests that only includes failing tests
This is a continuation of proposed patches by @gregbedwell here:
- https://reviews.llvm.org/D143516
- https://reviews.llvm.org/D143519

Co-authored-by: Greg Bedwell <[email protected]>

github-actions · 2024-11-08T08:02:25Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-11-08T08:02:57Z

@llvm/pr-subscribers-testing-tools

Author: None (rpatel321)

Changes

Add option (--report-failures-only) to generate a reduced report for lit tests that does not include passing tests
This is a continuation of proposed patches by @gregbedwell here:
- https://reviews.llvm.org/D143516
- https://reviews.llvm.org/D143519

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

4 Files Affected:

(modified) llvm/utils/lit/lit/cl_arguments.py (+6)
(modified) llvm/utils/lit/lit/main.py (+4)
(modified) llvm/utils/lit/lit/reports.py (+2-4)
(added) llvm/utils/lit/tests/xunit-output-report-failures-only.py (+17)

diff --git a/llvm/utils/lit/lit/cl_arguments.py b/llvm/utils/lit/lit/cl_arguments.py
index 3e5488f388ccfa..4ca0e72216ed58 100644
--- a/llvm/utils/lit/lit/cl_arguments.py
+++ b/llvm/utils/lit/lit/cl_arguments.py
@@ -165,6 +165,12 @@ def parse_args():
         type=lit.reports.XunitReport,
         help="Write XUnit-compatible XML test reports to the specified file",
     )
+    execution_group.add_argument(
+        "--report-failures-only",
+        help="When writing a 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,
diff --git a/llvm/utils/lit/lit/main.py b/llvm/utils/lit/lit/main.py
index 24ba804f0c363f..e8423ec3163a97 100755
--- a/llvm/utils/lit/lit/main.py
+++ b/llvm/utils/lit/lit/main.py
@@ -137,6 +137,10 @@ def main(builtin_params={}):
     print_results(discovered_tests, elapsed, opts)
 
     tests_for_report = selected_tests if opts.shard else discovered_tests
+    if opts.report_failures_only:
+        # Only report tests that failed
+        tests_for_report = [t for t in tests_for_report if t.isFailure()]
+
     for report in opts.reports:
         report.write_results(tests_for_report, elapsed)
 
diff --git a/llvm/utils/lit/lit/reports.py b/llvm/utils/lit/lit/reports.py
index 8312dcddc769ae..845aeda7b47f22 100755
--- a/llvm/utils/lit/lit/reports.py
+++ b/llvm/utils/lit/lit/reports.py
@@ -22,6 +22,8 @@ def __init__(self, output_file):
         self.output_file = output_file
         # Set by the option parser later.
         self.use_unique_output_file_name = False
+        self.skipped_codes = {lit.Test.EXCLUDED,
+                              lit.Test.SKIPPED, lit.Test.UNSUPPORTED}
 
     def write_results(self, tests, elapsed):
         if self.use_unique_output_file_name:
@@ -114,8 +116,6 @@ def remove_invalid_xml_chars(s):
 
 
 class XunitReport(Report):
-    skipped_codes = {lit.Test.EXCLUDED, lit.Test.SKIPPED, lit.Test.UNSUPPORTED}
-
     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)
@@ -273,8 +273,6 @@ def _write_results_to_file(self, tests, elapsed, file):
 
 
 class TimeTraceReport(Report):
-    skipped_codes = {lit.Test.EXCLUDED, lit.Test.SKIPPED, lit.Test.UNSUPPORTED}
-
     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])
diff --git a/llvm/utils/lit/tests/xunit-output-report-failures-only.py b/llvm/utils/lit/tests/xunit-output-report-failures-only.py
new file mode 100644
index 00000000000000..3c8c49362aa175
--- /dev/null
+++ b/llvm/utils/lit/tests/xunit-output-report-failures-only.py
@@ -0,0 +1,17 @@
+# UNSUPPORTED: system-windows
+
+# Check xunit output
+# RUN: rm -rf %t.xunit.xml
+# RUN: not %{lit} --report-failures-only --xunit-xml-output %t.xunit.xml %{inputs}/xunit-output
+# If xmllint is installed verify that the generated xml is well-formed
+# RUN: sh -c 'if command -v xmllint 2>/dev/null; then xmllint --noout %t.xunit.xml; fi'
+# RUN: FileCheck < %t.xunit.xml %s
+
+# 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" 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>
+# CHECK-NEXT: </testsuite>
+# CHECK-NEXT: </testsuites>

jh7370

I suspect there's probably some documentation somewhere to update that describes lit.

llvm/utils/lit/lit/main.py

jh7370 · 2024-11-08T08:38:20Z

llvm/utils/lit/lit/reports.py

+        self.skipped_codes = {lit.Test.EXCLUDED,
+                              lit.Test.SKIPPED, lit.Test.UNSUPPORTED}


Whilst there's nothing fundamentally wrong with this, it looks like it's unrelated to the focus of the PR, so should be split into its own one, if you want to make the change.

This made it's way over from patch #1 (https://reviews.llvm.org/D143516). However, since it's not required for my changes, I will split it into another PR as you are suggesting.

jh7370 · 2024-11-08T08:41:58Z

llvm/utils/lit/tests/xunit-output-report-failures-only.py

+# If xmllint is installed verify that the generated xml is well-formed
+# RUN: sh -c 'if command -v xmllint 2>/dev/null; then xmllint --noout %t.xunit.xml; fi'


This feels unnecessary, and it's forcing an UNSUPPORTED: system-windows line (actually, I suspect the requirement is on the shell not on non-Windows, but that's an aside - it would be better to not have the requirement at all). You're checking the contents of the file immediately below, so as long as your contents check is valid, you'll know that the XML must be valid.

I have removed the xmllint check and the forced UNSUPPORTED: system-windows line. I've also removed the rm -rf line as it's not really needed because the previous file will get overwritten and I don't think it's supported in windows shell either.

llvm/utils/lit/tests/xunit-output-report-failures-only.py

jh7370 · 2024-11-08T08:44:09Z

llvm/utils/lit/tests/xunit-output-report-failures-only.py

+# RUN: not %{lit} --report-failures-only --xunit-xml-output %t.xunit.xml %{inputs}/xunit-output
+# If xmllint is installed verify that the generated xml is well-formed
+# RUN: sh -c 'if command -v xmllint 2>/dev/null; then xmllint --noout %t.xunit.xml; fi'
+# RUN: FileCheck < %t.xunit.xml %s


The canonical way of passing an input file into FileCheck is to use --input-file.

llvm/utils/lit/lit/main.py

gregbedwell · 2024-11-08T10:46:07Z

Thanks so much for picking this up @rpatel321 !

gregbedwell · 2024-11-08T10:48:34Z

Just for completeness, here was my justification for this change from the original review I posted:

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.

Co-authored-by: James Henderson <[email protected]>

github-actions · 2024-11-12T09:19:08Z

✅ With the latest revision this PR passed the Python code formatter.

jh7370

The pre-merge code formatting check failed for the python code. You need to run darker on it to make sure it is formatted correctly.

LGTM, apart from that.

…ps://github.com/rpatel321/llvm-project into users/rpatel321/lit-report-failures-only-option

github-actions · 2024-11-13T08:30:53Z

@rpatel321 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Add --report-failures-only option for lit test reports

17f4900

Co-authored-by: Greg Bedwell <[email protected]>

llvmbot added llvm-lit testing-tools labels Nov 8, 2024

jh7370 requested a review from gregbedwell November 8, 2024 08:35

jh7370 reviewed Nov 8, 2024

View reviewed changes

rpatel321 and others added 5 commits November 8, 2024 09:47

Add full stop to comment

4140e14

Co-authored-by: James Henderson <[email protected]>

Use double comment markers and full stop

edc0935

Co-authored-by: James Henderson <[email protected]>

Move skipped codes back to XunitReport and TimeTraceReport

1ebd26c

Remove redundant xmllint check

5d16c98

Remove rm -rf command

e09e4f1

rpatel321 requested a review from jh7370 November 8, 2024 18:45

rpatel321 and others added 3 commits November 8, 2024 21:48

Update documentation

784e24e

Merge branch 'main' into users/rpatel321/lit-report-failures-only-option

0c7747f

Merge branch 'main' into users/rpatel321/lit-report-failures-only-option

f18430a

jh7370 approved these changes Nov 12, 2024

View reviewed changes

rpatel321 and others added 3 commits November 12, 2024 10:53

Fix formatting

6793a94

Merge branch 'users/rpatel321/lit-report-failures-only-option' of htt…

82a70bf

…ps://github.com/rpatel321/llvm-project into users/rpatel321/lit-report-failures-only-option

Merge branch 'main' into users/rpatel321/lit-report-failures-only-option

c58f133

jh7370 merged commit c63e83f into llvm:main Nov 13, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lit] Add --report-failures-only option for lit test reports #115439

[lit] Add --report-failures-only option for lit test reports #115439

Uh oh!

rpatel321 commented Nov 8, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Nov 8, 2024

Uh oh!

llvmbot commented Nov 8, 2024

Uh oh!

jh7370 left a comment

Uh oh!

Uh oh!

jh7370 Nov 8, 2024

Uh oh!

rpatel321 Nov 8, 2024

Uh oh!

jh7370 Nov 8, 2024

Uh oh!

rpatel321 Nov 8, 2024

Uh oh!

Uh oh!

jh7370 Nov 8, 2024

Uh oh!

Uh oh!

gregbedwell commented Nov 8, 2024

Uh oh!

gregbedwell commented Nov 8, 2024

Uh oh!

github-actions bot commented Nov 12, 2024 •

edited

Loading

Uh oh!

jh7370 left a comment

Uh oh!

Uh oh!

github-actions bot commented Nov 13, 2024

Uh oh!

Uh oh!

		self.skipped_codes = {lit.Test.EXCLUDED,
		lit.Test.SKIPPED, lit.Test.UNSUPPORTED}

		# If xmllint is installed verify that the generated xml is well-formed
		# RUN: sh -c 'if command -v xmllint 2>/dev/null; then xmllint --noout %t.xunit.xml; fi'

[lit] Add --report-failures-only option for lit test reports #115439

[lit] Add --report-failures-only option for lit test reports #115439

Uh oh!

Conversation

rpatel321 commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 8, 2024

Uh oh!

llvmbot commented Nov 8, 2024

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jh7370 Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

rpatel321 Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

jh7370 Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

rpatel321 Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jh7370 Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gregbedwell commented Nov 8, 2024

Uh oh!

gregbedwell commented Nov 8, 2024

Uh oh!

github-actions bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 13, 2024

Uh oh!

Uh oh!

rpatel321 commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 12, 2024 •

edited

Loading