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

Jump to bottom

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

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

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

cznic opened this issue Oct 12, 2016 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@cznic
Copy link
Contributor

cznic commented Oct 12, 2016 •
edited

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 The path to resolution is known, but the work has not been done. label Oct 12, 2016
@spenczar
Copy link
Contributor

spenczar commented Oct 13, 2016 •
edited

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
Copy link
Contributor Author

cznic 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!

@cznic cznic closed this as completed Oct 13, 2016
@cznic cznic mentioned this issue Jul 26, 2017
Closed
@gopherbot
Copy link

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
@ALTree
testing: explain how SkipNow and FailNow stop execution
bd74fd3 
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>
@Soulou Soulou mentioned this issue Aug 23, 2017
Closed
@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
4 participants
@quentinmit @cznic @spenczar @gopherbot