Clarify in `--help` that `cargo test --all-targets` excludes doctests #12422

obi1kenobi · 2023-07-31T16:33:52Z

Per my proposal here: #6669 (comment)

I tried to keep the edit minimalistic to match the surrounding style.

If the maintainers are amenable to it, I think it could also be useful to do one or more of:

Offer concrete guidance on what to do to run actually-all tests (--all-targets then separately --doc).
Link to the issue at: cargo test --all-targets does not run doc tests #6669
Mention that cargo test without --all-targets runs doctests by default, which seems not immediately obvious.

I'd be happy to attempt to add any of the above that the maintainers feel would be a good fit here.

rustbot · 2023-07-31T16:33:56Z

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

ehuss · 2023-07-31T17:28:38Z

I left a comment over at #6669 (comment). This isn't a review on this PR (I'll leave that up to epage), but just a passing comment that saying "except doctests" might imply that a doctest is a target (which it isn't). I would probably word this like (does not include doctests) or something like that, but I am splitting hairs and the exact wording isn't important to me, just a suggestion if you like it better.

I do agree it is helpful to clarify, so thank you for opening this!

epage · 2023-07-31T17:41:39Z

If the maintainers are amenable to it, I think it could also be useful to do one or more of:

I think at least clarifying the webpage / man page / cargo help test in the areas I highlighted would be good. We could fix the current miscommunication and handle this at the same time.

As for the ``-h / --help, we generally keep that brief (and I think `cargo add` is the only command with separate `-h` and `--help` on top of the man page)

epage · 2023-07-31T17:43:40Z

src/bin/cargo/commands/test.rs

@@ -34,7 +34,7 @@ pub fn cli() -> Command {
            "Test all tests",
            "Test only the specified bench target",
            "Test all benches",
-            "Test all targets",
+            "Test all targets except doctests",


I would probably word this like (does not include doctests) or something like that, but I am splitting hairs and the exact wording isn't important to me, just a suggestion if you like it better.

I like the suggestion for it being paranthetical

I think I also prefer wording besides "except" makes it sound like we are actively excluding it

does not include is ok

precludes might also work

I can switch to does not include with a parenthetical.

Though, one thing that phrasing doesn't address or even hint at is that cargo test without this flag does run doctests. From that perspective, adding this flag does have the effect of actively excluding doctests, even though I understand the difference in intent there.

obi1kenobi · 2023-07-31T18:13:35Z

I think at least clarifying the webpage / man page / cargo help test in the areas #6669 (comment) would be good. We could fix the current miscommunication and handle this at the same time.

I don't feel like I understand the finer points of targets well enough to tackle the "docs text implies doctests are a target" issue in the links you shared, but I did open another PR to add a clarification for the --all-targets flag on that page: #12423

obi1kenobi · 2023-07-31T18:53:23Z

I'm running into some linking-related problem where even though I have pkg-config and OpenSSL installed as recommended in the README, cargo is still failing to build.

I tried to edit the snapshot test by hand so as not to block this PR on the issues with my local setup.

weihanglo · 2023-07-31T19:27:51Z

I'm running into some linking-related problem where even though I have pkg-config and OpenSSL installed as recommended in the README, cargo is still failing to build.

Off topic but I am interested in this. If you are okay with it please share the detail on zulip or in a new issue :)

Per my proposal here: rust-lang#6669 (comment) I tried to keep the edit minimalistic to match the surrounding style. If the maintainers are amenable to it, I think it could also be useful to do one or more of: - Offer concrete guidance on what to do to run actually-all tests (`--all-targets` then separately `--doc`). - Link to the issue at: rust-lang#6669 - Mention that `cargo test` without `--all-targets` runs doctests by default, which seems not immediately obvious. I'd be happy to attempt to add any of the above that the maintainers feel would be a good fit here.

epage · 2023-07-31T20:14:52Z

Thanks!

@bors r+

bors · 2023-07-31T20:14:54Z

📌 Commit 2f59b20 has been approved by epage

It is now in the queue for this repository.

bors · 2023-07-31T20:15:00Z

⌛ Testing commit 2f59b20 with merge 8d1d20d...

bors · 2023-07-31T21:02:46Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 8d1d20d to master...

Update cargo 8 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..6dc1deaddf62c7748c9097c7ea88e9ec77ff1a1a 2023-07-31 00:26:46 +0000 to 2023-08-02 00:23:54 +0000 - `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429) - ci: rewrite bump check and respect semver (rust-lang/cargo#12395) - fix(update): Tweak CLI behavior (rust-lang/cargo#12428) - chore(deps): update compatible (rust-lang/cargo#12426) - Display crate version on timings graph (rust-lang/cargo#12420) - chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427) - Use thiserror for credential provider errors (rust-lang/cargo#12424) - Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422) r? `@ghost`

Update cargo 8 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..6dc1deaddf62c7748c9097c7ea88e9ec77ff1a1a 2023-07-31 00:26:46 +0000 to 2023-08-02 00:23:54 +0000 - `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429) - ci: rewrite bump check and respect semver (rust-lang/cargo#12395) - fix(update): Tweak CLI behavior (rust-lang/cargo#12428) - chore(deps): update compatible (rust-lang/cargo#12426) - Display crate version on timings graph (rust-lang/cargo#12420) - chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427) - Use thiserror for credential provider errors (rust-lang/cargo#12424) - Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422) r? ``@ghost``

Update cargo 10 commits in c91a693e7977e33a1064b63a5daf5fb689f01651..020651c52257052d28f6fd83fbecf5cfa1ed516c 2023-07-31 00:26:46 +0000 to 2023-08-02 16:00:37 +0000 - Update rustix to 0.38.6 (rust-lang/cargo#12436) - replace `master` branch by default branch in documentation (rust-lang/cargo#12435) - `#[allow(internal_features)]` in RUSTC_BOOTSTRAP test (rust-lang/cargo#12429) - ci: rewrite bump check and respect semver (rust-lang/cargo#12395) - fix(update): Tweak CLI behavior (rust-lang/cargo#12428) - chore(deps): update compatible (rust-lang/cargo#12426) - Display crate version on timings graph (rust-lang/cargo#12420) - chore(deps): update alpine docker tag to v3.18 (rust-lang/cargo#12427) - Use thiserror for credential provider errors (rust-lang/cargo#12424) - Clarify in `--help` that `cargo test --all-targets` excludes doctests (rust-lang/cargo#12422) r? `@ghost`

rustbot assigned epage Jul 31, 2023

rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-test S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023

epage reviewed Jul 31, 2023

View reviewed changes

obi1kenobi force-pushed the patch-2 branch from f99d661 to 2f59b20 Compare July 31, 2023 20:10

epage changed the title ~~Mention that cargo test --all-targets excludes doctests~~ Clarify in --help that cargo test --all-targets excludes doctests Jul 31, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023

bors merged commit 8d1d20d into rust-lang:master Jul 31, 2023

obi1kenobi deleted the patch-2 branch July 31, 2023 21:04

bors mentioned this pull request Jul 31, 2023

fix(cli): Make --help easier to browse #11905

Merged

weihanglo mentioned this pull request Aug 2, 2023

Update cargo rust-lang/rust#114364

Merged

ehuss added this to the 1.73.0 milestone Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify in `--help` that `cargo test --all-targets` excludes doctests #12422

Clarify in `--help` that `cargo test --all-targets` excludes doctests #12422

obi1kenobi commented Jul 31, 2023

rustbot commented Jul 31, 2023

ehuss commented Jul 31, 2023

epage commented Jul 31, 2023

epage Jul 31, 2023

obi1kenobi Jul 31, 2023

obi1kenobi commented Jul 31, 2023

obi1kenobi commented Jul 31, 2023

weihanglo commented Jul 31, 2023

epage commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

Clarify in --help that cargo test --all-targets excludes doctests #12422

Clarify in --help that cargo test --all-targets excludes doctests #12422

Conversation

obi1kenobi commented Jul 31, 2023

rustbot commented Jul 31, 2023

ehuss commented Jul 31, 2023

epage commented Jul 31, 2023

epage Jul 31, 2023

Choose a reason for hiding this comment

obi1kenobi Jul 31, 2023

Choose a reason for hiding this comment

obi1kenobi commented Jul 31, 2023

obi1kenobi commented Jul 31, 2023

weihanglo commented Jul 31, 2023

epage commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

Clarify in `--help` that `cargo test --all-targets` excludes doctests #12422

Clarify in `--help` that `cargo test --all-targets` excludes doctests #12422