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 | Drive by fix. Lit does not respect the MAX_FAILURES env var. |
llvm/docs/CommandGuide/lit.rst | ||
---|---|---|
120–121 | 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 | 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 | 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 | Thanks for taking care of that. | |
llvm/utils/lit/lit/cl_arguments.py | ||
204 | What happens if there are user-defined result codes that are spelled the same except for case? |
llvm/docs/CommandGuide/lit.rst | ||
---|---|---|
120–121 |
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 | ||
204 | 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 | 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 | ||
204 | 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 | ||
---|---|---|
204 |
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 | ||
---|---|---|
76 | 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!
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.