This is an archive of the discontinued LLVM Phabricator instance.

[lit] Add --show-xxx command line options
ClosedPublic

Authored by yln on Jun 19 2020, 1:38 PM.

Details

Summary

Provide --show-xxx flags for all non-failure result codes, just as we
already do for --show-xfail and --show-unsupported.

Diff Detail

Event Timeline

yln created this revision.Jun 19 2020, 1:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2020, 1:38 PM
yln marked an inline comment as done.Jun 19 2020, 1:45 PM

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

  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.

yln marked an inline comment as done.Jun 22 2020, 9:20 AM
yln added inline comments.
llvm/docs/CommandGuide/lit.rst
120–121 ↗(On Diff #272174)

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).
In D82233#2104573, @yln wrote:

Since --show subsumes --show-unsupported and --show-xfail, how do people feel about removing those in a separate follow-up change?

I like this a lot. And +1 for removing the redundant options.

yln updated this revision to Diff 272557.Jun 22 2020, 3:32 PM

Update docs to show how to specify multiple result codes.

yln marked an inline comment as done.Jun 22 2020, 3:33 PM
dexonsmith resigned from this revision.Jun 24 2020, 3:13 PM
jdenny marked an inline comment as done.Jun 26 2020, 1:49 PM

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?

yln marked 3 inline comments as done.Jul 8 2020, 11:35 AM
yln added inline comments.
llvm/docs/CommandGuide/lit.rst
120–121 ↗(On Diff #272174)

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?

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.

jdenny marked an inline comment as done.Jul 8 2020, 12:43 PM
jdenny added inline comments.
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.

yln updated this revision to Diff 276593.Jul 8 2020, 4:15 PM
yln marked an inline comment as done.

Add -show-xxx for all result codes in addtion to unsupported and xfail.

yln retitled this revision from [lit] Add --show command line option to [lit] Add --show-xxx command line options.Jul 8 2020, 4:16 PM
yln edited the summary of this revision. (Show Details)
yln marked an inline comment as done.Jul 8 2020, 4:23 PM

@jdenny

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

In that case, the case-insensitivity issue I raised is not relevant yet, right?

That's right!

jdenny accepted this revision.Jul 8 2020, 4:45 PM

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-*.

This revision is now accepted and ready to land.Jul 8 2020, 4:45 PM
yln added a comment.Jul 8 2020, 4:58 PM

The last update lost a drive-by fix for MAX_FAILURES. I didn't check to see if that was fixed elsewhere.

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!

This revision was automatically updated to reflect the committed changes.