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

testing: Calling t.Fatal in a subtest panics #17421

Closed
cznic opened this issue Oct 12, 2016 · 3 comments

Comments

Projects
None yet
4 participants
@cznic
Copy link
Contributor

commented Oct 12, 2016

Please answer these questions before submitting your issue. Thanks!

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

go version go1.7.1 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jnml"
GORACE=""
GOROOT="/home/jnml/go"
GOTOOLDIR="/home/jnml/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build509817202=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

jnml@r550:~$ go get -u github.com/cznic/gc
jnml@r550:~$ cd $GOPATH/src/github.com/cznic/gc
jnml@r550:~/src/github.com/cznic/gc$ git checkout 421781a  
Note: checking out '421781a'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at 421781a... Subtest issue reproduction case.
jnml@r550:~/src/github.com/cznic/gc$ 

What did you expect to see?

Something like

jnml@r550:~/src/github.com/cznic/gc$ go test
...
=== RUN   TestParser/GOROOT
    --- FAIL: TestParser/GOROOT (0.00s)
        all_test.go:1084: TODO
...

What did you see instead?

jnml@r550:~/src/github.com/cznic/gc$ go test
panic: test executed panic(nil) or runtime.Goexit

goroutine 15 [running]:
panic(0x5fcc60, 0xc4202af730)
    /home/jnml/go/src/runtime/panic.go:500 +0x1a1
testing.tRunner.func1(0xc42041f440)
    /home/jnml/go/src/testing/testing.go:579 +0x25d
runtime.Goexit()
    /home/jnml/go/src/runtime/panic.go:340 +0xec
testing.(*common).FailNow(0xc42041fa40)
    /home/jnml/go/src/testing/testing.go:453 +0x39
testing.(*common).Fatal(0xc42041fa40, 0xc420067ed0, 0x1, 0x1)
    /home/jnml/go/src/testing/testing.go:490 +0x6f
github.com/cznic/gc.testParser(0xc42041fa40, 0xc420712000, 0x645, 0x800)
    /home/jnml/src/github.com/cznic/gc/all_test.go:1084 +0x2ab
github.com/cznic/gc.TestParser.func2(0xc42041f440)
    /home/jnml/src/github.com/cznic/gc/all_test.go:1096 +0x4b
testing.tRunner(0xc42041f440, 0xc42251a090)
    /home/jnml/go/src/testing/testing.go:610 +0x81
created by testing.(*T).Run
    /home/jnml/go/src/testing/testing.go:646 +0x2ec
exit status 2
FAIL    github.com/cznic/gc 6.783s
jnml@r550:~/src/github.com/cznic/gc$ 

Additional information

Running with -race produces the same output.

@quentinmit quentinmit added this to the Go1.8 milestone Oct 12, 2016

@quentinmit quentinmit added the NeedsFix label Oct 12, 2016

@spenczar

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2016

This appears to me to be a bug in the code in github.com/cznic/gc. The test in question looks like this:

func TestParser(t *testing.T) {
  cover := append(gorootTestFiles, ycover)
  _ = t.Run("Yacc", func(*testing.T) { testParserYacc(t, cover) }) &&
    t.Run("GOROOT", func(*testing.T) { testParser(t, cover) }) &&
    t.Run("States", testParserStates)

}

The t.Run calls are ignoring their parameter, so the t in the testParser call is TestParser's *testing.T, not the one that's passed to t.Run("GOROOT". It should look like this:

func TestParser(t *testing.T) {
  cover := append(gorootTestFiles, ycover)
  _ = t.Run("Yacc", func(t *testing.T) { testParserYacc(t, cover) }) &&
    t.Run("GOROOT", func(t *testing.T) { testParser(t, cover) }) &&
    t.Run("States", testParserStates)

}
@cznic

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2016

@spenczar So simple ;-)

--- FAIL: TestParser (4.19s)
    --- FAIL: TestParser/GOROOT (0.00s)
        all_test.go:1084: TODO
FAIL
exit status 1
FAIL    github.com/cznic/gc 6.841s

It's a trap. Thank you!

@gopherbot

This comment has been minimized.

Copy link

commented Aug 2, 2017

Change https://golang.org/cl/52770 mentions this issue: testing: explain how SkipNow and FailNow stop execution

gopherbot pushed a commit that referenced this issue Aug 9, 2017

testing: explain how SkipNow and FailNow stop execution
SkipNow and FailNow must be called from the goroutine running the
test. This is already documented, but it's easy to call them by
mistake when writing subtests. In the following:

  func TestPanic(t *testing.T) {
    t.Run("", func(t2 *testing.T) {
	  t.FailNow()    // BAD: should be t2.FailNow()
	})
  }

the FailNow call on the outer t *testing.T correctly triggers a panic

  panic: test executed panic(nil) or runtime.Goexit

The error message confuses users (see issues #17421, #21175) because
there is no way to trace back the relevant part of the message ("test
executed ... runtime.Goexit") to a bad FailNow call without checking
the testing package source code and finding out that FailNow calls
runtime.Goexit.

To help users debug the panic message, mention in the SkipNow and
FailNow documentation that they stop execution by calling
runtime.Goexit.

Fixes #21175

Change-Id: I0a3e5f768e72b464474380cfffbf2b67396ac1b5
Reviewed-on: https://go-review.googlesource.com/52770
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@golang golang locked and limited conversation to collaborators Aug 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.