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

[epage]

By convention, users would expect to type in --no-capture. The fact that the argument is --nocapture trips people up, especially as they have to wait for their test to compile before they see the failure. Without spelling suggestions, they need to then consult the help to then remember its without the middle -. Unless someone is doing this all the time to build up muscle memory to counteract intuition, this will trip people up each time.

See also

• Rename test option --nocapture to --no-capture #24451
• https://hachyderm.io/@wezm@mastodon.decentralised.social/113485301109871075
• https://rust-lang.zulipchat.com/#narrow/channel/404371-t-testing-devex/topic/--nocapture/near/482538793

[epage]

I propose we audit libtest's CLI for

• consistency with conventions
• self-consistecy
• consistency with the rest of Rust programs
• consistency with other test runners

And propose we adjust CLI parameters to meet user expectations. By "adust" I mean that we make new parameters that are aliases for the existing parameters, and deprecate the existing parameters by hiding them in help and providing a deprecation message.

As T-testing-devex is considering first-class custom test harnesses which will be expected to follow libtest's "API", the sooner we nail this down, the more likely they won't need to deal with supporting the deprecated API.

#24451 (comment)

I would personally feel it's a little too late to make a change like this,

If we had made this change in 2015, the community migration cost would have been small. Its larger now but the longer we wait, the larger it becomes. I'm more worried about existing users having problems with this and the large number of potential new users.

[epage]

Usage: /home/epage/src/personal/cargo/target/debug/deps/cargo-ef836591b671bcbc [OPTIONS] [FILTERS...]

Options:
        --include-ignored
                        Run ignored and not ignored tests
        --ignored       Run only ignored tests
        --force-run-in-process
                        Forces tests to run in-process when panic=abort
        --exclude-should-panic
                        Excludes tests marked as should_panic
        --test          Run tests and not benchmarks
        --bench         Run benchmarks instead of tests
        --list          List all tests and benchmarks
    -h, --help          Display this message
        --logfile PATH  Write logs to the specified file
        --nocapture     don't capture stdout/stderr of each task, allow
                        printing directly
        --test-threads n_threads
                        Number of threads used for running tests in parallel
        --skip FILTER   Skip tests whose names contain FILTER (this flag can
                        be used multiple times)
    -q, --quiet         Display one character per test instead of one line.
                        Alias to --format=terse
        --exact         Exactly match filters rather than by substring
        --color auto|always|never
                        Configure coloring of output:
                        auto = colorize if stdout is a tty and tests are run
                        on serially (default);
                        always = always colorize output;
                        never = never colorize output;
        --format pretty|terse|json|junit
                        Configure formatting of output:
                        pretty = Print verbose output;
                        terse = Display one character per test;
                        json = Output a json document;
                        junit = Output a JUnit document
        --show-output   Show captured stdout of successful tests
    -Z unstable-options Enable nightly-only flags:
                        unstable-options = Allow use of experimental features
        --report-time   Show execution time of each test.
                        Threshold values for colorized output can be
                        configured via
                        `RUST_TEST_TIME_UNIT`, `RUST_TEST_TIME_INTEGRATION`
                        and
                        `RUST_TEST_TIME_DOCTEST` environment variables.
                        Expected format of environment variable is
                        `VARIABLE=WARN_TIME,CRITICAL_TIME`.
                        Durations must be specified in milliseconds, e.g.
                        `500,2000` means that the warn time
                        is 0.5 seconds, and the critical time is 2 seconds.
                        Not available for --format=terse
        --ensure-time   Treat excess of the test execution time limit as
                        error.
                        Threshold values for this option can be configured via
                        `RUST_TEST_TIME_UNIT`, `RUST_TEST_TIME_INTEGRATION`
                        and
                        `RUST_TEST_TIME_DOCTEST` environment variables.
                        Expected format of environment variable is
                        `VARIABLE=WARN_TIME,CRITICAL_TIME`.
                        `CRITICAL_TIME` here means the limit that should not
                        be exceeded by test.
        --shuffle       Run tests in random order
        --shuffle-seed SEED
                        Run tests in random order; seed the random number
                        generator with SEED

For casing, the two that are questionable are

• --logfile which T-testing-devex has an FCP for deprecating, see Export machine-readable test results to a file testing-devex-team#9
• --nocapture (I just put the middle - in...)

For consistency with the rest of Rust tools

• --test-threads: Cargo commands refer to this as -j --jobs

For consistency with pytest

• --no-capture is instead --capture=no with a short flag of -s
  • Other values fd|sys|no|tee-sys

For inspiration from cargo-nextest

• --ignored, --include-ignored: --run-ignored <default|only|all>
• --test-threads: -j --thread-threads

If people have other test runners they want to compare, you are welcome to add them! I'm scoping this to the existing, stable features of libtest.

[epage]

My hope is that we can close out the discussion quickly enough that we don't need to split it up. We'll likely need to do an FCP anyways for --nocapture, so might as well look at it holistically.

[epage]

Hmm, I see why you brought up a tracking issue. My focus on this is purely in naming and not in improving the semantics. My expectation is that we'll be moving filtering into Cargo and re-evaluate the interface then. Moving more logic into Cargo will be unblocked by the json format being stabilized (so Cargo communicates through tests through json) and this will reduce the burden of what custom test harnesses need to implement.

[sminez]

Is this something that would be possible to pick up once the details are finalised? I'd be interested in taking a look if it was.

[epage]

From Zulip

Once we have a decision, sure

[epage]

We talked about this in today's T-testing-devex meeting

• --nocapture: we were on board with changing this to --no-capture through deprecation/addition
• --test-threads: we are deferring this to see how things evolve for thread control within the compiler
• We didn't see anything else worth changing at this time with the assumption that most of the UX responsibility will be shifting to tools calling libtest, like cargo test.

Let's see if this works

@rfcbot fcp merge

[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. 🔔

[rfcbot]

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.

[djc]

@epage thanks for pushing to clean this up!!

[epage]

Something we had overlooked was that RUST_TEST_NOCAPTURE exists which checks for a non-zero value to disable capturing (including empty).

Options

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

Thoughts?

[djc]

Adding RUST_TEST_NO_CAPTURE would make sense to me. Switching to positive logic (while I usually prefer it) feels like a mismatch with the CLI, so maybe it doesn't make sense here.

[weihanglo]

It seems reasonable to do the same deprecation to env vars.

[epage]

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.