Make --nocapture optional

[kpreid]

When running tests, rust-analyzer unconditionally adds --nocapture to the test arguments. I would like this to be optional, because using --nocapture leads to sometimes-unreadable interleaved output, especially when two tests fail simultaneously, or when successful tests are producing verbose output intended to be read only if the test fails.

[dnut]

I would implement this by making cargo args configurable in general, allowing the user to enter an arbitrary string. There's nothing particularly special about --nocapture that entitles it to special treatment compared to other cargo args. I would also make it empty by default, as in --nocapture should not be enabled unless the user explicitly turns it on by typing it, due to the reasoning in #13040.

[kpreid]

A possible complication in "just" making test arguments customizable: there are both arguments to cargo and arguments to the test binary, and users might want to specify one or both. I think accepting a custom argument list that may include "--" should solve that problem adequately and intuitively.

[SludgePhD]

This feature would be great to have. The current behavior is opaque and the presence of --nocapture is never communicated to the user. Neither is the unconditionally set RUST_BACKTRACE=short.

This makes test suites that make use of #[should_panic] difficult to use with rust-analyzer because all the panic messages and backtraces make the output completely unreadable:

    Blocking waiting for file lock on build directory
   Compiling zaru-linalg v0.1.0 (/home/sludge/code/Zaru/crates/zaru-linalg)
    Finished test [unoptimized] target(s) in 0.43s
     Running unittests src/lib.rs (target/debug/deps/zaru_linalg-57770df66f31a28a)

running 3 tests
thread 'matrix::view::tests::view_oob' panicked at crates/zaru-linalg/src/matrix/view.rs:96:9:
assertion failed: c_off + VC <= self.num_columns()
stack backtrace:
test matrix::view::tests::compare ... ok
test matrix::view::tests::view ... ok
   0: rust_begin_unwind
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/std/src/panicking.rs:597:5
   1: core::panicking::panic_fmt
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:72:14
   2: core::panicking::panic
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/panicking.rs:127:5
   3: zaru_linalg::matrix::view::MatrixView<T,R,C>::fixed_view
   4: zaru_linalg::matrix::view::tests::view_oob
   5: zaru_linalg::matrix::view::tests::view_oob::{{closure}}
   6: core::ops::function::FnOnce::call_once
   7: core::ops::function::FnOnce::call_once
             at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
test matrix::view::tests::view_oob - should panic ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 42 filtered out; finished in 0.01s

(this output is with only a single #[should_panic] test)

[Veykril]

Right, RUST_BACKTRACE=short is set for r-a to show better panic info by default, leaking that into cargo invocations is something we should prevent though.

[MingweiSamuel]

--show-output does not show the output if a test is hanging. To re-enable --nocapture use

"rust-analyzer.runnables.extraTestBinaryArgs": [
        "--nocapture"
],

[matklad]

I personally feel strongly that --nocapture is the right default. The current default of --show-output makes it impossible to debug the tests, as you don't see the output until the end, which might not come.

The more "stuff" happens between the user and the test binary, the harder it is to realize what's happening.

The problem with interleaved test output should be fixed by making sure that passing tests do not print anything.

Interleaving output is a feature when you debug a tricky concurrency bug caused by interraction between the tests.

It is of course file to have an option to use --show-output as an easy-accessible checkbox, but I am 0.8 sure that --nocapture would be the right default leading to better ecosystem, precisely because it forces you to be intentional about your test's output.

To reiterate, this is my personal opinion, and, while I do hold is strongly, it could be wrong.

[dnut]

Regarding the treatment of stdout, I can see reasonable arguments on both sides, but I don't think these arguments are relevant to rust-analyzer. These arguments are more relevant to rustc. If the default behavior should be to not capture stdout, then why shouldn't we change rustc to have that behavior? I believe that rust-analyzer's defaults should be consistent with rustc's defaults, whatever they are. Discrepancies in behavior are a source of confusion and frustration.

[kpreid]

I personally feel strongly that --nocapture is the right default. … The problem with interleaved test output should be fixed by making sure that passing tests do not print anything.

For what it’s worth, I'll give some additional background, as the author of this issue and PR, since our experiences of testing seem to be very different:

I find it very frequently useful to write tests that “log” their actions with simple println!s, so that if the test fails, I can see what was computed before the failure, that led to the erroneous situation. It would be possible to implement this within individual tests, using an explicit log and a Drop implementation that prints it on panic; but setting one up is work the rustc test harness saves me from. And to me, if I have to modify an existing test, e.g. adding more printing, to understand why it failed, that's a deficiency in the test suite; it means I have to spend time editing tests and rerunning before I can get to fixing the bug I just introduced.

Instead of “passing tests do not print anything”, the property I like is “I don't see anything from passing tests”, which is what the test harness’s default mode (not --nocapture and not --show-output) does. The only reason my PR #17105 switched to --show-output rather than no flags was for a smaller change to the status quo (and I find the verbosity of --show-output less bothersome when it's isolated in its own tab of test output instead of in my main terminal).

Interleaving output is a feature when you debug a tricky concurrency bug caused by interraction between the tests.

I agree with the logic, but the case is rare for me: I have never had tests interfere with each other (in the kind of code I write, which is on the “avoid global state at nearly all costs” side of things), and hanging tests are very rare and usually need more intervention than just being able to look at the output in --nocapture style (e.g. modifying the code to print more of its progress, and running with other additional options). I’m also surprised by these two claims occurring simultaneously: if a passing test is designed to print nothing, then surely it is likely that a hanging test will also print nothing, so default --nocapture will not help?

Also, when RA used --nocapture, I regularly saw users having trouble with understanding interleaved output from multiple test failures — even the panic hook output by itself could be interleaved in illegible fashion (though that was recently improved by rust-lang/rust#127397).

---

One exception: I do think it would make sense to pass --nocapture along with --exact; when running exactly one test, there is no disadvantage to uncaptured output.