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: testing: t.Cleanup should run on panic #41355

Closed
powerman opened this issue Sep 12, 2020 · 8 comments
Closed

proposal: testing: t.Cleanup should run on panic #41355

powerman opened this issue Sep 12, 2020 · 8 comments
Labels
Milestone

Comments

@powerman
Copy link

@powerman powerman commented Sep 12, 2020

There was a discussion about gomock hang on panic because it's ctrl.Finish() was called inside t.Cleanup instead of defer (golang/mock#428). It helped me to realize t.Cleanup won't be called in case of panic, as I (and probably others too) was expected.

At a glance it looks feasible to change testing package to add few internal defer to ensure t.Cleanup will be called on panic/t.FailNow. And as current documentation doesn't mention is this will happens or not we can consider this change compatible.

In case this proposal will be rejected I think it's worth to update the doc and make explicit that t.Cleanup won't be called on panic, because this limits it usability a lot and probably contradict user's expectations.

@gopherbot gopherbot added this to the Proposal milestone Sep 12, 2020
@gopherbot gopherbot added the Proposal label Sep 12, 2020
@mvdan
Copy link
Member

@mvdan mvdan commented Sep 12, 2020

@mvdan mvdan changed the title proposal: testing: t.Cleanup should runs on panic proposal: testing: t.Cleanup should run on panic Sep 12, 2020
@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Sep 12, 2020

Cleanup definitely should run on panic. That was always part of the design (and the original code at least tested for that AFAIR) and if it doesn't, it should be considered a bug IMHO. Furthermore, even if one Cleanup function panics, the others should still run.
.

@mvdan
Copy link
Member

@mvdan mvdan commented Sep 12, 2020

Pinging @ianlancetaylor and @bradfitz too, who have worked on Cleanup or reviewed CLs for it. I imagine this issue is straightforward enough to not need a proposal.

@powerman
Copy link
Author

@powerman powerman commented Sep 12, 2020

Okay, I'm sorry, probably I misunderstood something. t.Cleanup do run on panic, the issue is somewhere else. Probably execution environment inside t.Cleanup somehow differ from usual defer:

package main_test

import (
	"testing"

	"github.com/golang/mock/gomock"
)

//go:generate mockgen -package=$GOPACKAGE -source=$GOFILE -destination=mock.$GOFILE T
type T interface{ F() }

func TestCleanupOnPanic(t *testing.T) {
	ctrl := gomock.NewController(t)
	cleanup := func() {
		println("enter cleanup")
		ctrl.Finish()
		println("leave cleanup")
	}
	mockT := NewMockT(ctrl)
	mockT.EXPECT().F()

	t.Cleanup(cleanup) // (1)
	// defer cleanup() // (2)
	t.Run("", func(t *testing.T) {
		panic("boom")
	})
	// cleanup() // (3)
}

Running go test on this result in:

enter cleanup
(it hangs)

But if we uncomment (2) or (3) and comment out (1) it'll work better (don't swallow panic and hang at least, but it still won't output error from gomock about missing call):

--- FAIL: TestCleanupOnPanic (0.00s)
    --- FAIL: TestCleanupOnPanic/#00 (0.00s)
panic: boom [recovered]
	panic: boom

And only if we'll remove t.Run we'll get error from gomock about missing call to F().

TBH I'm unsure now is this issue actually about Go itself or it's related only to gomock. Feel free to close if you believe it's only about gomock.

@powerman
Copy link
Author

@powerman powerman commented Sep 12, 2020

It looks like in case of panic inside t.Run neither (2) nor (3) will be executed at all, but (1) will be executed.
At same time if we replace panic inside t.Run with t.FailNow - all (1), (2) and (3) will be executed and will work as expected.
So, maybe something is wrong with panic inside t.Run case?

It is actually really weird panic inside t.Run somehow result in not executing defer defined before.

@powerman
Copy link
Author

@powerman powerman commented Sep 12, 2020

package main_test

import "testing"

func TestDeferOnPanicInsideRun(t *testing.T) {
	defer println("never here")
	t.Run("", func(t *testing.T) {
		panic("boom")
	})
}

Output:

$ go test
--- FAIL: TestDeferOnPanicInsideRun (0.00s)
    --- FAIL: TestDeferOnPanicInsideRun/#00 (0.00s)
panic: boom [recovered]
	panic: boom

goroutine 19 [running]:
testing.tRunner.func1.1(0x51a7c0, 0x56bb50)
	/usr/lib/go/src/testing/testing.go:1076 +0x30d
testing.tRunner.func1(0xc000086600)
	/usr/lib/go/src/testing/testing.go:1079 +0x41a
panic(0x51a7c0, 0x56bb50)
	/usr/lib/go/src/runtime/panic.go:969 +0x175
tmp_test.TestDeferOnPanicInsideRun.func1(0xc000086600)
	/home/powerman/proj/go/tmp/cleanup-on-panic/main_test.go:8 +0x39
testing.tRunner(0xc000086600, 0x54d038)
	/usr/lib/go/src/testing/testing.go:1127 +0xef
created by testing.(*T).Run
	/usr/lib/go/src/testing/testing.go:1178 +0x386
exit status 2
FAIL	tmp	0.004s
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 14, 2020

Change https://golang.org/cl/254637 mentions this issue: testing: fix panicking tests hang if Cleanup calls FailNow

@changkun
Copy link
Contributor

@changkun changkun commented Sep 14, 2020

But if we uncomment (2) or (3) and comment out (1) it'll work better (don't swallow panic and hang at least, but it still won't output error from gomock about missing call)

The hang happens if testing.T.FailNow is called in a panicking test withCleanup. Here is a simple reproducer:

func TestFailNowCleanup(t *testing.T) {
	t.Cleanup(func() {
		t.FailNow()
	})
	t.Run("x", func(t *testing.T) {
		panic("die")
	})
}

@powerman In your example, ctrl.Finish() will call testing.T.Fatalf eventually, and it invokes testing.T.FailNow.

In a panicking test, all cleanups are running in the panicking goroutine.

if r := t.runCleanup(recoverAndReturnPanic); r != nil {

The testing.T.FailNow will call runtime.Goexit before the actual panic:

panic(err)

Since it failed to call panic and the goroutine is early exited, and the test signal that supposes to send to a parent goroutine is not performed. Therefore it hangs.

I've sent a CL254637 for this. Let's see if the fix is worthwhile. /cc @mvdan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.