Skip to content

fix(test): Expose '--no-capture' in favor of --nocapture #139224

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

Merged
merged 2 commits into from
Apr 28, 2025

Conversation

epage
Copy link
Contributor

@epage epage commented Apr 1, 2025

This improves consistency with commonly expected CLI conventions,
avoiding a common stutter people make when running tests (trying what
they expect and then having to check the docs to then user whats
accepted).

An alternative could have been to take a value, like --capture <value> (e.g. pytest does this).
Overall, we're shifting focus for features to custom test harnesses (see #134283).
Most of pytests modes will likely be irrelevant in that situation.
As for the rest, its too early to tell which, if any, may be relevant,
so we're sticking with this small, quality of life improvement.

I expect we'll warn about --nocapture being deprecated in the future after a sufficient transition period has been allowed.
By deprecating --nocapture, we intend that custom test harnesses do
not need to support it for reasons outside of their own compatibility
requirements, much like the deprecation in #134283

I'm punting for now on the naming of RUST_TEST_NOCAPTURE.
I feel like T-testing-devex should do a wider look at environment
variables role in libtest before evaluating whether to

  • Deprecate it in favor of the user passing CLI flags or the test runner
    providing its own config
  • Deprecate in favor of RUST_TEST_NO_CAPTURE
  • Deprecate in favor of RUST_TEST_CAPTURE

Other CLI flags were evaluated for casing consistency:

Regarding the implementation, I moved --nocapture out of optgroups(), into parse_opts(), out of an abundance of caution in passing the options without a deprecated value to the usage generation. However, the usage does not actually show optional flags, so this could potentially be dropped, simplifying the PR.

Note: compiletest added --no-capture instead of --nocapture in #134809

T-testing-devex FCP: #133073 (comment)

Fixes #133073

@rustbot
Copy link
Collaborator

rustbot commented Apr 1, 2025

r? @joboet

rustbot has assigned @joboet.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 1, 2025
Comment on lines +202 to +204
let mut opts = optgroups();
// Flags hidden from `usage`
opts.optflag("", "nocapture", "Deprecated, use `--no-capture`");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From PR description:

Regarding the implementation, I moved --nocapture out of optgroups(), into parse_opts(), out of an abundance of caution in passing the options without a deprecated value to the usage generation. However, the usage does not actually show optional flags, so this could potentially be dropped, simplifying the PR.

@epage epage added A-libtest Area: `#[test]` / the `test` library T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 1, 2025
@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented Apr 1, 2025

You'll probably also have to adjust compiletest to use the new field name.

@joboet
Copy link
Member

joboet commented Apr 3, 2025

I think Thom is more familiar with test.
r? @thomcc

@rustbot rustbot assigned thomcc and unassigned joboet Apr 3, 2025
@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 10, 2025

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@epage epage changed the title fix(test): Expose '--no-capture', deprecating '--nocapture' fix(test): Expose '--no-capture' in favor of --nocapture Apr 10, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu self-assigned this Apr 10, 2025
@jieyouxu jieyouxu removed their assignment Apr 11, 2025
@rust-log-analyzer

This comment has been minimized.

@epage epage added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 14, 2025
@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 15, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 15, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

epage added 2 commits April 17, 2025 13:30
This will help with hiding some options in `--help`
This improves consistency with commonly expected CLI conventions,
avoiding a common stutter people make when running tests (trying what
they expect and then having to check the docs to then user whats
accepted).

An alternative could have been to take a value, like `--capture <value>` (e.g. `pytest` does this).
Overall, we're shifting focus for features to custom test harnesses (see rust-lang#134283).
Most of `pytest`s modes will likely be irrelevant in that situation.
As for the rest, its too early to tell which, if any, may be relevant,
so we're sticking with this small, quality of life improvement.

By deprecating `--nocapture`, we intend that custom test harnesses do
not need to support it for reasons outside of their own compatibility
requirements, much like the deprecation in rust-lang#134283

I'm punting for now on the naming of `RUST_TEST_NOCAPTURE`.
I feel like T-testing-devex should do a wider look at environment
variables role in lib`test` before evaluating whether to
- Deprecate it in favor of the user passing CLI flags or the test runner
  providing its own config
- Deprecate in favor of `RUST_TEST_NO_CAPTURE`
- Deprecate in favor of `RUST_TEST_CAPTURE`

Other CLI flags were evaluated for casing consistency:
- `--logfile` has the same problem but was deprecated in rust-lang#134283

Fixes rust-lang#133073
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 25, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 25, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@thomcc
Copy link
Member

thomcc commented Apr 27, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 27, 2025

📌 Commit aa8670f has been approved by thomcc

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 Apr 27, 2025
ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Apr 28, 2025
fix(test): Expose '--no-capture' in favor of `--nocapture`

This improves consistency with commonly expected CLI conventions,
avoiding a common stutter people make when running tests (trying what
they expect and then having to check the docs to then user whats
accepted).

An alternative could have been to take a value, like `--capture <value>` (e.g. `pytest` does this).
Overall, we're shifting focus for features to custom test harnesses (see rust-lang#134283).
Most of `pytest`s modes will likely be irrelevant in that situation.
As for the rest, its too early to tell which, if any, may be relevant,
so we're sticking with this small, quality of life improvement.

I expect we'll warn about `--nocapture` being deprecated in the future after a sufficient transition period has been allowed.
By deprecating `--nocapture`, we intend that custom test harnesses do
not need to support it for reasons outside of their own compatibility
requirements, much like the deprecation in rust-lang#134283

I'm punting for now on the naming of `RUST_TEST_NOCAPTURE`.
I feel like T-testing-devex should do a wider look at environment
variables role in lib`test` before evaluating whether to
- Deprecate it in favor of the user passing CLI flags or the test runner
  providing its own config
- Deprecate in favor of `RUST_TEST_NO_CAPTURE`
- Deprecate in favor of `RUST_TEST_CAPTURE`

Other CLI flags were evaluated for casing consistency:
- `--logfile` has the same problem but was deprecated in rust-lang#134283

Regarding the implementation, I moved `--nocapture` out of `optgroups()`, into `parse_opts()`, out of an abundance of caution in passing the options without a deprecated value to the usage generation.  However, the usage does not actually show optional flags, so this could potentially be dropped, simplifying the PR.

Note: `compiletest` added `--no-capture` instead of `--nocapture` in rust-lang#134809

T-testing-devex FCP: rust-lang#133073 (comment)

Fixes rust-lang#133073
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2025
…enton

Rollup of 10 pull requests

Successful merges:

 - rust-lang#136160 (Remove backticks from `ShouldPanic::YesWithMessage`'s `TrFailedMsg`)
 - rust-lang#138395 (Download GCC from CI on test builders)
 - rust-lang#138737 (uefi: Update r-efi)
 - rust-lang#138939 (Add `Arc::is_unique`)
 - rust-lang#139224 (fix(test): Expose '--no-capture' in favor of `--nocapture`)
 - rust-lang#139546 (std(docs): clarify how std::fs::set_permisions works with symlinks)
 - rust-lang#139883 (crashes: more tests)
 - rust-lang#140345 (Avoid re-interning in `LateContext::get_def_path`)
 - rust-lang#140351 (docs: fix incorrect stability markers on `std::{todo, matches}`)
 - rust-lang#140359 (specify explicit safety guidance for from_utf8_unchecked)

r? `@ghost`
`@rustbot` modify labels: rollup
ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Apr 28, 2025
fix(test): Expose '--no-capture' in favor of `--nocapture`

This improves consistency with commonly expected CLI conventions,
avoiding a common stutter people make when running tests (trying what
they expect and then having to check the docs to then user whats
accepted).

An alternative could have been to take a value, like `--capture <value>` (e.g. `pytest` does this).
Overall, we're shifting focus for features to custom test harnesses (see rust-lang#134283).
Most of `pytest`s modes will likely be irrelevant in that situation.
As for the rest, its too early to tell which, if any, may be relevant,
so we're sticking with this small, quality of life improvement.

I expect we'll warn about `--nocapture` being deprecated in the future after a sufficient transition period has been allowed.
By deprecating `--nocapture`, we intend that custom test harnesses do
not need to support it for reasons outside of their own compatibility
requirements, much like the deprecation in rust-lang#134283

I'm punting for now on the naming of `RUST_TEST_NOCAPTURE`.
I feel like T-testing-devex should do a wider look at environment
variables role in lib`test` before evaluating whether to
- Deprecate it in favor of the user passing CLI flags or the test runner
  providing its own config
- Deprecate in favor of `RUST_TEST_NO_CAPTURE`
- Deprecate in favor of `RUST_TEST_CAPTURE`

Other CLI flags were evaluated for casing consistency:
- `--logfile` has the same problem but was deprecated in rust-lang#134283

Regarding the implementation, I moved `--nocapture` out of `optgroups()`, into `parse_opts()`, out of an abundance of caution in passing the options without a deprecated value to the usage generation.  However, the usage does not actually show optional flags, so this could potentially be dropped, simplifying the PR.

Note: `compiletest` added `--no-capture` instead of `--nocapture` in rust-lang#134809

T-testing-devex FCP: rust-lang#133073 (comment)

Fixes rust-lang#133073
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2025
…enton

Rollup of 9 pull requests

Successful merges:

 - rust-lang#138395 (Download GCC from CI on test builders)
 - rust-lang#138737 (uefi: Update r-efi)
 - rust-lang#138939 (Add `Arc::is_unique`)
 - rust-lang#139224 (fix(test): Expose '--no-capture' in favor of `--nocapture`)
 - rust-lang#139546 (std(docs): clarify how std::fs::set_permisions works with symlinks)
 - rust-lang#139883 (crashes: more tests)
 - rust-lang#140345 (Avoid re-interning in `LateContext::get_def_path`)
 - rust-lang#140351 (docs: fix incorrect stability markers on `std::{todo, matches}`)
 - rust-lang#140359 (specify explicit safety guidance for from_utf8_unchecked)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2025
…enton

Rollup of 8 pull requests

Successful merges:

 - rust-lang#138395 (Download GCC from CI on test builders)
 - rust-lang#138737 (uefi: Update r-efi)
 - rust-lang#138939 (Add `Arc::is_unique`)
 - rust-lang#139224 (fix(test): Expose '--no-capture' in favor of `--nocapture`)
 - rust-lang#139546 (std(docs): clarify how std::fs::set_permisions works with symlinks)
 - rust-lang#140345 (Avoid re-interning in `LateContext::get_def_path`)
 - rust-lang#140351 (docs: fix incorrect stability markers on `std::{todo, matches}`)
 - rust-lang#140359 (specify explicit safety guidance for from_utf8_unchecked)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8ee9029 into rust-lang:master Apr 28, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 28, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2025
Rollup merge of rust-lang#139224 - epage:nocapture, r=thomcc

fix(test): Expose '--no-capture' in favor of `--nocapture`

This improves consistency with commonly expected CLI conventions,
avoiding a common stutter people make when running tests (trying what
they expect and then having to check the docs to then user whats
accepted).

An alternative could have been to take a value, like `--capture <value>` (e.g. `pytest` does this).
Overall, we're shifting focus for features to custom test harnesses (see rust-lang#134283).
Most of `pytest`s modes will likely be irrelevant in that situation.
As for the rest, its too early to tell which, if any, may be relevant,
so we're sticking with this small, quality of life improvement.

I expect we'll warn about `--nocapture` being deprecated in the future after a sufficient transition period has been allowed.
By deprecating `--nocapture`, we intend that custom test harnesses do
not need to support it for reasons outside of their own compatibility
requirements, much like the deprecation in rust-lang#134283

I'm punting for now on the naming of `RUST_TEST_NOCAPTURE`.
I feel like T-testing-devex should do a wider look at environment
variables role in lib`test` before evaluating whether to
- Deprecate it in favor of the user passing CLI flags or the test runner
  providing its own config
- Deprecate in favor of `RUST_TEST_NO_CAPTURE`
- Deprecate in favor of `RUST_TEST_CAPTURE`

Other CLI flags were evaluated for casing consistency:
- `--logfile` has the same problem but was deprecated in rust-lang#134283

Regarding the implementation, I moved `--nocapture` out of `optgroups()`, into `parse_opts()`, out of an abundance of caution in passing the options without a deprecated value to the usage generation.  However, the usage does not actually show optional flags, so this could potentially be dropped, simplifying the PR.

Note: `compiletest` added `--no-capture` instead of `--nocapture` in rust-lang#134809

T-testing-devex FCP: rust-lang#133073 (comment)

Fixes rust-lang#133073
@epage epage deleted the nocapture branch April 28, 2025 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-libtest Area: `#[test]` / the `test` library A-testsuite Area: The testsuite used to check the correctness of rustc disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--nocapture doesn't follow common CLI conventions, making it a stumbling block to people debugging failures
10 participants