Go: cmd/go: fail tests that invoke os.Exit(0) explicitly

[rhysd]

What version of Go are you using (go version)?

$ go version
go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env

GOARCH="amd64"

GOBIN=""

GOCACHE="/Users/rhysd/Library/Caches/go-build"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GOOS="darwin"

GOPATH="/Users/rhysd/.go"

GOPROXY=""

GORACE=""

GOROOT="/usr/local/Cellar/go/1.11.2/libexec"

GOTMPDIR=""

GOTOOLDIR="/usr/local/Cellar/go/1.11.2/libexec/pkg/tool/darwin_amd64"

GCCGO="gccgo"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD=""

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/9t/jwm1hlr905g_wlnzrmbnb3cr0000gn/T/go-build385579133=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Here is a code snip to explain this issue:

package foo

import (
    "os"
    "testing"
)

type Parsed struct{}

func parseArguments(args []string) (Parsed, error) {
    // parse arguments
    if len(args) == 0 {
        // Show help message
        os.Exit(0)
    }

    // check parsed arguments

    return Parsed{}, nil
}

func TestParse(t *testing.T) {
    _, err := parseArguments([]string{})
    if err != nil {
        t.Fatal(err)
    }
    // test parse result
}

func TestOther(t *testing.T) {
    t.Fatal()
}

Please write above code to some Go file and run:

$ go test
$ echo $?

It outputs 0. So it means that test is ok. However, actually test has stopped at the middle of execution since os.Exit(0) is accidentally called.
I'm not sure that this is a bug. It may be intended behavior. But when calling os.Exit(0) in tests accidentally (for example, due to lack of understanding of API), I may not notice the tests are wrongly run since it exits successfully. CI also cannot detect it.

What did you expect to see?

IMO, go test exiting with non-zero exit status when the tests exit at the middle of execution by os.Exit() would solve this issue.

What did you see instead?

echo $? echoes 0 and test exited successfully

[mvdan]

Here is a smaller repro:

$ cat go.mod
module foo.bar
$ cat f_test.go
package foo

import (
        "os"
        "testing"
)

func TestExit(t *testing.T) {
        os.Exit(0)
}

func TestFatal(t *testing.T) {
        t.Fatal()
}
$ go test
ok      foo.bar 0.001s
$ go test -v
=== RUN   TestExit
ok      foo.bar 0.001s

I agree that this can be confusing, since go test appears happy. It's only go test -v that looks off, since none of the tests actually succeed and finish.

Perhaps go test could error if a test binary exited too soon. For example, if we do go test -c and run the test binary, we get no output at all instead of a PASS or a FAIL. But I'm not sure if we could implement that without introducing even more confusing edge cases. For example, what should happen if a test binary panics?

/cc @bcmills @ianlancetaylor

[mvdan]

A perhaps simple fix would be for go test to complain if a test binary prints absolutely nothing. I think that should never happen.

[randall77]

@mvdan: That fix sounds like the right one.

[bcmills]

A test binary can provide an arbitrary func main, right? Why does it need to print anything on success?

[randall77]

Are you talking about TestMain? It prints a result (PASS) as well.
I suppose you could implement TestMain without calling m.Run on its argument, but that seems not supported.

[bcmills]

I did some experimenting and it looks like I was just mistaken. Carry on!

[gopherbot]

Change https://golang.org/cl/184457 mentions this issue: cmd/go: fail if a test binary succeeds with no output

[eliben]

From the CL:

A few TestMain funcs in the standard library needed fixing, as they
returned without printing anything as a means to skip testing the entire
package

How concerning is this for breaking user tests? If you had to fix up stdlib tests, it's likely that many tests in user code are going to break.

@randall77 https://github.com/golang/go/issues/31969 mentions a TestMain that doesn't invoke m.Run (though the docs of testing ask not to do that). Also, as @mvdan's CL demonstrates, this is done in the stdlib: https://github.com/golang/go/blob/master/src/cmd/internal/goobj/goobj_test.go#L33

[mvdan]

Historically, it has been acceptable to break user programs that were doing the opposite of what is documented. For example, see https://github.com/golang/go/issues/31859. We want to avoid this breakage whenever possible, but in some cases it's the best option.

I think this is one of those cases. The fix will uncover package tests that were succeeding by accident, which was the motivation of this bug. And after all, fixing the broken tests was a two-line change, and I think that's a reasonable thing to ask of the users who weren't following the docs.

[gopherbot]

Change https://golang.org/cl/200104 mentions this issue: doc/go1.14: note that tests without any output now fail

[gopherbot]

Change https://golang.org/cl/200106 mentions this issue: Revert "cmd/go: fail if a test binary exits with no output"

[bcmills]

I'm reverting this CL due to #34791. We can un-revert it if we decide that it is acceptable to break the assumptions of #18153, or if we figure out a way to implement this change without breaking those assumptions.

[mvdan]

Thanks for taking care of this @bcmills. I do fundamentally disagree with the premise of #18153. For example, it makes tests brittle, and requires them to be run with a real terminal, and no arguments:

$ 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

go test -v with no arguments fails, too.

Moreover, it makes it very hard (or impossible, I suspect) to inspect or modify the output in any way, like we needed to do here.

Do you reckon a proposal is necessary to make a proper decision here?

[mvdan]

I realise that the original idea was to color test output, and not necessarily make tests fail or pass depending on stdout being a terminal. Still, I think it's wrong to change the test's behavior depending on whether it happens to be run on a terminal. If a user really wants color on their test outputs, they could use a test flag, or an environment variable like $TERM != "dumb". At least in that case, you wouldn't get the inconsistencies I showed above.

[seebs]

You might be able to do this using a pty, but I am pretty uncomfortable with the assumption that all terminals handle color codes, etcetera.

An observation: This doesn't really help much if you have a test function which prints something and then exits. You can't defer a check, because os.Exit skips deferred functions. I don't think there's anything comparable to atexit(), and I am not at all sure I'd want one added.

Maybe test should build a program which intercepts os.Exit using linkname? There's no way that could go wrong.

[mvdan]

Certainly the fix I implemented (and got reverted) wouldn't catch all scenarios. The intent was basically to catch honest mistakes, not tests trying to do harmful things. If a test is trying really hard to do harmful stuff, it could do much worse :)

I personally think that simple rules around "the test binary's output stopped unexpectedly" are simpler and better in the long run than doing magic things with linkname. And we probably don't need linkname to catch the most common honest mistakes.

[rsc]

Exiting 0 is how you indicate success. If the test goes out of its way to do that, I am not convinced we should second-guess it.

[mvdan]

I would normally agree, but note that an os.Exit(0) silently skips the rest of the tests, doesn't let TestMain exit properly, and could hide real test failures. This issue is not about reasonable uses of os.Exit(0), it's about unexpected or unintended ones.

I'd argue that os.Exit should only be used as part of TestMain and never within a test, which covers the example given in the original post here, and which is the direction we were trying to take go test in.

Even if we choose to not do anything for the original issue here, I'm still strongly opposed to having go test sometimes provide tests with a real terminal, though.

[mvdan]

This thread started going back to the original os.Exit issue, so I've started a new proposal about go test and terminal outputs at https://github.com/golang/go/issues/34877.

[rsc]

The other thing we could do, if this is a serious problem worth fixing, is to let package testing inform package os that a TestXxx function is in progress and os.Exit should not be called. Then it would panic if called (0 or non-zero).

[mvdan]

If modifying os.Exit directly seems acceptable, then that would definitely be a cleaner and more exhaustive solution than trying to guess what happened from peeking at a test binary's output. It would be a less magical version of what @seebs proposed in https://github.com/golang/go/issues/29062#issuecomment-540619565.

[andybons]

This is marked as early-in-cycle but we have about three weeks left before the freeze. Assuming you plan to continue work on this, @mvdan, I'm moving to Backlog. If not, please move to Unplanned. Thanks

[rsc]

Moving to proposal milestone. I'd like to get to consensus that we should do this at all.

[rsc]

@bradfitz points out that we often re-exec a test binary as a child process and expect it to do some useful work instead of running a test and then exit 0 during the "test function". Any change here that detected that would cause problems. We could fix the standard library but likely others have picked up this idiom too.

So "fail tests that invoke os.Exit(0) explicitly" will break tests that work today. We probably don't want to break all those tests and force people to rewrite them.

I don't see a way out of this. It seems like we should just continue to allow os.Exit(0). Does anyone see a way out?

[bcmills]

we often re-exec a test binary as a child process and expect it to do some useful work instead of running a test and then exit 0 during the "test function"

That's true, but to me that mostly implies that the parent cmd/go process needs to be involved in the final pass/fail decision. (A test binary running as a child process is generally invoked as os.Args[0] rather than going back out through go test.)

[bcmills]

Just spitballing, but here's one possible idea: hook os.Exit, but have it only fail the process if the parent PID matches some explicit PID passed to it by cmd/go (either as a flag or an environment variable).

[bradfitz]

That's true, but to me that mostly implies that the parent cmd/go process needs to be involved in the final pass/fail decision. (A test binary running as a child process is generally invoked as os.Args[0] rather than going back out through go test.)

I hadn't considered that, and I agree that's always the case in code I've written or seen.

Just spitballing, but here's one possible idea: hook os.Exit, but have it only fail the process if the parent PID matches some explicit PID passed to it by cmd/go (either as a flag or an environment variable).

SGTM. I'd probably use an environment variable, though, rather than having flag noise or a hidden flag.

In the meeting I'd proposed gross hacks of looking at ppid and trying to determine if it's you, etc, but couldn't come up with anything portable & not gross. But the parent passing down an environment variable "GO_FAIL_OS_EXIT_IF_PPID=1235" seems like it'd be portable & easy.

[rsc]

Instead of an environment variable, we could also define a -test.disallowexit0 flag and have cmd/go pass it when running tests. Any subprocess started by a test will not have that command-line argument passed to it. Thoughts?

[rsc]

(This flag would be special in that it would not correspond to a cmd/go flag. That is, you cannot run "go test -disallowexit0".)

[rsc]

Based on the discussion here, it sounds like we could possibly move forward with the following:

• Add a new flag in package testing, -test.panicexit0, which will be recognized when invoking test binaries.
• In the testing package, if -test.panicexit0 is given, use some backdoor into os to catch os.Exit(0).
• If os.Exit(0) is called, panic("os.Exit(0) during test") instead (we can't use t.Fatal because it might not be in the test goroutine, and panicking makes sure to show the full stack trace to the call).
• In cmd/go, invoke tests unconditionally with -test.panicexit0. There is no flag to disable this.

Any test that reinvokes the test binary sets its own command line, so it will not run afoul of this.

However, it is worth noting that now running "go test x" and "go test -c x && ./x.test" are a bit more different than they have been, in that the former panics on os.Exit(0) but the latter does not. (The former also has timeouts etc that the latter does not.)

Thoughts?

[rsc]

Any thoughts? @rhysd, @mvdan, @bcmills

[bcmills]

Either a flag or an environment variable seems ok by me.

The environment variable would be a bit easier for users to filter out when constructing subprocesses. (They might want to propagate other flags such as -test.timeout, and filtering flags seems more difficult than adding GO_FAIL_OS_EXIT0_IF_PPID= or similar to cmd.Env.)

[rhysd]

I'm not sure I could catch up all discussions here since I'm not fully understanding internal implementation of testing and use case of testing other than go test. But the suggestion by @rsc looks good to me as a go test user.

[gopherbot]

Change https://golang.org/cl/208483 mentions this issue: misc: log 'ok' from 'go run' tests on success

[mvdan]

Apologies for the late reply; I echo what @bcmills said.

[rsc]

Environment variable implies PPID tricks because you need some way to block it from the child. The nice thing about the flag is that no one reinvokes the test binary with all the original test flags so the flag gets cleared "for free". It's also more explicit. We have too much environment configuration already.

Based on the discussion here, the concrete steps in https://github.com/golang/go/issues/29062#issuecomment-553533170 seem like a likely accept.

Leaving open for a week for final comments.

[bcmills]

no one reinvokes the test binary with all the original test flags so the flag gets cleared "for free"

I have a Google-internal exectest package (a helper library for exec-based tests) that _does_ preserve the original test flags — it filters out only -test.run.

That said, it also does a lot of other non-portable things (like introspecting the program counter to figure out which function calls to skip or replay in the subprocess), so I would not be at all upset to have to filter out an additional flag.

[rsc]

No change in consensus. Accepted.

[dmitshur]

This issue is currently labeled as early-in-cycle for Go 1.15. That time is now, so friendly ping. If it no longer needs to be done early in cycle, that label can be removed.

[gopherbot]

Change https://golang.org/cl/222243 mentions this issue: cmd/go: make go test -json report failures for panicking/exiting tests

[gopherbot]

Change https://golang.org/cl/222658 mentions this issue: [release-branch.go1.14] cmd/go: make go test -json report failures for panicking/exiting tests

[dmitshur]

This issue is currently labeled as early-in-cycle for Go 1.16.
That time is now, so this is a friendly ping so the issue is looked at again.

[gopherbot]

Change https://golang.org/cl/250977 mentions this issue: cmd/go, testing, os: fail test that calls os.Exit(0)

[gopherbot]

Change https://golang.org/cl/251262 mentions this issue: testing: restore os.Exit(0) after every call to (*M).Run