[PATCH] D82233: [lit] Add --show command line option

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

jdenny marked an inline comment as done.
jdenny added inline comments.


================
Comment at: llvm/docs/CommandGuide/lit.rst:120-121
+
+ Show the names of the specified tests. Choose from:
+ all, excluded, skipped, unsupported, pass, flakypass, xfail.
+
----------------
yln wrote:
> jdenny wrote:
> > yln wrote:
> > > varungandhi-apple wrote:
> > > > Could you add an example here with how multiple items should be selected? For example, one might wonder
> > > > 
> > > > 1. Can you do something like `--show skipped,xfail`?
> > > > 2. What are the semantics of `--show skipped --show xfail` (does it mean skipped AND xfail or does it only mean xfail)?
> > > > 
> > > > You might also want to add a sentence to `--help` text on how to use multiple options.
> > > Good point, I will add an example here.
> > > 1. See test.
> > > 2. Last one wins.
> > > 3. The `--help` part is auto-generated by argparse (because we use the `choices` parameter).
> > Why not a comma-separated list?
> > 
> > For example, I downloaded your patch and tried this:
> > 
> > ```
> > $ ./bin/llvm-lit test/Support
> > -- Testing: 2 tests, 2 workers --
> > PASS: LLVM :: Support/check-default-options.txt (1 of 2)
> > PASS: LLVM :: Support/interrupts.test (2 of 2)
> > 
> > Testing Time: 0.11s
> >   Passed: 2
> > $ ./bin/llvm-lit --show all test/Support
> > usage: lit [-h] [--version] [-j N] [--config-prefix NAME] [-D NAME=VAL] [-q]
> >            [-s] [-v] [-vv] [-a] [-o PATH] [--no-progress-bar]
> >            [--show-unsupported] [--show-xfail]
> >            [--show {all,excluded,skipped,unsupported,pass,flakypass,xfail} [{all,excluded,skipped,unsupported,pass,flakypass,xfail} ...]]
> >            [--path PATH] [--vg] [--vg-leak] [--vg-arg ARG] [--time-tests]
> >            [--no-execute] [--xunit-xml-output XUNIT_XML_OUTPUT]
> >            [--timeout MAXINDIVIDUALTESTTIME] [--max-failures MAX_FAILURES]
> >            [--allow-empty-runs] [--max-tests N] [--max-time N] [--shuffle]
> >            [-i] [--filter REGEX] [--num-shards M] [--run-shard N] [--debug]
> >            [--show-suites] [--show-tests] [--show-used-features]
> >            TEST_PATH [TEST_PATH ...]
> > lit: error: argument --show: invalid choice: 'test/Support' (choose from 'all', 'excluded', 'skipped', 'unsupported', 'pass', 'flakypass', 'xfail')
> > ```
> > 
> > The usage message shows that `--show` can be used before `TEST_PATH`, but it doesn't work unless I add `--` in between, which isn't listed in the options.  Alternatively, I can specify `--show` after `TEST_PATH`, but that also isn't mentioned in the usage summary above.
> > 
> > If the values were comma-separated, this wouldn't be an issue.
> > Why not a comma-separated list?
> 
> Yes, I think this would also "feel" more natural than the current space-separated list and avoid the oddities about parameter ordering.
> Surprisingly, argparse does not support this.  I see two options:
> 
> * Implement this ourselves (it's actually not too bad): https://stackoverflow.com/a/60205263/271968
> * We could generate flags, e.g., `--show-xfail`, for each result code. Pro: this would be in line with the existing flags for unsupported and fail. Con: Extending this scheme to accept user-defined codes would be harder.
> 
> What do you think?  Do you have a preference or additional ideas?
One aspect I like about the second option (`--show-*`) is that the semantics of multiple occurrences are more intuitive.  The first option (comma-separated list) begs the question of accummulate vs. override.  But this is a minor point.

If I had to choose right now, I'd go with the second option, primarily because it's consistent with the existing interface.  However, I don't have strong feelings about this, so I could be happy with either.









================
Comment at: llvm/utils/lit/lit/cl_arguments.py:204
+        else:
+           opts.shown_codes.add(lit.Test.ResultCode._instances[code.upper()])
 
----------------
yln wrote:
> jdenny wrote:
> > What happens if there are user-defined result codes that are spelled the same except for case?
> Unfortunately, this can't be used (yet) to specify user-defined result codes at all.  User codes are usually registered in config files and this code executes before we evaluate configs, i.e., it will print `argument --show: invalid choice: 'user-code' (choose from ...)`
> 
> If we think it's worth it then we could push "choice validation" to a later point after we processed the user configs.
I don't know that `--show` support for user-defined result codes needs to be implemented in this patch.

In that case, the case-insensitivity issue I raised is not relevant yet, right?  That can be addressed later then.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82233