[lit] Add --ignore-fail

[jdenny]

Summary

For some build configurations, check-all calls lit multiple times to
run multiple lit test suites. Most recently, I've found this to be
true when configuring openmp as part of LLVM_ENABLE_RUNTIMES, but
this is not the first time.

If one test suite fails, none of the remaining test suites run, so you
cannot determine if your patch has broken them. It can then be
frustrating to try to determine which check- targets will run the
remaining tests without getting stuck on the failing tests.

When such cases arise, it is probably best to adjust the cmake
configuration for check-all to run all test suites as part of one
lit invocation. Because that fix will likely not be implemented and
land immediately, this patch introduces --ignore-fail to serve as a
workaround for developers trying to see test results until it does
land:

$ LIT_OPTS=--ignore-fail ninja check-all

One problem with --ignore-fail is that it makes it challenging to
detect test failures in a script, perhaps in CI. This problem should
serve as motivation to actually fix the cmake configuration instead of
continuing to use --ignore-fail indefinitely.

[jhenderson]

Personally, I think it should be the default to run all testsuites rather than exiting after the first failure. The exit code should then be the combined one, i.e. whether any of the suites has failed.

Is it not possible to pass the exit code up the stack rather than sys.exit(1) there? The caller (llvm-lit.py?) of the multiple suites would then be responsible for checking all main() calls passed.

[thopre]

In D96371#2553434, @jhenderson wrote:

Personally, I think it should be the default to run all testsuites rather than exiting after the first failure. The exit code should then be the combined one, i.e. whether any of the suites has failed.

Is it not possible to pass the exit code up the stack rather than sys.exit(1) there? The caller (llvm-lit.py?) of the multiple suites would then be responsible for checking all main() calls passed.

Was about to comment the same. Something a la --keep-going instead of --ignore-fail

[jdenny]

In D96371#2553434, @jhenderson wrote:

Personally, I think it should be the default to run all testsuites rather than exiting after the first failure. The exit code should then be the combined one, i.e. whether any of the suites has failed.

Is it not possible to pass the exit code up the stack rather than sys.exit(1) there? The caller (llvm-lit.py?) of the multiple suites would then be responsible for checking all main() calls passed.

The caller of lit each time is ninja/make. To fix this case, cmake files would likely be redesigned so that lit is called once for all suites together. Surely that's technically possible, and I agree it would be better than using --ignore-fail.

However, I argue that this isn't the first time I've seen check-all call lit multiple times, and I'm assuming it will happen again even if this case is fixed. This patch provides an easy workaround every time, and it has a small footprint within lit.

[thopre]

In D96371#2554143, @jdenny wrote:

In D96371#2553434, @jhenderson wrote:

Personally, I think it should be the default to run all testsuites rather than exiting after the first failure. The exit code should then be the combined one, i.e. whether any of the suites has failed.

Is it not possible to pass the exit code up the stack rather than sys.exit(1) there? The caller (llvm-lit.py?) of the multiple suites would then be responsible for checking all main() calls passed.

The caller of lit each time is ninja/make. To fix this case, cmake files would likely be redesigned so that lit is called once for all suites together. Surely that's technically possible, and I agree it would be better than using --ignore-fail.

However, I argue that this isn't the first time I've seen check-all call lit multiple times, and I'm assuming it will happen again even if this case is fixed. This patch provides an easy workaround every time, and it has a small footprint within lit.

Surely you can ignore the error code in the lit caller one way or another?

[jdenny]

In D96371#2554353, @thopre wrote:

In D96371#2554143, @jdenny wrote:

In D96371#2553434, @jhenderson wrote:

Personally, I think it should be the default to run all testsuites rather than exiting after the first failure. The exit code should then be the combined one, i.e. whether any of the suites has failed.

Is it not possible to pass the exit code up the stack rather than sys.exit(1) there? The caller (llvm-lit.py?) of the multiple suites would then be responsible for checking all main() calls passed.

The caller of lit each time is ninja/make. To fix this case, cmake files would likely be redesigned so that lit is called once for all suites together. Surely that's technically possible, and I agree it would be better than using --ignore-fail.

However, I argue that this isn't the first time I've seen check-all call lit multiple times, and I'm assuming it will happen again even if this case is fixed. This patch provides an easy workaround every time, and it has a small footprint within lit.

Surely you can ignore the error code in the lit caller one way or another?

Agreed. But my point is that --ignore-fail can serve as a workaround each time this happens until someone figures out how to fix the cmake scripts.

[thopre]

I'm still not sure, I think a failure is a powerful motivator to fix something but patch LGTM otherwise. @jhenderson what do you think?

[jhenderson]

In D96371#2556413, @thopre wrote:

I'm still not sure, I think a failure is a powerful motivator to fix something but patch LGTM otherwise. @jhenderson what do you think?

Sorry, I'm a little bit busy with other reviews, so haven't had a chance to dig into this properly yet.

If I follow what you're saying correctly, rather than check-all executing lit in a single run across multiple directories, in some cases, it is spawning a new lit process to run some test subset. Is that right? If so, I wonder if there's a better solution of getting check-all to actually run all the tests as one big test.

[jdenny]

In D96371#2559299, @jhenderson wrote:

In D96371#2556413, @thopre wrote:

I'm still not sure, I think a failure is a powerful motivator to fix something but patch LGTM otherwise. @jhenderson what do you think?

Sorry, I'm a little bit busy with other reviews, so haven't had a chance to dig into this properly yet.

If I follow what you're saying correctly, rather than check-all executing lit in a single run across multiple directories, in some cases, it is spawning a new lit process to run some test subset. Is that right?

Yes. In the current case, that subset is for runtimes specified in LLVM_ENABLE_RUNTIMES. I believe the last case I saw was compiler-rt/test/gwp_asan/unit, but I think that one has since been fixed.

If so, I wonder if there's a better solution of getting check-all to actually run all the tests as one big test.

Agreed.

However, especially after these responses, I see --ignore-fail as a temporary workaround. By "temporary", I mean until the cmake config issue is fixed. I see it as a permanent lit feature because this isn't the first time our cmake config has had this issue, and I'm betting it won't be the last time. Otherwise, I wouldn't have proposed this patch upstream.

In D96371#2556413, @thopre wrote:

I'm still not sure, I think a failure is a powerful motivator to fix something

Good point. Even so, I think there is still motivation to fix such cmake issues: because the exit status is zero when using --ignore-fail, trying to detect failures in a script can be ugly.

I've updated the review summary to account for everyone's feedback, which has honed my view. Thanks.

[jhenderson]

I'm coming around to the benefits of this, but first have you considered whether the D96662 would be sufficient for your needs? It seems that you could use that to ignore/xfail the bad tests and continue, without using --ignore-fail and risk accidentally missing failures to do with your work (possibly in more obscure places).

[jdenny]

In D96371#2564848, @jhenderson wrote:

I'm coming around to the benefits of this, but first have you considered whether the D96662 would be sufficient for your needs? It seems that you could use that to ignore/xfail the bad tests and continue,

--xfail could prove frustrating for my current use case. The problem is that openmp test failures are often racy. I'd have to run check-all potentially many times trying to make my --xfail match the failing tests.

--skip avoids that problem because it doesn't care about the specified tests' results. But I want to see those results, so I'd need to run check-all twice, once with the problematic tests, and once skipping them. Seems ok, but...

without using --ignore-fail and risk accidentally missing failures to do with your work (possibly in more obscure places).

This point almost convinced me in the case of --skip. However, one advantage of --ignore-fail is that it makes it possible to just simply run everything. With --ignore-fail, I don't have to figure out the right pattern that skips just tests in the first lit invocation without accidentally skipping other tests I didn't realize matched my pattern. For example, some openmp-related patterns would match in both lit invocations because clang's test suite runs in the second lit invocation. With --ignore-fail, I don't have to run check-all (almost 2 hours on my laptop) many times trying to get the --skip pattern just right. I won't get through the second lit invocation and think I'm done without realizing a third lit invocation has been introduced (no, I'm not aware of a third right now). --ignore-fail just runs everything.

The caveat is that I might miss test failures in the scroll, but anyone using --ignore-fail should know they're ignoring failures, so surely they're on the lookout.

To be clear, I think --xfail and --skip will prove useful for other purposes, especially when you're investigating a smaller set of tests or trying to write careful scripts. In contrast, check-all can be huge. I don't want to tinker trying to make it run everything. After waiting two hours, I just want to see the test results... especially if I previously ran it once, came back two hours later, and discovered it skipped entire test suites.

[jhenderson]

Okay. I'm happy to proceed. I think the lit documentation needs updating to mention this change?

llvm/utils/lit/tests/ignore-fail.py

8

What's the point of this line?

[jdenny]

In D96371#2567707, @jhenderson wrote:

Okay. I'm happy to proceed.

Thanks for considering it. Likewise to @thopre.

I think the lit documentation needs updating to mention this change?

You mean lit.rst? I'm happy to put it there. However, first, what is the threshold in general? Many options are missing there. Is that intentional or by accident? (If it's always by accident, we really ought to consider generating that part from lit --help so we don't have to maintain two versions of the same thing. That should be a different patch of course.)

llvm/utils/lit/tests/ignore-fail.py

8

Without it, lit chokes on XFAIL: below. I actually didn't notice it mattered when I wrote it. But I'm developing a habit of using END. in lit's test suite, where lit directives often appear in FileCheck patterns.

[jhenderson]

In D96371#2568808, @jdenny wrote:

I think the lit documentation needs updating to mention this change?

You mean lit.rst? I'm happy to put it there. However, first, what is the threshold in general? Many options are missing there. Is that intentional or by accident?

At least in the LLVM binutils, the threshold has been "any option", and the only way to catch it is to make sure reviewers remember to mention it in the review. I think it's likely the missing options are all accidental, but I haven't dug into the history to confirm this one way or the other.

(If it's always by accident, we really ought to consider generating that part from lit --help so we don't have to maintain two versions of the same thing. That should be a different patch of course.)

I am not opposed to someone doing that, but good luck getting it to work nicely (complete with sub-sectioning as is already present). Any solution should be applied more generally across the other tools with documentation in the Command Guide, I think. I also wouldn't say that the help text and the documentation should always match, so they're not always two places for the same thing. In general, the documentation can be more detailed, with e.g. examples, which don't really belong in the help text.

[jdenny]

Addressed @jhenderson's review. Rebased.

[jdenny]

In D96371#2578424, @jhenderson wrote:

In D96371#2568808, @jdenny wrote:

I think the lit documentation needs updating to mention this change?

You mean lit.rst? I'm happy to put it there. However, first, what is the threshold in general? Many options are missing there. Is that intentional or by accident?

At least in the LLVM binutils, the threshold has been "any option", and the only way to catch it is to make sure reviewers remember to mention it in the review. I think it's likely the missing options are all accidental, but I haven't dug into the history to confirm this one way or the other.

(If it's always by accident, we really ought to consider generating that part from lit --help so we don't have to maintain two versions of the same thing. That should be a different patch of course.)

I am not opposed to someone doing that, but good luck getting it to work nicely (complete with sub-sectioning as is already present). Any solution should be applied more generally across the other tools with documentation in the Command Guide, I think.

Agreed, but we could start with one tool and extend to others as we figure out the best approach.

I also wouldn't say that the help text and the documentation should always match, so they're not always two places for the same thing. In general, the documentation can be more detailed, with e.g. examples, which don't really belong in the help text.

Perhaps there could be some way to override/extend the help text for specific options.

Well, for now, I've added --ignore-fail to lit.rst, and I'm not going to work on this more general approach. Maybe later. Thanks for presenting your thoughts.

[jhenderson]

LGTM, thanks!

[jdenny]

Thanks!

llvm/docs/CommandGuide/lit.rst

 
 .. option:: --time-tests
 
  Track the wall time individual tests take to execute and includes the results
  in the summary output.  This is useful for determining which tests in a test
  suite take the most time to execute.  Note that this option is most useful
  with ``-j 1``.
 
+.. option:: --ignore-fail
+
+ Exit with status zero even if some tests fail.
+
 .. option:: --no-indirectly-run-check
 
  Do not error if a test would not be run if the user had specified the
  containing directory instead of naming the test directly.
 
 .. _selection-options:
 
 SELECTION OPTIONS

llvm/utils/lit/lit/cl_arguments.py

execution_group.add_argument("--timeout",
                  "0 means no time limit. [Default: 0]",
             type=_non_negative_int)
     execution_group.add_argument("--max-failures",
             help="Stop execution after the given number of failures.",
             type=_positive_int)
     execution_group.add_argument("--allow-empty-runs",
             help="Do not fail the run if all tests are filtered out",
             action="store_true")
+    execution_group.add_argument("--ignore-fail",
+            dest="ignoreFail",
+            action="store_true",
+            help="Exit with status zero even if some tests fail")
     execution_group.add_argument("--no-indirectly-run-check",
             dest="indirectlyRunCheck",
             help="Do not error if a test would not be run if the user had "
                  "specified the containing directory instead of naming the "
                  "test directly.",
             action="store_false")
 
     selection_group = parser.add_argument_group("Test Selection")

llvm/utils/lit/lit/main.py

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)
 
     has_failure = any(t.isFailure() for t in discovered_tests)
     if has_failure:
+        if opts.ignoreFail:
+            sys.stderr.write("\nExiting with status 0 instead of 1 because "
+                             "'--ignore-fail' was specified.\n")
+        else:
             sys.exit(1)
 
-
 def create_params(builtin_params, user_params):
     def parse(p):
         return p.split('=', 1) if '=' in p else (p, '')
 
     params = dict(builtin_params)
     params.update([parse(p) for p in user_params])
     return params
 

llvm/utils/lit/tests/Inputs/ignore-fail/fail.txt

+RUN: false

llvm/utils/lit/tests/Inputs/ignore-fail/lit.cfg

+import lit.formats
+config.name = 'ignore-fail'
+config.suffixes = ['.txt']
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None

llvm/utils/lit/tests/Inputs/ignore-fail/xfail.txt

+RUN: false
+XFAIL: *

llvm/utils/lit/tests/Inputs/ignore-fail/xpass.txt

+RUN: true
+XFAIL: *

llvm/utils/lit/tests/ignore-fail.py

+# Check that --ignore-fail produces exit status 0 despite various kinds of
+# test failures but doesn't otherwise suppress those failures.
+
+# RUN: not %{lit} -j 1 %{inputs}/ignore-fail | FileCheck %s
+# RUN: %{lit} -j 1 --ignore-fail %{inputs}/ignore-fail | FileCheck %s
+
+# END.
+
+# CHECK: FAIL: ignore-fail :: fail.txt
+# CHECK: UNRESOLVED: ignore-fail :: unresolved.txt
+# CHECK: XFAIL: ignore-fail :: xfail.txt
+# CHECK: XPASS: ignore-fail :: xpass.txt
+
+#      CHECK: Testing Time:
+# CHECK-NEXT:   Expectedly Failed : 1
+# CHECK-NEXT:   Unresolved : 1
+# CHECK-NEXT:   Failed : 1
+# CHECK-NEXT:   Unexpectedly Passed: 1
+#  CHECK-NOT: {{.}}