compiletest failing tests aren't that noisy by default

[dev-ardi]

This suppresses the whole block if the env var is not set.

I don't think that it's very useful to have that information, especially when it's in the middle of other important information.

I'd even suggest pushing all of the duplicate information up or down in order to condense the diffs as much as possible.

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
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]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[jieyouxu]

So this one I'm not very sure about. Because having the exact command invocation has been useful for me multiple times especially because some test suites have preset flags that can affect the behavior of what I'm trying to test / what failed. I would imagine that this should be an option in config.toml that adjusts the preset for command output verbosity that bootstrap passes to compiletest.

[jieyouxu]

@dev-ardi could you elaborate on why you want to suppress the exact command status and command output?

[dev-ardi]

@dev-ardi could you elaborate on why you want to suppress the exact command status and command output?

It's very useful when you need it, but most of the time you don't need it and are just big blocks of noise in the output. I would at least have an option to hide it. I don't really care whether it's in config.toml or an env variable, and whether it should be on or off by default. As long as I can hide (and it's documented somewhere how to hide it for others) it I'm happy :P

[jieyouxu]

That sounds reasonable to me. I would prefer this, like the diff tool, to be an option that's configurable from bootstrap config.toml, something like:

# config.toml
[test]
# Verbose command output is enabled-by-default, but you can toggle it off
# if you don't need the verbose command invocation.
verbose-command-invocation = true 

Specifically I would prefer that the current verbose command invocation is shown-by-default, but offer an option to hide that if for some reason you don't need it.

[jieyouxu]

Also please do not hide the command status in any case, e.g. a test failing with exit status 1 can be significantly different from a test failing with exit status 101 (panics, ICEs, etc.).

@rustbot author

[dev-ardi]

There seems to already be an option like that, Config::verbose_tests.

Do you think I should hide it by default or not?

[jieyouxu]

No we should not hide it by default, because quite a few people actually want to see the exact command invocation.

Config::verbose_tests

We could try use that, though I'm not too sure what that particular bootstrap config does. I'll roll a T-bootstrap reviewer.

r? bootstrap

[dev-ardi]

Then if that shouldn't be the default we can't use that option. Thoughts?

[jieyouxu]

I would add a new config option like verbose-test-command-invocation maybe under [build], subject to what T-bootstrap prefers.

[ChrisDenton]

This likely needs some discussion with T-compiler/contributors. I personally find the information very useful in compiler tests and I don't think it should be hidden by default. But that's just my opinion. I use compiler tests infrequently so the less I need to configure the better. Someone who uses them alot may not mind having to tune different nobs to get the information they need.

[jieyouxu]

This likely needs some discussion with T-compiler/contributors. I personally find the information very useful in compiler tests and I don't think it should be hidden by default. But that's just my opinion. I use compiler tests infrequently so the less I need to configure the better. Someone who uses them alot may not mind having to tune different nobs to get the information they need.

https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Vibe.20check.20on.20suppressing.20failed.20test.20command.20invocation

(I forgor to link it)
TL;DR:

• Many contributors find the command invocation crucial to their workflow.
• We should not and cannot suppress the command invocation by default (there can be an opt-in config option to hide the command invocation).
• The status itself should not be hidden.

[bors]

☔ The latest upstream changes (presumably #133841) made this pull request unmergeable. Please resolve the merge conflicts.