[prev in list] [next in list] [prev in thread] [next in thread] 

List:       llvm-commits
Subject:    [PATCH] D78078: [lit] Add FILTERED test result category
From:       "Joel E. Denny via Phabricator via llvm-commits" <llvm-commits () lists ! llvm ! org>
Date:       2020-04-30 22:39:06
Message-ID: b628e19b75dc47b5437c6a94995c8762 () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
llvm-commits mailing list
llvm-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits


[prev in list] [next in list] [prev in thread] [next in thread]