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

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

jdenny accepted this revision.
jdenny marked an inline comment as done.
jdenny added a comment.
This revision is now accepted and ready to land.

LGTM.  Thanks.



================
Comment at: llvm/utils/lit/tests/selecting.py:25
 # CHECK-FILTER: Testing: 2 of 5 tests
+# CHECK-FILTER: Filtered Tests : 3
 
----------------
yln wrote:
> yln wrote:
> > 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.
> 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?  
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78078