-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[llvm][lit] Add option to run only the failed tests #158043
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
base: main
Are you sure you want to change the base?
Conversation
Couldn't find an existing flag that does this. If there's already a way to do this, happy to drop the PR |
@llvm/pr-subscribers-testing-tools Author: Michael Buch (Michael137) ChangesThis patch adds a new Full diff: https://github.com/llvm/llvm-project/pull/158043.diff 5 Files Affected:
diff --git a/llvm/utils/lit/lit/cl_arguments.py b/llvm/utils/lit/lit/cl_arguments.py
index 8238bc42395af..a87715f16df2b 100644
--- a/llvm/utils/lit/lit/cl_arguments.py
+++ b/llvm/utils/lit/lit/cl_arguments.py
@@ -302,6 +302,12 @@ def parse_args():
help="Filter out tests with paths matching the given regular expression",
default=os.environ.get("LIT_FILTER_OUT", "^$"),
)
+ selection_group.add_argument(
+ "--filter-failed",
+ dest="filterFailed",
+ help="Only run tests which failed in the previous run.",
+ action="store_true",
+ )
selection_group.add_argument(
"--xfail",
metavar="LIST",
diff --git a/llvm/utils/lit/lit/main.py b/llvm/utils/lit/lit/main.py
index a585cc0abdd48..6c650724bb33d 100755
--- a/llvm/utils/lit/lit/main.py
+++ b/llvm/utils/lit/lit/main.py
@@ -90,6 +90,9 @@ def main(builtin_params={}):
and not opts.filter_out.search(t.getFullName())
]
+ if opts.filterFailed:
+ selected_tests = [t for t in selected_tests if t.previous_failure]
+
if not selected_tests:
sys.stderr.write(
"error: filter did not match any tests "
diff --git a/llvm/utils/lit/tests/Inputs/ignore-fail/pass.txt b/llvm/utils/lit/tests/Inputs/ignore-fail/pass.txt
new file mode 100644
index 0000000000000..18efe9e49e95b
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/ignore-fail/pass.txt
@@ -0,0 +1 @@
+RUN: true
diff --git a/llvm/utils/lit/tests/filter-failed.py b/llvm/utils/lit/tests/filter-failed.py
new file mode 100644
index 0000000000000..074f14cf7fc34
--- /dev/null
+++ b/llvm/utils/lit/tests/filter-failed.py
@@ -0,0 +1,18 @@
+# Checks that --filter-failed only runs tests that previously failed.
+
+# RUN: not %{lit} %{inputs}/ignore-fail
+# RUN: not %{lit} --filter-failed %{inputs}/ignore-fail | FileCheck %s
+
+# END.
+
+# CHECK: Testing: 3 of 5 tests
+# CHECK-DAG: FAIL: ignore-fail :: fail.txt
+# CHECK-DAG: UNRESOLVED: ignore-fail :: unresolved.txt
+# CHECK-DAG: XPASS: ignore-fail :: xpass.txt
+
+# CHECK: Testing Time:
+# CHECK: Total Discovered Tests:
+# CHECK-NEXT: Excluded : 2 {{\([0-9]*\.[0-9]*%\)}}
+# CHECK-NEXT: Unresolved : 1 {{\([0-9]*\.[0-9]*%\)}}
+# CHECK-NEXT: Failed : 1 {{\([0-9]*\.[0-9]*%\)}}
+# CHECK-NEXT: Unexpectedly Passed: 1 {{\([0-9]*\.[0-9]*%\)}}
diff --git a/llvm/utils/lit/tests/ignore-fail.py b/llvm/utils/lit/tests/ignore-fail.py
index 494c6e092c906..51196fbae9e5e 100644
--- a/llvm/utils/lit/tests/ignore-fail.py
+++ b/llvm/utils/lit/tests/ignore-fail.py
@@ -10,9 +10,11 @@
# CHECK-DAG: UNRESOLVED: ignore-fail :: unresolved.txt
# CHECK-DAG: XFAIL: ignore-fail :: xfail.txt
# CHECK-DAG: XPASS: ignore-fail :: xpass.txt
+# CHECK-DAG: PASS: ignore-fail :: pass.txt
# CHECK: Testing Time:
# CHECK: Total Discovered Tests:
+# CHECK-NEXT: Passed : 1 {{\([0-9]*\.[0-9]*%\)}}
# CHECK-NEXT: Expectedly Failed : 1 {{\([0-9]*\.[0-9]*%\)}}
# CHECK-NEXT: Unresolved : 1 {{\([0-9]*\.[0-9]*%\)}}
# CHECK-NEXT: Failed : 1 {{\([0-9]*\.[0-9]*%\)}}
|
I'm surprised we didn't already have that — I guess it's because there's always a risk of breaking other tests while making changes, but that doesn't diminish the utility of this flag! |
Cann you add documentation to this? https://llvm.org/docs/CommandGuide/lit.html |
done in latest commit! |
llvm/docs/CommandGuide/lit.rst
Outdated
@@ -314,6 +314,10 @@ The timing data is stored in the `test_exec_root` in a file named | |||
place of this option, which is especially useful in environments where the | |||
call to ``lit`` is issued indirectly. | |||
|
|||
.. option:: --filter-failed | |||
|
|||
Run only those tests that previously failed. |
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.
It would be helpful noting what happens a) to newly added tests and b) if lit hasn't been run before. These cases deserve testing too.
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.
Added a test for this in the latest commit. Since i'm now echo
ing into the inputs directory I decided to create a dedicated one for this test (so it doesn't affect the ignore-fail
test in case things go wrong).
llvm/utils/lit/lit/cl_arguments.py
Outdated
selection_group.add_argument( | ||
"--filter-failed", | ||
dest="filterFailed", | ||
help="Only run tests which failed in the previous run.", |
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.
Nit: there's a bit of a max of help text ending with and without '.'. I think the majority omit it.
# CHECK: Testing Time: | ||
# CHECK: Total Discovered Tests: |
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.
What's with the inconsistent spacing here?
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.
Thanks, mostly looks good to me. I've got a couple more test suggestions:
- Show the behaviour when a test failed on the first run and was deleted before being run again with --filter-failed.
- Show that a failed test that then subsequently passes (under a --filter-failed run) doesn't get run if the tests are executed with --filter-failed again. In other words, the "failed" state is cleared once a test has been rerun successfully.
Yup those seem like good things to test, thanks! Added in latest commit |
# RUN: not %{lit} --filter-failed %{inputs}/filter-failed-delete > %s.rerun.log | ||
# RUN: mv %{inputs}/filter-failed-delete/fail.txt.bk %{inputs}/filter-failed-delete/fail.txt | ||
# | ||
# RUN: cat %s.rerun.log | FileCheck %s --check-prefix=CHECK-RERUN |
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.
# RUN: cat %s.rerun.log | FileCheck %s --check-prefix=CHECK-RERUN | |
# RUN: FileCheck %s --input-file=%s.rerun.log --check-prefix=CHECK-RERUN |
# | ||
# RUN: mv %{inputs}/filter-failed-delete/fail.txt %{inputs}/filter-failed-delete/fail.txt.bk | ||
# RUN: not %{lit} --filter-failed %{inputs}/filter-failed-delete > %s.rerun.log | ||
# RUN: mv %{inputs}/filter-failed-delete/fail.txt.bk %{inputs}/filter-failed-delete/fail.txt |
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've seen changes trying to make tests work in a read-only context, which this certainly won't. Could this test all be run in a temporary directory, copied from some source?
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! In the latest commit i copy the inputs to %t
and do all the manipulation there. That makes the tests much shorter and removes the need for three separate input directories. Is this what you meant? Not sure if the rm -rf %t
is valid for read-only contexts though.
# RUN: cp %{inputs}/filter-failed-rerun/pass.txt %{inputs}/filter-failed-rerun/fail.txt | ||
# RUN: not %{lit} %{inputs}/filter-failed-rerun > %s.rerun-1.log | ||
# RUN: not %{lit} --filter-failed %{inputs}/filter-failed-rerun > %s.rerun-2.log | ||
# RUN: mv %{inputs}/filter-failed-rerun/fail.txt.bk %{inputs}/filter-failed-rerun/fail.txt |
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.
Same comments in this file as the other test.
This patch adds a new `--filter-failed` option to `llvm-lit`, which when set, will only run the tests that have previously failed.
cab413c
to
4b39c15
Compare
4b39c15
to
4aa6397
Compare
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.
Basically looks good. Couple more minor points and then we're good.
On the note of %t
, it should work in a read-only context, since regular tests generally write objects there without issue.
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.
LGTM.
Looks like Windows CI isn't happy. Checking.. |
Oh probably because I'm echoing the file incorrectly on Windows:
|
Hmm a bit confused. It looks like |
@jh7370 do you have any idea what could be going on here? Do you see anything wrong here that would cause the tests not to work on Windows. For some reason in |
I don't know why CI isn't showing it up as failing, but filter-failed-delete.py fails for me, as well as the other two. |
This patch adds a new
--filter-failed
option tollvm-lit
, which when set, will only run the tests that have previously failed.