[lit] Add an option to read an xfail list from a file #163959
Conversation
|
@llvm/pr-subscribers-testing-tools Author: None (dcandler) ChangesThe However, we've encountered cases where the list needs to be quite long (100+ tests), which leads to some very long command strings. This patch extends the existing options with additional ones, Full diff: https://github.com/llvm/llvm-project/pull/163959.diff 5 Files Affected:
diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index 6a721ebf9cad0..061f9b4c48186 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -346,6 +346,10 @@ The timing data is stored in the `test_exec_root` in a file named
LIT_XFAIL="affinity/kmp-hw-subset.c;libomptarget :: x86_64-pc-linux-gnu :: offloading/memory_manager.cpp"
+.. option:: --xfail-from-file PATH
+
+ Read a line separated list of tests from a file to be used by :option:`--xfail`.
+
.. option:: --xfail-not LIST
Do not treat the specified tests as ``XFAIL``. The environment variable
@@ -356,6 +360,10 @@ The timing data is stored in the `test_exec_root` in a file named
primary purpose is to suppress an ``XPASS`` result without modifying a test
case that uses the ``XFAIL`` directive.
+.. option:: --xfail-not-from-file PATH
+
+ Read a line separated list of tests from a file to be used by :option:`--xfail-not`.
+
.. option:: --exclude-xfail
``XFAIL`` tests won't be run, unless they are listed in the ``--xfail-not``
diff --git a/llvm/utils/lit/lit/cl_arguments.py b/llvm/utils/lit/lit/cl_arguments.py
index 8238bc42395af..28d7611896e98 100644
--- a/llvm/utils/lit/lit/cl_arguments.py
+++ b/llvm/utils/lit/lit/cl_arguments.py
@@ -309,6 +309,12 @@ def parse_args():
help="XFAIL tests with paths in the semicolon separated list",
default=os.environ.get("LIT_XFAIL", ""),
)
+ selection_group.add_argument(
+ "--xfail-from-file",
+ metavar="PATH",
+ help="XFAIL tests with paths in the line separated list contained in "
+ "the specified file",
+ )
selection_group.add_argument(
"--xfail-not",
metavar="LIST",
@@ -316,6 +322,12 @@ def parse_args():
help="do not XFAIL tests with paths in the semicolon separated list",
default=os.environ.get("LIT_XFAIL_NOT", ""),
)
+ selection_group.add_argument(
+ "--xfail-not-from-file",
+ metavar="PATH",
+ help="do not XFAIL tests with paths in the line separated list "
+ "contained in the specified file",
+ )
selection_group.add_argument(
"--exclude-xfail",
help="exclude XFAIL tests (unless they are in the --xfail-not list). "
@@ -396,6 +408,13 @@ def parse_args():
for report in opts.reports:
report.use_unique_output_file_name = opts.use_unique_output_file_name
+ if opts.xfail_from_file:
+ with open(opts.xfail_from_file, "r", encoding="utf-8") as f:
+ opts.xfail.extend(f.read().splitlines())
+ if opts.xfail_not_from_file:
+ with open(opts.xfail_not_from_file, "r", encoding="utf-8") as f:
+ opts.xfail_not.extend(f.read().splitlines())
+
return opts
diff --git a/llvm/utils/lit/tests/Inputs/xfail-cl/xfail-not.list b/llvm/utils/lit/tests/Inputs/xfail-cl/xfail-not.list
new file mode 100644
index 0000000000000..24b776b870893
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/xfail-cl/xfail-not.list
@@ -0,0 +1,2 @@
+true-xfail.txt
+top-level-suite :: a :: test-xfail.txt
diff --git a/llvm/utils/lit/tests/Inputs/xfail-cl/xfail.list b/llvm/utils/lit/tests/Inputs/xfail-cl/xfail.list
new file mode 100644
index 0000000000000..251f6cdf9d3bd
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/xfail-cl/xfail.list
@@ -0,0 +1,3 @@
+false.txt
+false2.txt
+top-level-suite :: b :: test.txt
diff --git a/llvm/utils/lit/tests/xfail-cl.py b/llvm/utils/lit/tests/xfail-cl.py
index f1e0e335e396f..9d7102bc2ec9b 100644
--- a/llvm/utils/lit/tests/xfail-cl.py
+++ b/llvm/utils/lit/tests/xfail-cl.py
@@ -16,6 +16,10 @@
# RUN: %{inputs}/xfail-cl \
# RUN: | FileCheck --check-prefixes=CHECK-EXCLUDED,CHECK-EXCLUDED-OVERRIDE %s
+# RUN: %{lit} --xfail-from-file %{inputs}/xfail-cl/xfail.list \
+# RUN: --xfail-not-from-file %{inputs}/xfail-cl/xfail-not.list \
+# RUN: %{inputs}/xfail-cl \
+# RUN: | FileCheck --check-prefix=CHECK-FILTER %s
# RUN: env LIT_XFAIL='false.txt;false2.txt;top-level-suite :: b :: test.txt' \
# RUN: LIT_XFAIL_NOT='true-xfail.txt;top-level-suite :: a :: test-xfail.txt' \
|
The `--xfail` and `--xfail-not` lit options allow a list of tests to be marked as expected to fail/pass on the command line. We've found this to be useful in CI to temporarily change the state of a test without making changes to the test sources, while a more appropriate fix is investigated. However, we've encountered cases where the list needs to be quite long (100+ tests), which leads to some very long command strings. This patch extends the existing options with additional ones, `--xfail-from-file` and `--xfail-not-from-file` that will instead read the list from a file instead of directly from the command line. The underlying functionality remains exactly the same, with only an alternative source for the lists.
dcandler
force-pushed
the
lit_xfail_files
branch
from
bb48391 to
e2ed803
Compare
There was a problem hiding this comment.
Eventually you would hit shell size limits too, so this is a bit like clang's response files (the ones you write options in because they're too long). Your alternative is the environment variable, but it will have its own size limits.
Shame there isn't a way to disambiguate a file path to the existing option. We could see if the string given was an existing file, but if the path/pattern of an xfail is the path of a file, that can't work. Plus it doesn't clearly look like it's being passed as a file to anyone reading the code.
--xfail-from-file
Let me guess some of the naming logic.
--xfail-listis ok but it doesn't make it clear it wants a file (could just be another name for the existing option).--xfail-filecould either be a file containing xfails, or a single path of a test you want to xfail.--xfails-from-file(plural) sounds logical, but the existing option isn't pluralised so it would be inconsistent.
I am fine with the name on that basis.
The main thing I'd like to see here is it made clear that that's all this is, existing options but with values taken from a file.
...which makes me wonder should lit have the option to take all of its options from a file? We have other arguments with values like this.
Python argparse supports this https://docs.python.org/3/library/argparse.html#fromfile-prefix-chars
Could that work for you?
| .. option:: --xfail-from-file PATH | ||
|
|
||
| Read a line separated list of tests from a file to be used by :option:`--xfail`. |
There was a problem hiding this comment.
Maybe "line separated" is a standard term but I don't know if it means:
thing
thing
Or:
thing\nthing
I guess the latter?
Or in other words: one test name/pattern per line.
There was a problem hiding this comment.
And yes I see below, the splitlines() call.
There was a problem hiding this comment.
If you've got a citation for the term, fine, but it's not familiar to me.
|
|
||
| .. option:: --xfail-not-from-file PATH | ||
|
|
||
| Read a line separated list of tests from a file to be used by :option:`--xfail-not`. |
There was a problem hiding this comment.
Saying it's used by the option is detail that users don't really need. What they might want to know is that this option does add to that other one. So you could say:
Acts as --xfail-not, except that the paths are read from the specified file. If this option and --xfail-not are specified at the same time, both sets of paths are considered.
Then you're also going back to the "this option but from a file" idea that you stated in the PR description.
| "--xfail-from-file", | ||
| metavar="PATH", | ||
| help="XFAIL tests with paths in the line separated list contained in " | ||
| "the specified file", |
There was a problem hiding this comment.
Maybe reword to split purpose and format:
XFAIL tests whose paths are contained in the specified file. Each path should be newline separated in the file.
There was a problem hiding this comment.
But also consider like above, saying it is just that other option but sourced from a file.
The basic format is: You should experiment to see how it handles newlines or not. It could be that your 100 xfails still need to be all on one line. |
There was a problem hiding this comment.
Can you just use the environment variable? We have a list that is 1000+ tests long in https://github.com/llvm/llvm-project/blob/main/llvm/utils/profcheck-xfail.txt that we use in https://github.com/llvm/llvm-zorg/blob/449407d6c16c997a75191fb0bdb6dc460e5d8017/zorg/buildbot/builders/annotated/profcheck.sh#L18 for this purpose. I don't think you're going to run into shell size limits until you're disabling a huge number of tests.
I'm also not currently convinced that just having a long command line justifies the slight additional complexity of this feature.
That's been my current workaround, but the platform limits on Windows were what made me look at other solutions.
After some refactoring of our downstream code, this does indeed work. The entire test list does need to all be on one line so it isn't particularly readable, but since it's a generated file only read by CMake I don't think that will be an issue. I agree it would be better to avoid overcomplicating with additional options, so I'll close this for now. Thanks for taking a look. |
4 participants
The
--xfailand--xfail-notlit options allow a list of tests to be marked as expected to fail/pass on the command line. We've found this to be useful in CI to temporarily change the state of a test without making changes to the test sources, while a more appropriate fix is investigated.However, we've encountered cases where the list needs to be quite long (100+ tests), which leads to some very long command strings.
This patch extends the existing options with additional ones,
--xfail-from-fileand--xfail-not-from-filethat will instead read the list from a file instead of directly from the command line. The underlying functionality remains exactly the same, with only an alternative source for the lists.