[PATCH] D77819: [lit] Add SKIPPED test result category

[Joel E. Denny via Phabricator via llvm-commits]

jdenny marked an inline comment as done.
jdenny added a comment.

In D77819#1974786 <https://reviews.llvm.org/D77819#1974786>, @yln wrote:

> > Can skipped tests be covered in lit's test suite?  I'm not sure how easy it is to do it in a portable manner, but platform-specific coverage is better than nothing.
>
> Added a test for the overall lit timeout (`--max-time`) which previously didn't have one, which let's us observe the "skipped test" logic.  I am not sure how to add one for the user interrupt [Ctrl+C].


It think it would be messy: run the lit test suite in the background redirecting to a file, record the PID, poll that file in the foreground until expected passes appear, and then kill -SIGINT.  Very slow tests should be skipped.  It works from my bash shell, but I'm not sure about portability.

For now, your `-max-failures` tests are a race-free, portable way to exercise at least one code path for skipped tests, so I would not object if you felt that was sufficient.



================
Comment at: llvm/utils/lit/lit/main.py:283
         return
-    if code == lit.Test.PASS:
+    # TODO(yln): FLAKYPASS? Make this more consistent!
+    if code in {lit.Test.SKIPPED, lit.Test.PASS}:
----------------
yln wrote:
> jdenny wrote:
> > Does this todo mean you don't like the name SKIPPED?
> No, I just noticed that handling of FLAKYPASS is missing here and I want to address it as a separate change.
Got it.  Thanks.


================
Comment at: llvm/utils/lit/lit/main.py:284
-       (lit.Test.UNSUPPORTED == code and not opts.show_unsupported) or \
-       (lit.Test.UNRESOLVED == code and (opts.max_failures is not None)):
         return
----------------
yln wrote:
> jdenny wrote:
> > There's a change in the effect of `opts.max_failures` on both skipped tests and the remaining unresolved tests.
> > 
> > It seems like that should be mentioned in the patch summary.
> > 
> > Can at least the latter change be covered in lit's test suite?
> UNRESOLVED tests are failures where the test failed due to an "infrastructure" issue, e.g., failure to parse a `REQUIRES:` line.
> 
> Previously all unexecuted tests (unexecuted for whatever reason: infrastructure failures, overall lit timeout, user interrupt, --max-tests) were all marked UNRESOLVED. So both logical groups for unexecuted tests: "skipped" and "unresolved" were given the same label and treated as failures.  However, a "skipped" shouldn't imply failure.
> 
> I think the check for `opts.max_failures` here was just an ad-hoc way to dry to deal with mixing both skipped and failed tests into the UNRESOLVED category: when `--max-failures` was specified and we stop executing because of it we mark all remaining tests as UNRESOLVED. However we shouldn't print those as failures here.
> 
> Anyways, things are more consistent now.  We have a proper category for "skipped" (not a failure) and the interaction between max_failures and UNRESOLVED has been removed.
Thanks for explaining.


================
Comment at: llvm/utils/lit/tests/max-time.py:3
+#
+# RUN: %{lit} %{inputs}/max-time --max-time=1 2>&1  |  FileCheck %s
+
----------------
On heavily loaded test systems, is there a chance of a race here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77819/new/

https://reviews.llvm.org/D77819