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

cargo test --help: clarify --tests and --benches #14675

Merged
merged 2 commits into from
Oct 12, 2024
Merged

Conversation

RalfJung
Copy link
Member

This tries to reduce the confusion expressed in #10936.

I am not sure what it means to "test a target in benchmark mode", this is copied verbatim from cargo help test.

@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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 Oct 11, 2024
@@ -41,9 +41,9 @@ pub fn cli() -> Command {
"Test only the specified example",
"Test all examples",
"Test only the specified test target",
"Test all test targets",
"Test all targets that have the `test = true` manifest flag set",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make them shorter, like

Suggested change
"Test all targets that have the `test = true` manifest flag set",
"Test all targets that have `test = true` set",

and ditto --benches?

(I still find it confusing though, because it doesn't really test "all" targets. Anyway this PR is already an improvement)

"Test only the specified bench target",
"Test all bench targets",
"Test all targets in benchmark mode that have the `bench = true` manifest flag set",
Copy link
Member

@weihanglo weihanglo Oct 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what it means to "test a target in benchmark mode",

I don't really know, either. I think "in benchmark mode" should be removed from here and in man pages as well. Basically how I understand it is collecting Cargo targets that have bench = true and nothing else.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to confirm that before adjusting the docs.
Seems like this was added in f78ab2c, so Cc @ehuss

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "in benchmark mode" is left over from when cargo used to change modes/profiles based on which targets were being built. When we stabilized named profiles, we also changed cargo to stop doing that and use only a single profile.

I would delete it here and in the docs.

Copy link
Member Author

@RalfJung RalfJung Oct 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have done that. This was duplicated in a bunch of places so I hope this will work.

The term benchmark mode is now gone entirely; test mode still appears in a few places -- probably better for one of you to take a look since I am lacking the necessary context here.

@rustbot rustbot added A-cli-help Area: built-in command-line help A-documenting-cargo-itself Area: Cargo's documentation labels Oct 12, 2024
@RalfJung
Copy link
Member Author

I realized the cargo test --help texts are duplicated for cargo check --help and a bunch of others as well, I hope I now got them all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it was also talking about "benchmarking in test mode" vs "benchmarking in bench mode" -- I removed it for consistency, or is that still relevant somehow?

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 12, 2024

📌 Commit 2783a0f has been approved by weihanglo

It is now in the queue for this repository.

@bors 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 Oct 12, 2024
@bors
Copy link
Collaborator

bors commented Oct 12, 2024

⌛ Testing commit 2783a0f with merge de128be...

@bors
Copy link
Collaborator

bors commented Oct 12, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing de128be to master...

@bors bors merged commit de128be into rust-lang:master Oct 12, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-documenting-cargo-itself Area: Cargo's documentation Command-bench Command-build Command-check Command-fix Command-rustc Command-rustdoc Command-test S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants