Provide --show-xxx flags for all non-failure result codes, just as we
already do for --show-xfail and --show-unsupported.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Since --show subsumes --show-unsupported and --show-xfail, how do people feel about removing those in a separate follow-up change?
llvm/docs/CommandGuide/lit.rst | ||
---|---|---|
166 ↗ | (On Diff #272174) | Drive by fix. Lit does not respect the MAX_FAILURES env var. |
llvm/docs/CommandGuide/lit.rst | ||
---|---|---|
120–121 ↗ | (On Diff #272174) | Could you add an example here with how multiple items should be selected? For example, one might wonder
You might also want to add a sentence to --help text on how to use multiple options. |
llvm/docs/CommandGuide/lit.rst | ||
---|---|---|
120–121 ↗ | (On Diff #272174) | Good point, I will add an example here.
|
Looks like a useful feature. Thanks for working on it.
llvm/docs/CommandGuide/lit.rst | ||
---|---|---|
120–121 ↗ | (On Diff #272174) | 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. |
166 ↗ | (On Diff #272174) | Thanks for taking care of that. |
llvm/utils/lit/lit/cl_arguments.py | ||
200 | What happens if there are user-defined result codes that are spelled the same except for case? |
llvm/docs/CommandGuide/lit.rst | ||
---|---|---|
120–121 ↗ | (On Diff #272174) |
Yes, I think this would also "feel" more natural than the current space-separated list and avoid the oddities about parameter ordering.
What do you think? Do you have a preference or additional ideas? |
llvm/utils/lit/lit/cl_arguments.py | ||
200 | 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. |
llvm/docs/CommandGuide/lit.rst | ||
---|---|---|
120–121 ↗ | (On Diff #272174) | 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. |
llvm/utils/lit/lit/cl_arguments.py | ||
200 | 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. |
I changed our approach to just adding --show-<result-code> flags for all non-failure result codes, just as we already do for --show-xfail and --show-unsupported.
The help entries for the flags look like this:
--show-excluded Show excluded tests (EXCLUDED) --show-skipped Show skipped tests (SKIPPED) --show-unsupported Show unsupported tests (UNSUPPORTED) --show-pass Show passed tests (PASS) --show-flakypass Show passed with retry tests (FLAKYPASS) --show-xfail Show expectedly failed tests (XFAIL)
llvm/utils/lit/lit/cl_arguments.py | ||
---|---|---|
200 |
That's right! |
The last update lost a drive-by fix for MAX_FAILURES. I didn't check to see if that was fixed elsewhere.
Other than the comment suggestion I just added, LGTM.
llvm/utils/lit/lit/cl_arguments.py | ||
---|---|---|
71 | I'd appreciate a comment here to clarify that user-defined result codes are not supported by --show-*. |
Fixed by author: https://github.com/llvm/llvm-project/commit/8cd117c24f48428e01f88cf18480e5af7eb20c0c
Other than the comment suggestion I just added, LGTM.
I will add a comment before committing. Thanks for the quick reviews!
I'd appreciate a comment here to clarify that user-defined result codes are not supported by --show-*.