[lit] Add FILTERED test result category

[yln]

Summary

Track and print the number of tests that were discovered but not
executed due to filtering options:

• --filter (regex filter)
• --max-tests (limits number of tests)
• sharding feature

With this change all discovered tests are accounted for: every
discovered test is included in one of the counts printed in the summary.

[yln]

llvm/utils/lit/lit/main.py

60

Sort all discovered tests (not just the filtered ones) for consistency in test result output. There is a test which covers this.

286

Should this be 'Filtered Out' instead? I think 'Filtered' is clear in this context, but 'Filtered Out' would be "more correct".

[yln]

Any comments on this?

[serge-sans-paille]

llvm/utils/lit/tests/selecting.py

25

It's not a strong opinion, but seeing that, Testing: 2 of 5 tests already contains all the information, and Filtered Tests : 3 looks redundant :-/

[yln]

llvm/utils/lit/tests/selecting.py

25

Note that with the status bar (-sv, which is the default if you do ninja check-...) the "Testing: x of y tests" header goes away when the status bar is cleared and the summary is printed. Without a status bar (e.g., in our CI runs) this header can be thousands of lines above the summary.

This line is also only printed if you filter your tests (--filter, --max-tests, sharding feature). It shouldn't "visually bother" you if you aren not filtering your tests.

In addition, it might not be very useful in the common case, but it might be *very* useful in cases where you are not aware that some filtering is going on (and your test suite passes, because you run less tests than intended).

[jdenny]

llvm/utils/lit/lit/main.py

60

Looking through the test changes in this patch, I don't see what effect the change to this line makes. Could you explain more? Or maybe the existing tests don't reveal it?

114

What's the reason for this change? You cannot have a failure for a test that isn't executed, right?

286

I'd prefer a correct term, both here and in code (filtered_tests, mark_filtered). I think "excluded" is correct.

Moreover, "Filtered" implies tests end up here due to --filter. As you point out in the summary, tests can also end up here due to --max-tests or shards. Perhaps "excluded" is generally a better term to capture all these possibilities without implying just one of them. (I suppose you can ignore this second argument if someone's planning a --exclude.)

llvm/utils/lit/tests/selecting.py

25

It seems useful to me. Thanks for working on it.

Usually a test suite should check that the useful aspect of a feature actually works. Do you think it would be worthwhile to show in at least one test what would separate the two lines when there's no status bar?

Checking in a comment somewhere to summarize the usefulness of this feature (as in your phab comment above) seems worthwhile. This would discourage others from proposing removal of the feature because their use cases happen not to reveal its usefulness.

[yln]

llvm/utils/lit/lit/main.py

60

A few existing tests fail without this because we now pass in discovered_tests in to print_results.

114

That is correct. The two unexecuted result codes "skipped" and "filtered"/(what I am trying to add here) are not failures. So it doesn't matter in this line. Overall I want to replace executed_tests with discovered_tests everywhere.

For example, we have a "fast" and a "normal" CI configuration. The normal one runs a ninja target, and the fast one runs the same ninja target with a filter. I want to include the "filtered" tests in the xml output so that it becomes easier to compare the results of the two configurations.

286

Good suggestion. I am happy with "excluded".

llvm/utils/lit/tests/selecting.py

25

I will try to add a test.

[yln]

Settle on "Excluded tests". Rename filtered_tests to selected_tests.

[yln]

Missed a rename in previous update.

[yln]

llvm/utils/lit/tests/selecting.py

25

Adding a test for the usefulness is difficult: we only use the dynamic progress bar (which deletes its header, i.e., test CHECK-NOT: Testing: ...) when we detect an "interactive" terminal.
The other case is where there is a lot of output between the two lines. Are you okay with omitting a dedicated test for this?

[yln]

@jdenny friendly ping

[jdenny]

LGTM. Thanks.

llvm/utils/lit/tests/selecting.py

25

You could add something representative:

# CHECK-FILTER: Testing: 2 of 5 tests
# CHECK-FILTER: PASS: sub-suite :: test-one.txt (1 of 2)
# CHECK-FILTER: PASS: top-level-suite :: test-one.txt (2 of 2)
# CHECK-FILTER: Excluded Tests : 3

A comment would help people to understand what this represents and thus why they shouldn't see the "Excluded Tests" line as unnecessary verbosity to be removed in a later patch.

llvm/utils/lit/lit/Test.py

 FLAKYPASS   = ResultCode('FLAKYPASS', False)
 XFAIL       = ResultCode('XFAIL', False)
 FAIL        = ResultCode('FAIL', True)
 XPASS       = ResultCode('XPASS', True)
 UNRESOLVED  = ResultCode('UNRESOLVED', True)
 UNSUPPORTED = ResultCode('UNSUPPORTED', False)
 TIMEOUT     = ResultCode('TIMEOUT', True)
 SKIPPED     = ResultCode('SKIPPED', False)
+EXCLUDED    = ResultCode('EXCLUDED', False)
 
 # Test metric values.
 
 class MetricValue(object):
     def format(self):
         """
         format() -> str
 

llvm/utils/lit/lit/main.py

if opts.maxIndividualTestTime is not None:  # `not None` is important (default: 0)
             lit_config.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(lit_config.maxIndividualTestTime,
                         opts.maxIndividualTestTime))
             lit_config.maxIndividualTestTime = opts.maxIndividualTestTime
 
-    filtered_tests = [t for t in discovered_tests if
+    determine_order(discovered_tests, opts.order)
+
+    selected_tests = [t for t in discovered_tests if
                       opts.filter.search(t.getFullName())]
-    if not filtered_tests:
+    if not selected_tests:
         sys.stderr.write('error: filter did not match any tests '
                          '(of %d discovered).  ' % len(discovered_tests))
         if opts.allow_empty_runs:
             sys.stderr.write("Suppressing error because '--allow-empty-runs' "
                              'was specified.\n')
             sys.exit(0)
         else:
             sys.stderr.write("Use '--allow-empty-runs' to suppress this "
                              'error.\n')
             sys.exit(2)
 
-    determine_order(filtered_tests, opts.order)
-
     if opts.shard:
         (run, shards) = opts.shard
-        filtered_tests = filter_by_shard(filtered_tests, run, shards, lit_config)
-        if not filtered_tests:
+        selected_tests = filter_by_shard(selected_tests, run, shards, lit_config)
+        if not selected_tests:
             sys.stderr.write('warning: shard does not contain any tests.  '
                              'Consider decreasing the number of shards.\n')
             sys.exit(0)
 
-    filtered_tests = filtered_tests[:opts.max_tests]
+    selected_tests = selected_tests[:opts.max_tests]
+
+    mark_excluded(discovered_tests, selected_tests)
 
     start = time.time()
-    run_tests(filtered_tests, lit_config, opts, len(discovered_tests))
+    run_tests(selected_tests, lit_config, opts, len(discovered_tests))
     elapsed = time.time() - start
 
     # TODO(yln): eventually, all functions below should act on discovered_tests
     executed_tests = [
-        t for t in filtered_tests if t.result.code != lit.Test.SKIPPED]
+        t for t in selected_tests if t.result.code != lit.Test.SKIPPED]
 
     if opts.time_tests:
         print_histogram(executed_tests)
 
-    print_results(filtered_tests, elapsed, opts)
+    print_results(discovered_tests, elapsed, opts)
 
     if opts.output_path:
         #TODO(yln): pass in discovered_tests
         write_test_results(executed_tests, lit_config, elapsed, opts.output_path)
     if opts.xunit_output_file:
         write_test_results_xunit(executed_tests, opts)
 
     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 executed_tests)
+    has_failure = any(t.isFailure() for t in discovered_tests)
     if has_failure:
         sys.exit(1)
 
 
 def create_params(builtin_params, user_params):
     def parse(p):
         return p.split('=', 1) if '=' in p else (p, '')
 
def filter_by_shard(tests, run, shards, lit_config):
     msg = 'Selecting shard {run}/{shards} = size {sel_tests}/{total_tests} = ' \
           'tests #({shards}*k)+{run} = [{preview}]'.format(
               run=run, shards=shards, sel_tests=len(selected_tests),
               total_tests=len(tests), preview=preview)
     lit_config.note(msg)
     return selected_tests
 
 
+def mark_excluded(discovered_tests, selected_tests):
+    excluded_tests = set(discovered_tests) - set(selected_tests)
+    result = lit.Test.Result(lit.Test.EXCLUDED)
+    for t in excluded_tests:
+        t.setResult(result)
+
+
 def run_tests(tests, lit_config, opts, discovered_tests):
     workers = min(len(tests), opts.workers)
     display = lit.display.create_display(opts, len(tests), discovered_tests,
                                          workers)
 
     def progress_callback(test):
         display.update(test)
         if opts.order == 'failing-first':
def add_result_category(result_code, label):
     assert isinstance(result_code, lit.Test.ResultCode)
     category = (result_code, "%s Tests" % label, label)
     result_codes.append(category)
 
 
 # Status code, summary label, group label
 result_codes = [
     # Passes
+    (lit.Test.EXCLUDED,    'Excluded Tests',      'Excluded'),
     (lit.Test.SKIPPED,     'Skipped Tests',       'Skipped'),
     (lit.Test.UNSUPPORTED, 'Unsupported Tests',   'Unsupported'),
     (lit.Test.PASS,        'Expected Passes',     ''),
     (lit.Test.FLAKYPASS,   'Passes With Retry',   ''),
     (lit.Test.XFAIL,       'Expected Failures',   'Expected Failing'),
     # Failures
     (lit.Test.UNRESOLVED,  'Unresolved Tests',    'Unresolved'),
     (lit.Test.TIMEOUT,     'Individual Timeouts', 'Timed Out'),
     (lit.Test.FAIL,        'Unexpected Failures', 'Failing'),
     (lit.Test.XPASS,       'Unexpected Passes',   'Unexpected Passing')
 ]
 
 
 def print_results(tests, elapsed, opts):
     tests_by_code = {code: [] for (code, _, _) in result_codes}
     for test in tests:
         tests_by_code[test.result.code].append(test)
 
     for (code, _, group_label) in result_codes:
         print_group(code, group_label, tests_by_code[code], opts)
 
     print_summary(tests_by_code, opts.quiet, elapsed)
 
 
 def print_group(code, label, tests, opts):
     if not tests:
         return
     # TODO(yln): FLAKYPASS? Make this more consistent!
-    if code in {lit.Test.SKIPPED, lit.Test.PASS}:
+    if code in {lit.Test.EXCLUDED, lit.Test.SKIPPED, lit.Test.PASS}:
         return
     if (lit.Test.XFAIL == code and not opts.show_xfail) or \
        (lit.Test.UNSUPPORTED == code and not opts.show_unsupported):
         return
     print('*' * 20)
     print('%s Tests (%d):' % (label, len(tests)))
     for test in tests:
         print('  %s' % test.getFullName())

llvm/utils/lit/tests/selecting.py

 # CHECK-BAD-FILTER-ALLOW: Suppressing error because '--allow-empty-runs' was specified.
 
 # 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-FILTER: Excluded Tests : 3
 
 
 # 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-MAX: Excluded Tests : 2
 
 
 # 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 3 --run-shard 1 %{inputs}/discovery >%t.out 2>%t.err
 # RUN: FileCheck --check-prefix=CHECK-SHARD0-ERR < %t.err %s
 # RUN: FileCheck --check-prefix=CHECK-SHARD0-OUT < %t.out %s
 # CHECK-SHARD0-ERR: note: Selecting shard 1/3 = size 2/5 = tests #(3*k)+1 = [1, 4]
 # CHECK-SHARD0-OUT: Testing: 2 of 5 tests
+# CHECK-SHARD0-OUT: Excluded Tests : 3
 #
 # RUN: %{lit} --num-shards 3 --run-shard 2 %{inputs}/discovery >%t.out 2>%t.err
 # RUN: FileCheck --check-prefix=CHECK-SHARD1-ERR < %t.err %s
 # RUN: FileCheck --check-prefix=CHECK-SHARD1-OUT < %t.out %s
 # CHECK-SHARD1-ERR: note: Selecting shard 2/3 = size 2/5 = tests #(3*k)+2 = [2, 5]
 # CHECK-SHARD1-OUT: Testing: 2 of 5 tests
 #
 # RUN: %{lit} --num-shards 3 --run-shard 3 %{inputs}/discovery >%t.out 2>%t.err