This is an archive of the discontinued LLVM Phabricator instance.

[lit] Better/earlier errors for empty runs
ClosedPublic

Authored by yln on Nov 11 2019, 4:55 PM.

Details

Summary

Fail early, when we discover no tests at all, or filter out all of them.

There is also --allow-empty-runs to disable test to allow workflows
like LIT_FILTER=abc ninja check-all. Apparently check-all invokes
lit multiple times if certain projects are enabled, which would produce
unwanted "empty runs". Specify via LIT_OPTS=--allow-empty-runs.

There are 3 causes for empty runs:

  1. No tests discovered. This is always an error. Fix test suite config or command line.
  2. All tests filtered out. This is an error by default, but can be suppressed via --alow-empty-runs. Should prevent accidentally passing empty runs, but allow the workflow above.
  3. The number of shards is greater than the number of tests. Currently, this is never an error. Personally, I think we should consider making this an error by default; if this happens, you are doing something wrong. I added a warning but did not change the behavior, since this warrants more discussion.

Diff Detail

Event Timeline

yln created this revision.Nov 11 2019, 4:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2019, 4:55 PM
atrick accepted this revision.Nov 11 2019, 5:32 PM

LGTM

This revision is now accepted and ready to land.Nov 11 2019, 5:32 PM
llvm/utils/lit/lit/cl_arguments.py
8

Is this comment relevant to this patch?

llvm/utils/lit/lit/main.py
71

When --allow-empty-runs is not already specified, could we have this diagnostic suggest it? Otherwise, could we have it report that it was specified?

llvm/utils/lit/lit/run.py
48

Would it be wortwhile to turn this guard into an assert rather than just removing it?

yln marked 6 inline comments as done.Nov 12 2019, 8:09 AM
yln added inline comments.
llvm/utils/lit/lit/main.py
71

Good point. Will do.

llvm/utils/lit/lit/run.py
48

This function works for zero tests (although it then creates a process pool for no reason). It's just that you can't create a process pool with zero workers and we use min(test_count, cpu_cores) to determine the number of workers. We have an assert for this above.

The number of shards is greater than the number of tests. Currently, this is never an error. Personally, I think we should consider making this an error by default; if this happens, you are doing something wrong. I added a warning but did not change the behavior, since this warrants more discussion.

I haven't used shards much, so I don't know what use cases are relevant.

My guess is that LIT_FILTER would be a reason why you might end up with more shards than tests: you already have some distributed test run configuration with shards, and you temporarily add LIT_FILTER to focus on specific failing tests. It seems reasonable that --allow-empty-runs would again suppress the error. I don't know whether this use case arises in practice.

Thanks for working on all this.

yln updated this revision to Diff 228902.Nov 12 2019, 8:56 AM
yln marked 2 inline comments as done.

Address feedback.x

jdenny marked an inline comment as done.Nov 12 2019, 8:57 AM
jdenny added inline comments.
llvm/utils/lit/lit/run.py
48

Sounds reasonable.

jdenny accepted this revision.Nov 12 2019, 9:00 AM
This revision was automatically updated to reflect the committed changes.

This patch fails on Windows when built using the Visual Studio 2017 builder. The new RUN lines in selecting.py for the "nonexistent" case will produce a diagnostic complaining about a missing 'build_mode' substitution:

$ ":" "RUN: at line 7"
$ "not" "env" "-u" "FILECHECK_OPTS" "-u" "FILECHECK_DUMP_INPUT_ON_FAILURE" "C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python36_64\python.exe" "D:/Data/upstream/llvm-project/llvm\utils\lit\lit.py" "D:/Data/upstream/mbuild/utils/lit/tests\Inputs/nonexistent"
$ "FileCheck" "--check-prefix=CHECK-BAD-PATH" "D:\Data\upstream\mbuild\utils\lit\tests\selecting.py"
# command stderr:
D:\Data\upstream\mbuild\utils\lit\tests\selecting.py:9:19: error: CHECK-BAD-PATH: expected string not found in input
# CHECK-BAD-PATH: Did not disover any tests for provided path(s).
                  ^
<stdin>:1:1: note: scanning from here
lit.py: D:\Data\upstream\mbuild\utils\lit\tests\lit.site.cfg:18: fatal: unable to find 'build_mode' parameter, use '--param=build_mode=VALUE'
^
<stdin>:1:62: note: possible intended match here
lit.py: D:\Data\upstream\mbuild\utils\lit\tests\lit.site.cfg:18: fatal: unable to find 'build_mode' parameter, use '--param=build_mode=VALUE'
                                                             ^

error: command failed with exit status: 1

This has to have something to do with Visual Studio being a multi-config builder (you specify Debug/Release/etc at build time, not CMake time) as opposed to ninja's single-config operation. I'm afraid I'm not much help beyond that, though.

I think it's using utils\lit\tests\lit.site.cfg, which produces the error, when it would normally use utils/lit/tests/Inputs/nonexistent/lit.cfg. Creating even an empty version of that file would probably fix this.

I think it's using utils\lit\tests\lit.site.cfg, which produces the error, when it would normally use utils/lit/tests/Inputs/nonexistent/lit.cfg. Creating even an empty version of that file would probably fix this.

Hmmm then the directory wouldn't be nonexistent... I added an extra directory level in D70239, see what you think.

I think it's using utils\lit\tests\lit.site.cfg, which produces the error, when it would normally use utils/lit/tests/Inputs/nonexistent/lit.cfg. Creating even an empty version of that file would probably fix this.

Hmmm then the directory wouldn't be nonexistent...

Good point. Especially now that we know it can make a difference, it's probably worthwhile keeping that part. I wonder if we should additionally check the case of an empty existing directory, which would also fail as originally intended here.

I added an extra directory level in D70239, see what you think.

I'm following up further there.