[PATCH] D78078: [lit] Add FILTERED test result category

[Julian Lettner via Phabricator via llvm-commits]

yln marked 4 inline comments as done.
yln added inline comments.


================
Comment at: llvm/utils/lit/lit/main.py:60
 
+    determine_order(discovered_tests, opts.order)
+
----------------
jdenny wrote:
> yln wrote:
> > Sort all discovered tests (not just the filtered ones) for consistency in test result output.  There is a test which covers this.
> 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?
A few existing tests fail without this because we now pass in `discovered_tests` in to `print_results`.


================
Comment at: llvm/utils/lit/lit/main.py:116
 
-    has_failure = any(t.isFailure() for t in executed_tests)
+    has_failure = any(t.isFailure() for t in discovered_tests)
     if has_failure:
----------------
jdenny wrote:
> What's the reason for this change?  You cannot have a failure for a test that isn't executed, right?
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.


================
Comment at: llvm/utils/lit/lit/main.py:273
 all_codes = [
+    (lit.Test.FILTERED,    'Filtered Tests',    'Filtered'),
     (lit.Test.SKIPPED,     'Skipped Tests',     'Skipped'),
----------------
jdenny wrote:
> yln wrote:
> > Should this be 'Filtered Out' instead?  I think 'Filtered' is clear in this context, but 'Filtered Out' would be "more correct".
> 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`.)
Good suggestion.  I am happy with "excluded".


================
Comment at: llvm/utils/lit/tests/selecting.py:25
 # CHECK-FILTER: Testing: 2 of 5 tests
+# CHECK-FILTER: Filtered Tests : 3
 
----------------
jdenny wrote:
> yln wrote:
> > serge-sans-paille wrote:
> > > 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 :-/
> > 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).
> 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.
I will try to add a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78078