Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make libtest flags more discoverable #12494

Closed
jyn514 opened this issue Aug 14, 2023 · 9 comments · Fixed by #13448
Closed

Make libtest flags more discoverable #12494

jyn514 opened this issue Aug 14, 2023 · 9 comments · Fixed by #13448
Labels
A-console-output Area: Terminal output, colors, progress bar, etc. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-test S-triage Status: This issue is waiting on initial triage.

Comments

@jyn514
Copy link
Member

jyn514 commented Aug 14, 2023

Problem

The libtest unit test runner has a lot of flags. Cargo allows passing these through with cargo test --lib -- . This is not super easy to discover - cargo test --help does say "Run cargo test -- --help for test binary options" at the end, but if you don't know that libtest and cargo have different flags, the error messages aren't great:

; cargo +nightly test --ignored
error: unexpected argument '--ignored' found

  tip: a similar argument exists: '--ignore-rust-version'

Usage: cargo test --ignore-rust-version [TESTNAME] [-- [args]...]

For more information, try '--help'.

Proposed Solution

Ideally, cargo would know which flags libtest supports and suggest passing those through:

error: unexpected argument '--ignored' found

  tip: libtest supports the `--ignored` argument; try `cargo test -- --ignored`
  tip: a similar argument exists: '--ignore-rust-version'

Usage: cargo test --ignore-rust-version [TESTNAME] [-- [args]...]

Notes

Right now, it's kind of hard to parse the --help output of libtest programmatically, so cargo would either have to hack a parser together or hardcode the options, neither of which is particularly maintainable. Ideally --help --format json would output a structured list that cargo can easily parse (I can open an upstream issue in rust-lang/rust if you're interested in pursuing this) and then idk embed it at build time or something like that.

My motivation for opening this issue is that I want to suggest --ignored in libtest if 1 or more tests are ignored, but right now that will be kind of confusing for people using cargo because it's a libtest flag and not a cargo test flag.

@jyn514 jyn514 added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Aug 14, 2023
@epage
Copy link
Contributor

epage commented Aug 14, 2023

clap-rs/clap#4706 would allow cargo to add flags that only exist to produce a specific error message. We've been talking about using it to help in other cases.

I think using that could work

  • The fact that this case errors shows that by adding one of these flags, we won't make things worse.
  • libtest's CLI doesn't change all that much
  • We won't know about harness = false but if a harness doesn't support the flag (which imo it should), there is little harm. If the test harness supports extra flags, then we just don't help in those cases.
  • This is much different than parsing arbitrary command-lines of other programs because that requires full knowledge of the CLI rather than just the subset that this can get away with if they drift apart

@jyn514
Copy link
Member Author

jyn514 commented Aug 15, 2023

hmm, did you mean to link a different issue? #4706 doesn't look related.

@epage
Copy link
Contributor

epage commented Aug 15, 2023

sigh second time in the last week I didn't update a link for being cross-repo.

Fixed

@weihanglo
Copy link
Member

There is another issue making cargo test -- --help hard to read — #10392.

@jyn514
Copy link
Member Author

jyn514 commented Aug 16, 2023

There is another issue making cargo test -- --help hard to read — #10392.

Suggesting test --lib -- --help instead of test -- --help in the cargo --help output might make that less of an issue.

@epage
Copy link
Contributor

epage commented Aug 16, 2023

Does clap-rs/clap#5075 look like it'll fit the needs of this?

@jyn514
Copy link
Member Author

jyn514 commented Sep 20, 2023

Does clap-rs/clap#5075 look like it'll fit the needs of this?

I think that's a question for the cargo team, not for me. I didn't look at the error message but in general it looks like that API requires hard coding all the names of the libtest flags 🤷

@epage epage added Command-test A-console-output Area: Terminal output, colors, progress bar, etc. labels Feb 16, 2024
@epage
Copy link
Contributor

epage commented Feb 16, 2024

#11702 links out to a lot of PRs where we did things like this. Specifically #12693 is an example of an arbitrary suggestion.

However, looking at this further, I wonder if I would consider this a bug in clap. Generally clap suggests to put the argument after a -- and I'm not sure why it isn't doing that in this case.

@epage
Copy link
Contributor

epage commented Feb 16, 2024

Note that this is specific to --ignored because of the prefix checks for the suggested alternative prevent the -- tip from showing up.

For example:

error: unexpected argument '--show-output' found

  tip: to pass '--show-output' as a value, use '-- --show-output'

Usage: cargo test [OPTIONS] [TESTNAME] [-- [ARGS]...]

For more information, try '--help'.

There isn't a way in clap to turn this off, so making this like #11702 would lead to providing the same suggestion two different ways.

epage added a commit to epage/clap that referenced this issue Feb 16, 2024
Inspired by rust-lang/cargo#12494.
Part of this is that our "did you mean" does prefix checks so it can be
overly aggressive in providing suggestions.
To avoid providing needless suggestions I limited this change to `last`
/ `trailing_var_arg` as those convey that `--` is more likely a valid
suggestion.
bors added a commit that referenced this issue Feb 16, 2024
fix(test): Suggest `--` for libtest arguments

We already do this so long as the argument doesn't look like a `cargo test` argument (e.g. `--show-output`)
but `--ignored` looks like `--ignore-rust-version` do the the suggestion algorithms prefix checks.

Before
```
error: unexpected argument '--ignored' found

  tip: a similar argument exists: '--ignore-rust-version'

Usage: cargo test --ignore-rust-version [TESTNAME] [-- [ARGS]...]

For more information, try '--help'.
```
After
```
error: unexpected argument '--ignored' found

  tip: a similar argument exists: '--ignore-rust-version'
  tip: to pass '--ignored' as a value, use '-- --ignored'

Usage: cargo test --ignore-rust-version [TESTNAME] [-- [ARGS]...]

For more information, try '--help'.
```

This was fixed in clap-rs/clap#5356 and we just need to update to take advantage of it.

Fixes #12494
@bors bors closed this as completed in 6f93fa7 Feb 16, 2024
stupendoussuperpowers pushed a commit to stupendoussuperpowers/cargo that referenced this issue Feb 28, 2024
We already do this so long as the argument doesn't look like a
`cargo test` argument (e.g. `--show-output`)
but `--ignored` looks like `--ignore-rust-version` do the the suggestion
algorithms prefix checks.

Before
```
error: unexpected argument '--ignored' found

  tip: a similar argument exists: '--ignore-rust-version'

Usage: cargo test --ignore-rust-version [TESTNAME] [-- [ARGS]...]

For more information, try '--help'.
```
After
```
error: unexpected argument '--ignored' found

  tip: a similar argument exists: '--ignore-rust-version'
  tip: to pass '--ignored' as a value, use '-- --ignored'

Usage: cargo test --ignore-rust-version [TESTNAME] [-- [ARGS]...]

For more information, try '--help'.
```

Fixes rust-lang#12494
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-console-output Area: Terminal output, colors, progress bar, etc. C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-test S-triage Status: This issue is waiting on initial triage.
Projects
Development

Successfully merging a pull request may close this issue.

3 participants