Go: proposal: cmd/go: be consistent about not giving tests direct access to the terminal

[mvdan]

Summary

go test run on a terminal currently runs tests with os.Stdout pointing directly at said terminal. go test . never gives access to a terminal, as any package arguments cause output buffering.

At best, this leads to inconsistent and confusing behavior for test writers and users. At worst, this leads to tests behaving differently depending on whether or not their output is a terminal.

A test's output is simply text, and a terminal isn't guaranteed to be present. I propose that go test never give a test direct access to a terminal.

Description

The decision to allow go test to pass through a real terminal to the tests was made explicit in #18153. The reasoning behind that issue and decision is that it's nice when tests run by a human on a terminal can be shown with colored output, without affecting when tests are run by non-humans.

However, I think that was the wrong decision to make. First, it adds inconsistencies; many common commands like go test . or go test ./... won't give access to a real terminal because of how go test treats and buffers output from each test. Here is a quick demonstration:

$ cat f_test.go
package foo

import (
        "os"
        "testing"

        "golang.org/x/crypto/ssh/terminal"
)

func TestFoo(t *testing.T) {
        if !terminal.IsTerminal(int(os.Stdout.Fd())) {
                t.Fatal("not a terminal!")
        }
}
$ go test
PASS
ok      test.tld/foo    0.002s
$ go test | cat
--- FAIL: TestFoo (0.00s)
    f_test.go:12: not a terminal!
FAIL
exit status 1
FAIL    test.tld/foo    0.002s
$ go test .
--- FAIL: TestFoo (0.00s)
    f_test.go:12: not a terminal!
FAIL
FAIL    test.tld/foo    0.002s
FAIL

I also believe this sets a bad precedent. A test shouldn't care at all if its standard output is a terminal or not. If a test developer really wants prettier or colorful output, it should be based on explicit settings like $TERM or flags like -pretty.

Needing to give tests direct access to the terminal also limits what go test can do. For example, in #29062 we tried to capture stdout in all cases to see if a test abruptly called os.Exit(0), which could result in no other tests being run without the user ever realising. This is impossible to do if we can't capture standard output. Even if this particular approach doesn't end up being approved, I still think go test should be able to inspect or manipulate test output in any way it needs.

/cc @bcmills @rsc @rhysd @bradfitz from previously linked issues

[seebs]

As a person who sometimes runs things on terminals that don't happen to support ANSI color, or non-terminals that do: I would very much like to see things not use "is a terminal" as a surrogate for "supports color output".

If "we want special treatment when people are running tests", maybe we should have a -test.human flag (or go test -human) which attempts to do things suitable for human-is-watching testing. But even that sounds undesireable -- again, that's not the same question as "should try to colorize output" or "if trying to colorize output, assume ANSI escape sequences can set color".

[rsc]

FWIW, the distinction in general is very clearly documented in go help test. Look for "two different modes". The fact that go test is uncached and go test . is cached is a much larger difference than the terminal setting, and it's an important one that can't be removed.

Right now in local directory mode the test runs with stdout/stderr both connected to the overall go command process's stdout, to make output appear immediately upon printing. We could I suppose connect it to a pipe that the go command would copy to the real stdout promptly. It's unclear how important that is and/or what it would break.

[rsc]

Looking at #18153, what is going to break is colored output in local directory mode.
As much as I'd love to see that go away, I kind of doubt it will.
Instead people will hard-code the use of ANSI codes even when piping into tools, which will be even more terrible.

[mvdan]

FWIW, the distinction in general is very clearly documented in go help test. Look for "two different modes".

I'm not tryng to get rid of the caching difference, though. I'm trying to remove the discrepancy of the access to the output terminal. Also note that said access to a terminal in "local directory mode" has never been documented.

It's unclear how important that is

It's a requirement for #29062, and I'm sure it will be an unfortunate limitation again in the future for go test. This is one of the two main arguments, the other one being the lack of consistency.

Instead people will hard-code the use of ANSI codes even when piping into tools, which will be even more terrible.

I kind of see your point there, but I'm not sure that this is a good argument to not make things consistent and better in go test. Developers will be able to write bad tests either way. If anything, I'd argue that implementing this proposal will be a chance for us to remind people to not use colored output in tests, at least not by default. We can clearly signal that in 1.14's release notes, for example.

[akyoto]

I believe that correctness is more important than colors in a terminal.

go test exiting with code 0 when tests are literally containing t.Fatal() sounds like a red flag to me.

[rsc]

29062 can be fixed by hooking os.Exit internally; no need for pipes.

I think we should document the current state (terminal in local directory mode, otherwise not),
not try to change it.

[akyoto]

In that case, it would leave only the consistency problem to discuss.

Personal thoughts on the consistency issue:

1. The situations in which the 2 test calls will have different results are highly unlikely.
2. Nonetheless, it is theoretically possible and therefore an inconsistency.
3. Having a real terminal is "nice-to-have" for some kind of tests* but mostly not needed.
4. Colors could potentially rely on something else to detect support other than "is a terminal".
5. Documenting the current state would at least make it defined behavior and clear the confusion.

Point 3 example (from fatih's color package):

[mvdan]

can be fixed by hooking os.Exit internally; no need for pipes.

I think we're focusing too much on that issue. It was one of the reasons behind the proposal, but not the only one. I still think that the current setup is inconsistent and undesirable, even if we agree that go test doesn't need to put a pipe in front of os.Stdout.

I think we should document the current state (terminal in local directory mode, otherwise not),
not try to change it.

Sorry, but I strongly disagree there. Once this behavior is documented, there's no going back. What is your argument for locking in the terminal usage?

You gave some reasons earlier, like people still being able to use colored output anyway. I replied to those in https://github.com/golang/go/issues/34877#issuecomment-542268817.

Ultimately I'm not one of the owners of cmd/go or testing, so I can't make a decision. Still, I'm left confused to see that the proposal is heading in the exact opposite direction than originally intended.

[rsc]

Sorry, but I strongly disagree there. Once this behavior is documented, there's no going back. What is your argument for locking in the terminal usage?

First, it is already documented that there is a clear distinction between go test and go test .. Any attempts to make those commands "the same" will fail. They are not and never will be.

Second, we have had issues filed before that explicitly established the rule that the console is available in local directory mode. The fixes for those issues did not write it down, but surely people are depending on it, or they wouldn't have filed the issues in the first place (specifically #18153).

I don't see any benefit here to compensate for breaking users. The original benefit stated was to implement the os.Exit(0) check, but there are other ways to do that, and it might not be a good idea anyway. What are the other benefits?

[rsc]

Ping, anyone who wants to suggest other benefits besides ability to implement the os.Exit(0) check. Otherwise, I think this should be closed.

[mvdan]

The benefit I was trying to show earlier is that go test and go test . would be more consistent and more reliable, without being exactly the same. I hardly believe that, were we to design go test today, we'd choose to make the console available some of the time.

There's the argument that it's not worth breaking those users to make go test a bit more consistent. I don't agree with the argument, because I think people who write tests depending on the console are just writing bad tests to begin with.

Having said that, I see that there's a tradeoff. I just don't agree that this proposal has no benefit beyond the os.Exit check.

[seebs]

I really feel like there's benefit to consistency as its own thing -- because if a test's behavior does in some way get affected by the console, it seems like this could cause very strange and hard to debug failures if people don't realize that's a thing. If I were just running go test in my development workflow, and someone started getting really weird failures with tests, it might be a long time before we thought of the differences with go test ., and a longer time before we realized that console-access was the secret ingredient.

I can see it being desireable to have console access available, or to not have it available, but it seems really weirdly brittle to have it be tied to the invocation of go test in this way.

To put it another way: Say I really want to test a thing which genuinely DOES need console access to function. Is it even possible to express that? Should it be?

[rsc]

I think it is perfectly fine for tests to have access to the console. Sometimes you do have manual tests that you need to interact with, perhaps selected by a command-line flag, and that is OK. You may think this is "bad tests" but not everyone agrees, and that's OK too.

go test and go test . are different in multiple ways, by design. This is one of those cases where if we were designing from scratch it would be great to debate the merits of various approaches. But that decision was made years ago, people rely on it, and there would need to be a _very_ large benefit to compensate for breaking all users who rely on the current behavior. I don't see that very large benefit here.

[rsc]

@seebs, if your test is failing mysteriously with/without console access then it sounds like you're not printing a good error message in your test. That would be better to focus on, no?

[rsc]

Based on the discussion and in particular https://github.com/golang/go/issues/34877#issuecomment-548032109, this seems like a likely decline.

Leaving open for a week for final comments.

[seebs]

You know, that's a good point: It might be surprising that the behavior changes, but a good test should probably be clear enough about what went wrong that you can figure it out.

[rsc]

No change in consensus. Declined.