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: t.FailNow() in a deferred function masks test panic #29207

Open
matthewmcnew opened this issue Dec 13, 2018 · 8 comments
Open

testing: t.FailNow() in a deferred function masks test panic #29207

matthewmcnew opened this issue Dec 13, 2018 · 8 comments
Labels
NeedsInvestigation
Milestone

Comments

@matthewmcnew
Copy link

@matthewmcnew matthewmcnew commented Dec 13, 2018

When a test panics, any deferred functions will still process allowing the test to properly clean up. If a deferred function executes t.FailNow() the test output will not include any information on the panic or the panic's goroutine trace.

An example is shown here: https://play.golang.org/p/gWPDVluKaST
Despite that test clearly panicking with a t.FailNow() in the deferred function the test output only shows:

--- FAIL: TestPanic (0.00s)
FAIL

The desired behavior would be along the lines of this example: https://play.golang.org/p/qrLtmYnkqAh. (Where the deferred function calls runtime.Goexit() instead) This produces the following output:

--- FAIL: TestPanic (0.00s)
panic: Test Panic
	panic: test executed panic(nil) or runtime.Goexit

goroutine 5 [running]:
testing.tRunner.func1(0x4560b0, 0xf)
	/usr/local/go/src/testing/testing.go:792 +0x460
runtime.Goexit()
	/usr/local/go/src/runtime/panic.go:397 +0x140
main.TestPanic.func1()
	/tmp/sandbox284554843/main.go:18 +0x20
panic(0x121940, 0x165a58)
	/usr/local/go/src/runtime/panic.go:513 +0x240
main.TestPanic(0x4560b0, 0xbab699fc)
	/tmp/sandbox284554843/main.go:20 +0x60
testing.tRunner(0x4560b0, 0x1558f8)
	/usr/local/go/src/testing/testing.go:827 +0x140
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:878 +0x3c0

I believe this is the result of this line:

if !t.finished && err == nil {

!t.finished will return false because t.Now() marks the test finished and err will be nil because the runtime.Goexit() in t.FatalNow() is what is being recovered (not the original panic).

This prevents the expected panic on this line:

panic(err)

@bcmills bcmills changed the title testing.T.FailNow() masks test panic testing: (*T).FailNow() in a deferred function masks test panic Dec 13, 2018
@bcmills bcmills added the NeedsInvestigation label Dec 13, 2018
@bcmills bcmills added this to the Go1.13 milestone Dec 13, 2018
@bcmills
Copy link
Member

@bcmills bcmills commented Dec 13, 2018

Probably FailNow should check for a panic, but in order to distinguish a panic from a runtime.GoExit we must attempt to recover it (#25448 (comment)), and that destroys the stack information associated with the original panic. (I think I discussed that caveat with @aclements at GopherCon...)

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 13, 2018

CC @mpvl @josharian

@matthewmcnew matthewmcnew changed the title testing: (*T).FailNow() in a deferred function masks test panic testing: t.FailNow() in a deferred function masks test panic Dec 13, 2018
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 13, 2018

I don't see how testing.FailNow could test for a panic. Calling recover won't work, as recover only returns non-nil if called directly from a deferred function, which would not be the case if it is called by testing.FailNow.

What does this come up, though? Why defer a call to FailNow or Fatal?

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 13, 2018

Hmm, good point. FailNow itself can't check for a panic, but the test runner can.

Unfortunately, it appears that runtime.Goexit currently cancels a panic (https://play.golang.org/p/xO5kMrMQkVS). I'm not sure whether that is a bug or the intended behavior of Goexit.

@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Mar 5, 2019

What does this come up, though? Why defer a call to FailNow or Fatal?

Test helpers that are deferred. E.g. to cleanup temp files.

@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Mar 5, 2019

I could change calls to t.Fatal in deferred functions to be t.Error and then return, but imo this should just work to make error handling code in the test itself and deferred functions to maintain the same structure.

@andybons andybons removed this from the Go1.13 milestone Jul 8, 2019
@andybons andybons added this to the Go1.14 milestone Jul 8, 2019
@rsc rsc removed this from the Go1.14 milestone Oct 9, 2019
@rsc rsc added this to the Backlog milestone Oct 9, 2019
@jim-slattery-rs
Copy link

@jim-slattery-rs jim-slattery-rs commented Feb 28, 2022

@seankhliao: is the t.Cleanup issue #51395 definitely a duplicate of this?

I'm wondering whether the t.Cleanup functions introduced in 2020 might be a different path that could be handled better, even if this issue with deferred functions remains open.

@seankhliao
Copy link
Member

@seankhliao seankhliao commented Feb 28, 2022

Cleanup is shorthand for, and can only be implemented as defer. I don't see how you could fix Cleanup without fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

8 participants