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.Failed() returns false during panic #49929

Open
gbuenoandrade opened this issue Dec 2, 2021 · 5 comments
Open

testing: t.Failed() returns false during panic #49929

gbuenoandrade opened this issue Dec 2, 2021 · 5 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@gbuenoandrade
Copy link

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 .

@rsc rsc changed the title testing: t.Failed() returns false from cleanup function despite test panicking testing: t.Failed() returns false during panic Dec 3, 2021
@rsc
Copy link
Contributor

rsc commented Dec 3, 2021

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.

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 3, 2021
@mknyszek mknyszek added this to the Backlog milestone Dec 3, 2021
@rsc
Copy link
Contributor

rsc commented Dec 3, 2021

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).

@rsc rsc self-assigned this Dec 3, 2021
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 3, 2021
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 3, 2021
@rsc rsc modified the milestones: Backlog, Go1.19 Dec 3, 2021
@gbuenoandrade
Copy link
Author

gbuenoandrade commented Dec 4, 2021

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.

@ianlancetaylor
Copy link
Contributor

Rolling forward to 1.20. Please comment if you disagree. Thanks.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Go1.20 Jun 24, 2022
@gopherbot gopherbot modified the milestones: Go1.20, Go1.21 Feb 1, 2023
@gopherbot gopherbot modified the milestones: Go1.21, Go1.22 Aug 8, 2023
@ansel1
Copy link

ansel1 commented Jan 9, 2024

I believe CL 506755, which was a fix for #61034, also fixes this. That CL was reverted due to an issue it introduced, but hopefully it will be resubmitted eventually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: No status
Development

No branches or pull requests

7 participants