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

feat: remove --keep-going from cargo test/bench #12478

Merged
merged 2 commits into from
Aug 13, 2023

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Aug 11, 2023

What does this PR try to resolve?

It confuses people that both --no-fail-fast and --keep-going exist on cargo test and cargo bench but with slightly different behavior. The intended use cases for --keep-going involve build commands like build/check/clippy but never test/bench.

Hence, this commit removes --keep-going from test/bench and provides guidance of --no-fail-fast instead.

If people really want to build as many tests as possible, they can also do it in two steps:

cargo build --tests --keep-going
cargo test --test --no-fail-fast

How should we test and review this PR?

Only one commit containing two new tests verifying the error message.

To test the completion, simply run:

zsh

fpath+=$PWD/src/etc
autoload -Uz compinit
compinit

cargo t <tab>

bash

. ./src/etc/cargo.bashcomp.sh

cargo t <tab>

Additional information

cc #10496 (comment)

How long should we wait after this gets merged for stabilizing --keep-going?

It confuses people that both `--no-fail-fast` and `--keep-going` exist
on `cargo test` and `cargo bench` but with slightly different behavior.
The intended use cases for `--keep-going` involve build commands like
`build`/`check`/`clippy` but never `test`/`bench`.

Hence, this commit removes `--keep-going` from `test`/`bench` and
provides guidance of `--no-fail-fast` instead.

If people really want to build as many tests as possible, they can also
do it in two steps:

    cargo build --tests --keep-going
    cargo test --test --no-fail-fast
@rustbot
Copy link
Collaborator

rustbot commented Aug 11, 2023

r? @ehuss

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

@rustbot rustbot added 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-test S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 11, 2023
@weihanglo
Copy link
Member Author

Wish we could have something from #11702 or clap-rs/clap#4706 for suggestions.

To test, simply run:

zsh

```zsh
fpath+=$PWD/src/etc
autoload -Uz compinit
compinit

cargo t <tab>
```

bash:

```bash

. ./src/etc/cargo.bashcomp.sh

cargo t <tab>
```
@rustbot rustbot added the A-completions Area: shell completions label Aug 11, 2023
@epage
Copy link
Contributor

epage commented Aug 13, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 13, 2023

📌 Commit de8b913 has been approved by epage

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 Aug 13, 2023
@bors
Copy link
Collaborator

bors commented Aug 13, 2023

⌛ Testing commit de8b913 with merge 7e9de3f...

@bors
Copy link
Collaborator

bors commented Aug 13, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 7e9de3f to master...

@bors bors merged commit 7e9de3f into rust-lang:master Aug 13, 2023
19 checks passed
@weihanglo weihanglo deleted the keep-going branch August 13, 2023 05:11
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2023
Update cargo

21 commits in d78bbf4bde3c6b95caca7512f537c6f9721426ff..7e9de3f4ec3708f500bec142317895b96131e47c
2023-08-03 12:58:25 +0000 to 2023-08-13 00:47:32 +0000
- feat: remove `--keep-going` from `cargo test/bench` (rust-lang/cargo#12478)
- chore: window-sys should be a platform-specific dependency (rust-lang/cargo#12483)
- docs: make the env var source of rerun-if-env-changed clearer (rust-lang/cargo#12482)
- doc: note the backward compatible `.cargo/credential` file exists (rust-lang/cargo#12479)
- Fix elided lifetime in associated const (rust-lang/cargo#12475)
- prompt the use of `--nocapture` flag if `cargo test` process is terminated via a signal. (rust-lang/cargo#12463)
- cargo-credential: reset stdin & stdout to the Console (rust-lang/cargo#12469)
- Fix cargo remove incorrectly removing used patches (rust-lang/cargo#12454)
- chore(gh): Expand update window (rust-lang/cargo#12466)
- Fix panic when enabling http.debug for certain strings (rust-lang/cargo#12468)
- fix(cli): Make `--help` easier to browse (rust-lang/cargo#11905)
- fix: preserve jobserver file descriptors on rustc invocation to get `TargetInfo` (rust-lang/cargo#12447)
- refactor: migrate to `tracing` (rust-lang/cargo#12458)
- docs: add example for cargo-credential (rust-lang/cargo#12461)
- Bail out an error when using cargo:: in custom build script (rust-lang/cargo#12332)
- Fix printing multiple warning messages for unused fields in [registries] table (rust-lang/cargo#12439)
- Update windows dependencies (rust-lang/cargo#12453)
- Rustfmt a let-else statement (rust-lang/cargo#12451)
- Add allow(internal_features) (rust-lang/cargo#12450)
- Update pretty_env_logger to 0.5 (rust-lang/cargo#12445)
- Remove build metadata from libgit2-sys dependency (rust-lang/cargo#12444)

r? `@ghost`
@ehuss ehuss added this to the 1.73.0 milestone Aug 22, 2023
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-completions Area: shell completions A-documenting-cargo-itself Area: Cargo's documentation Command-bench 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