libtest support for env variables

[epage]

With rust-lang/rust#133073, we decided to rename --nocapture but overlooked RUST_TEST_NOCAPTURE. I suggested we defer how to handle RUST_TEST_NOCAPTURE to not block --no-capture on bigger decisions.

Options

• Add RUST_TEST_NO_CAPTURE
• Switch to positive logic with RUST_TEST_CAPTURE
  • Maybe re-evaluate the true/false condition
• Deprecate it

[epage]

From rust-lang/rust#133073 (comment)

Thinking more on this, I'm leaning towards "defer".

--no-capture is a nice quality of life improvement that I'd rather not block on "bigger questions". For me, the big question I'm starting to wonder about is if we should deprecate the env variables. My suspicion is that they are a low-effort config system that is meant to make up for a lacking in the common test runner (cargo test). In that case, we should instead focus on the common test runner. These could also cause problems as Cargo tries to coordinate the behavior of test binaries. If cargo test is trying to read json output and the user bypasses cargo to tell the test to report other content, that could become problematic.

[epage]

Looking into the history of env variable support

1. RUST_THREADS was respected by the Rust runtime and libextra/test got this for free
2. Decouple number of concurrent tests from RUST_THREADS env var rust#7335 suggested decoupling libextra/test from the runtime by adding RUST_TASK_THREADS (implementation: Make the unit-test framework check RUST_TEST_TASKS over RUST_THREADS. rust#8823)
3. Optionally allow writing to stdout in test runs rust#13374 suggested the ability to not capture output. test: Add an option to not capture output rust#13706 added support for --nocapture / RUST_TEST_NOCAPTURE.
4. A --test-threads was finally requested in Add a --test-threads option for rustc test binaries rust#25636 (implementation: Add --test-threads option to test binaries rust#35414)

So this feels like its subject to path dependence. libtest inherited env variable support and then tweaked it. Without further information, I'm guessing RUST_TEST_NOCAPTURE just then copied that pattern.

[epage]

As this point, libtest supports

• RUST_TEST_SHUFFLE (unstable)
• RUST_TEST_SHUFFLE_SEED (unstable)
• RUST_TEST_NOCAPTURE
• RUST_TEST_THREADS

[epage]

@rfcbot fcp merge

I propose that we deprecate the use of environment variables in libtest. Due to compatibility reasons, we do not plan to eventually remove any of the stable environment variables. The timing of when to start reporting deprecation message on use will be determined by the perceived level of use of the variable and whether replacement functionality is available (e.g. if we feel that RUST_TEST_THREADS is of heavy use for config, we may wait until it can be handled in cargo's config system).

This would include:

• RUST_TEST_THREADS
• RUST_TEST_NOCAPTURE
• RUST_TEST_SHUFFLE (unstable)
• RUST_TEST_SHUFFLE_SEED (unstable)

Benefits of doing this:

• We reduce the API surface expected of custom test harnesses which can also reduce their complexity and build times
• Avoid issues from users bypassing test runners (like cargo test). Test runners could track a list of all libtest environment variables but that could be a losing proposition to maintain, if for no other reason than this coupling between the two parts being invisible
• Allow test runners (like cargo test) to provide a better experience for configuring this. For example in cargo tests case, if this were represented in config, then we'd get persistent-file, environment, and CLI control "for free" and the configuration and documentation would be in place users are more likely to look.

History

At each step, I could not find evidence of design discussions on whether to support CLI, env, or both. The first env variable seems to come from the fact that it was being forked out of an existing env variable that had a much wider scope.

At best, this seems like a way to offer a more persistent configuration for these flags but environment variables hidden away in libtest is a bit clunky and this seems like the wrong layer to handle this problem.

Originally: RUST_THREADS was respected by the Rust runtime and libextra/test got this for free

2013: rust-lang/rust#7335 suggested splitting RUST_TEST_TASKS out of RUST_THREADS. In that issue and the implementation (rust-lang/rust#8823).

2014: rust-lang/rust#13374 ask for support to disable capturing of stdout/stderr. --nocapture and RUST_TEST_NOCAPTURE were added together.

2015: rust-lang/rust#23525 renamed RUST_TEST_TASKS to RUST_TEST_THREADS

2016: rust-lang/rust#25636 asked to configure RUST_TEST_THREADS via --test-threads which was implemented in rust-lang/rust#35414

2021: rust-lang/rust#85440 asked for test randomization which was implemented in rust-lang/rust#89082, adding --shuffle / RUST_TEST_SHUFFLEand--shuffle-seed SEED/RUST_TEST_SHUFFLE_SEED`

Potentially relevant issues

• libtest should set RUST_TEST_THREADS environment variable rust#74845

[rfcbot]

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Muscraft
• @calebcartwright
• @epage
• @thomcc
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

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