[lit] Better/earlier errors for empty runs

[yln]

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.

[atrick]

LGTM

[jdenny]

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]

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.

[jdenny]

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]

Address feedback.x

[jdenny]

llvm/utils/lit/lit/run.py

48

Sounds reasonable.

[probinson]

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.

[jdenny]

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.

[probinson]

In D70105#1745051, @jdenny wrote:

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.

[jdenny]

In D70105#1745634, @probinson wrote:

In D70105#1745051, @jdenny wrote:

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.

llvm/utils/lit/lit/cl_arguments.py

 import argparse
 import os
 import shlex
 import sys
 
 import lit.util
 
+# TODO(yln): scan dest, to see if is redundant
 def parse_args():
     parser = argparse.ArgumentParser()
     parser.add_argument('test_paths',
             nargs='+',
             metavar="TEST_PATH",
             help='File or path to include in the test suite')
 
     parser.add_argument("--version",
execution_group.add_argument("--timeout",
             dest="maxIndividualTestTime",
             help="Maximum time to spend running a single test (in seconds). "
                  "0 means no time limit. [Default: 0]",
             type=_non_negative_int) # TODO(yln): --[no-]test-timeout, instead of 0 allowed
     execution_group.add_argument("--max-failures",
             dest="maxFailures",
             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")
 
     selection_group = parser.add_argument_group("Test Selection")
     selection_group.add_argument("--max-tests",
             dest="max_tests",
             metavar="N",
             help="Maximum number of tests to run",
             type=_positive_int)
     selection_group.add_argument("--max-time", #TODO(yln): --timeout

llvm/utils/lit/lit/main.py

litConfig = lit.LitConfig.LitConfig(
         debug = opts.debug,
         isWindows = isWindows,
         params = params,
         config_prefix = opts.configPrefix,
         maxFailures = opts.maxFailures,
         echo_all_commands = opts.echoAllCommands)
 
     tests = lit.discovery.find_tests_for_inputs(litConfig, opts.test_paths)
+    if not tests:
+        sys.stderr.write('Did not disover any tests for provided path(s).\n')
+        sys.exit(2)
 
     # Command line overrides configuration for maxIndividualTestTime.
     if opts.maxIndividualTestTime is not None:  # `not None` is important (default: 0)
         if opts.maxIndividualTestTime != litConfig.maxIndividualTestTime:
             litConfig.note(('The test suite configuration requested an individual'
                 ' test timeout of {0} seconds but a timeout of {1} seconds was'
                 ' requested on the command line. Forcing timeout to be {1}'
                 ' seconds')
                 .format(litConfig.maxIndividualTestTime,
                         opts.maxIndividualTestTime))
             litConfig.maxIndividualTestTime = opts.maxIndividualTestTime
 
     if opts.showSuites or opts.showTests:
         print_suites_or_tests(tests, opts)
         return
 
     numTotalTests = len(tests)
 
     if opts.filter:
         tests = [t for t in tests if opts.filter.search(t.getFullName())]
+        if not tests:
+            sys.stderr.write('Filter did not match any tests '
+                             '(of %d discovered).\n' % numTotalTests)
+            code = 0 if opts.allow_empty_runs else 2
+            sys.exit(code)
 
     determine_order(tests, opts.order)
 
     if opts.shard:
         (run, shards) = opts.shard
         tests = filter_by_shard(tests, run, shards, litConfig)
+        if not tests:
+            sys.stderr.write('Shard does not contain any tests.  Consider '
+                             'decreasing the number of shards.\n')
+            sys.exit(0)
 
     if opts.max_tests:
         tests = tests[:opts.max_tests]
 
     opts.numWorkers = min(len(tests), opts.numWorkers)
 
     start = time.time()
     run_tests(tests, litConfig, opts, numTotalTests)
     elapsed = time.time() - start
 
     print_summary(tests, elapsed, opts)
 
     if opts.output_path:
         write_test_results(tests, litConfig, elapsed, opts.output_path)
     if opts.xunit_output_file:
         write_test_results_xunit(tests, opts)
 
     if litConfig.numErrors:
-        sys.stderr.write('\n%d error(s), exiting.\n' % litConfig.numErrors)
+        sys.stderr.write('\n%d error(s) in tests.\n' % litConfig.numErrors)
         sys.exit(2)
 
     if litConfig.numWarnings:
         sys.stderr.write('\n%d warning(s) in tests.\n' % litConfig.numWarnings)
 
     has_failure = any(t.isFailure() for t in tests)
     if has_failure:
         sys.exit(1)

llvm/utils/lit/lit/run.py

 import multiprocessing
 import os
 import time
 
 import lit.Test
 import lit.util
 import lit.worker
 
 # No-operation semaphore for supporting `None` for parallelism_groups.
 #   lit_config.parallelism_groups['my_group'] = None
 class NopSemaphore(object):
     def acquire(self): pass
     def release(self): pass
 
 def create_run(tests, lit_config, workers, progress_callback, timeout=None):
-    # TODO(yln) assert workers > 0
+    assert workers > 0
     if workers == 1:
         return SerialRun(tests, lit_config, progress_callback, timeout)
     return ParallelRun(tests, lit_config, progress_callback, timeout, workers)
 
 class Run(object):
     """A concrete, configured testing run."""
 
     def __init__(self, tests, lit_config, progress_callback, timeout):
def execute(self):
         stop executing tests.
 
         Returns the elapsed testing time.
 
         Upon completion, each test in the run will have its result
         computed. Tests which were not actually executed (for any reason) will
         be given an UNRESOLVED result.
         """
-        if not self.tests:
-            return 0.0
-
         self.failure_count = 0
         self.hit_max_failures = False
 
         # Larger timeouts (one year, positive infinity) don't work on Windows.
         one_week = 7 * 24 * 60 * 60  # days * hours * minutes * seconds
         timeout = self.timeout or one_week
         deadline = time.time() + timeout
 

llvm/utils/lit/tests/selecting.py

 # RUN: %{lit} %{inputs}/discovery | FileCheck --check-prefix=CHECK-BASIC %s
 # CHECK-BASIC: Testing: 5 tests
 
+
+# Check that we exit with an error if we do not discover any tests, even with --allow-empty-runs.
+#
+# RUN: not %{lit} %{inputs}/nonexistent                    2>&1 | FileCheck --check-prefix=CHECK-BAD-PATH %s
+# RUN: not %{lit} %{inputs}/nonexistent --allow-empty-runs 2>&1 | FileCheck --check-prefix=CHECK-BAD-PATH %s
+# CHECK-BAD-PATH: Did not disover any tests for provided path(s).
+
+# Check that we exit with an error if we filter out all tests, but allow it with --allow-empty-runs.
+#
+# RUN: not %{lit} --filter 'nonexistent'                    %{inputs}/discovery 2>&1 | FileCheck --check-prefix=CHECK-BAD-FILTER %s
+# RUN:     %{lit} --filter 'nonexistent' --allow-empty-runs %{inputs}/discovery 2>&1 | FileCheck --check-prefix=CHECK-BAD-FILTER %s
+# CHECK-BAD-FILTER: Filter did not match any tests (of 5 discovered).
+
+
 # Check that regex-filtering works, is case-insensitive, and can be configured via env var.
 #
 # RUN: %{lit} --filter 'o[a-z]e' %{inputs}/discovery | FileCheck --check-prefix=CHECK-FILTER %s
 # RUN: %{lit} --filter 'O[A-Z]E' %{inputs}/discovery | FileCheck --check-prefix=CHECK-FILTER %s
 # RUN: env LIT_FILTER='o[a-z]e' %{lit} %{inputs}/discovery | FileCheck --check-prefix=CHECK-FILTER %s
 # CHECK-FILTER: Testing: 2 of 5 tests
 
+
 # Check that maximum counts work
 #
 # RUN: %{lit} --max-tests 3 %{inputs}/discovery | FileCheck --check-prefix=CHECK-MAX %s
 # CHECK-MAX: Testing: 3 of 5 tests
 
 
 # Check that sharding partitions the testsuite in a way that distributes the
 # rounding error nicely (i.e. 5/3 => 2 2 1, not 1 1 3 or whatever)

 # RUN: %{lit} --num-shards 100 --run-shard 2 %{inputs}/discovery >%t.out 2>%t.err
 # RUN: FileCheck --check-prefix=CHECK-SHARD-BIG-ERR1 < %t.err %s
 # RUN: FileCheck --check-prefix=CHECK-SHARD-BIG-OUT1 < %t.out %s
 # CHECK-SHARD-BIG-ERR1: note: Selecting shard 2/100 = size 1/5 = tests #(100*k)+2 = [2]
 # CHECK-SHARD-BIG-OUT1: Testing: 1 of 5 tests
 #
 # RUN: %{lit} --num-shards 100 --run-shard 6 %{inputs}/discovery >%t.out 2>%t.err
 # RUN: FileCheck --check-prefix=CHECK-SHARD-BIG-ERR2 < %t.err %s
-# RUN: FileCheck --check-prefix=CHECK-SHARD-BIG-OUT2 < %t.out %s
 # CHECK-SHARD-BIG-ERR2: note: Selecting shard 6/100 = size 0/5 = tests #(100*k)+6 = []
-# CHECK-SHARD-BIG-OUT2: Testing: 0 of 5 tests
+# CHECK-SHARD-BIG-ERR2: Shard does not contain any tests.  Consider decreasing the number of shards.
 #
 # RUN: %{lit} --num-shards 100 --run-shard 50 %{inputs}/discovery >%t.out 2>%t.err
 # RUN: FileCheck --check-prefix=CHECK-SHARD-BIG-ERR3 < %t.err %s
-# RUN: FileCheck --check-prefix=CHECK-SHARD-BIG-OUT3 < %t.out %s
 # CHECK-SHARD-BIG-ERR3: note: Selecting shard 50/100 = size 0/5 = tests #(100*k)+50 = []
-# CHECK-SHARD-BIG-OUT3: Testing: 0 of 5 tests
+# CHECK-SHARD-BIG-ERR3: Shard does not contain any tests.  Consider decreasing the number of shards.
 
 
 # Check that range constraints are enforced
 #
 # RUN: not %{lit} --num-shards 0 --run-shard 2 %{inputs}/discovery >%t.out 2>%t.err
 # RUN: FileCheck --check-prefix=CHECK-SHARD-ERR < %t.err %s
 # CHECK-SHARD-ERR: error: argument --num-shards: requires positive integer, but found '0'
 #
 # RUN: not %{lit} --num-shards 3 --run-shard 4 %{inputs}/discovery >%t.out 2>%t.err
 # RUN: FileCheck --check-prefix=CHECK-SHARD-ERR2 < %t.err %s
 # CHECK-SHARD-ERR2: error: --run-shard must be between 1 and --num-shards (inclusive)