Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: cmd/go: fail tests that invoke os.Exit(0) explicitly #29062

Open
rhysd opened this issue Dec 2, 2018 · 33 comments
Assignees
Milestone

Comments

@rhysd
Copy link

@rhysd rhysd commented Dec 2, 2018

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

@rhysd rhysd changed the title 'go test' successfully exits even if the program calls os.Exit(0) 'go test' successfully exits even if some test calls os.Exit(0) Dec 2, 2018
@mvdan mvdan added the NeedsDecision label Dec 2, 2018
@mvdan mvdan changed the title 'go test' successfully exits even if some test calls os.Exit(0) cmd/go: test seems to succeed even if some test calls os.Exit(0) Dec 2, 2018
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Dec 2, 2018

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

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Dec 2, 2018

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

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Dec 4, 2018

@mvdan: That fix sounds like the right one.

@bcmills bcmills added this to the Go1.13 milestone Dec 4, 2018
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 4, 2018

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

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Dec 4, 2018

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

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 5, 2018

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

@bcmills bcmills changed the title cmd/go: test seems to succeed even if some test calls os.Exit(0) cmd/go: fail tests that invoke os.Exit(0) explicitly Jan 17, 2019
@mvdan mvdan self-assigned this Jul 1, 2019
@mvdan mvdan added NeedsFix and removed NeedsDecision labels Jul 1, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 1, 2019

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

@eliben

This comment has been minimized.

Copy link
Member

@eliben eliben commented Aug 31, 2019

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 #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

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Sep 9, 2019

Historically, it has been acceptable to break user programs that were doing the opposite of what is documented. For example, see #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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 9, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 9, 2019

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

@bcmills bcmills reopened this Oct 9, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 9, 2019

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.

gopherbot pushed a commit that referenced this issue Oct 9, 2019
This reverts CL 184457.

Reason for revert: introduced failures in the regression test for #18153.

Fixes #34791
Updates #29062

Change-Id: I4040965163f809083c023be055e69b1149d6214e
Reviewed-on: https://go-review.googlesource.com/c/go/+/200106
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Contributor

@seebs seebs commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 11, 2019

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

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Oct 11, 2019

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 comment has been minimized.

Copy link
Member

@mvdan mvdan commented Oct 13, 2019

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 #34877.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 15, 2019

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

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Oct 15, 2019

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 #29062 (comment).

@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Oct 15, 2019

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

@andybons andybons modified the milestones: Go1.14, Backlog Oct 15, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 21, 2019

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

@rsc rsc added the Proposal label Oct 21, 2019
@rsc rsc modified the milestones: Backlog, Proposal Oct 21, 2019
@rsc rsc changed the title cmd/go: fail tests that invoke os.Exit(0) explicitly proposal: cmd/go: fail tests that invoke os.Exit(0) explicitly Oct 21, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 30, 2019

@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

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 31, 2019

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

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 31, 2019

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

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 31, 2019

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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 6, 2019

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 comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 6, 2019

(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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 13, 2019

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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 20, 2019

Any thoughts? @rhysd, @mvdan, @bcmills

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Author

@rhysd rhysd commented Nov 21, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.