testing: t.Failed() returns false during panic
gbuenoandrade opened this issue · 4 comments
Does this issue reproduce with the latest release?
Yes (1.17.3)
What operating system and processor architecture are you using (go env
)?
Any
What did you do?
Called t.Failed()
from a deferred cleanup function registered for a test that panicked (see https://go.dev/play/p/f0Xu8OyA--h).
What did you expect to see?
t.Failed()
return true.
What did you see instead?
t.Failed()
returned false.
The issue isn't verified if the test has parallel subtests (see https://go.dev/play/p/37YXOUyv5-z). I believe that's because in that case this branch will be ignored, and this closure will call t.Fail()
before invoking the cleanup functions .
https://go.dev/play/p/THKjlNkot6b
package main
import (
"testing"
)
func Test(t *testing.T) {
defer func() { println("defer", t.Failed()) }()
t.Cleanup(func() { println("Cleanup", t.Failed()) })
panic(1)
}
Changing the panic to t.Fatal("foo") does say it failed, so panic probably ideally would do the same.
The problem is it really can't, because the only way to find out about a panic is a recover in a deferred function,
and the handler that will mark the test failed runs after the more recently deferred stuff.
We might be able to special case t.Cleanup, but it wouldn't help the plain defer.
On further thought, there's a clear distinction between defer and cleanup. Deferred func can recover the panic and stop the failure. The cleanup cannot. So it makes sense for cleanup to see t.Failed and the defer not to. I see the bug in the Cleanup case and will send a CL (but not for Go 1.18).
Thanks for looking into this, @rsc!
So it makes sense for cleanup to see t.Failed and the defer not to
I agree. Also, currently we are being a bit inconsistent since t.Failed
does return true
from a cleanup function if the test has parallel subtests:
https://go.dev/play/p/G_BE1zxkcEG
package main
import (
"testing"
)
func Test(t *testing.T) {
t.Cleanup(func() { println("Cleanup", t.Failed()) })
t.Run("", func(t *testing.T) { t.Parallel() })
panic(1)
}
Prints: Cleanup true
.
Rolling forward to 1.20. Please comment if you disagree. Thanks.