-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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: complain loudly during concurrent use of T.FatalX and T.SkipX #15758
Comments
Seems infeasible. FailNow/SkipNow are fundamentally built around runtime.Goexit, and there is no way to tell whether they are being called from the "right" goroutine. Bad API design maybe, but it's the way it is. We can't change from runtime.Goexit to a panic in general; the whole point of using Goexit was that it does not look like a panic and cannot be stopped. I don't see any way to change the current behavior nor do I see any way to make the current behavior louder. |
@rsc, the http2 package has a testing-time-only mechanism to track that certain methods only run on the expected goroutines. See https://github.com/golang/net/blob/master/http2/gotrack.go#L22 We could do something like that. |
I've been wondering if we should take the goroutine ID out of the runtime.Stack output, precisely for this reason. :-( |
@rsc, that'd be fine, if you give me an as-useful debugging primitive elsewhere. |
Reopening as @balshetzer has an approach that may work, based on looking for certain functions in the stack trace. I guess we can give it a try and see if it seems reasonable. |
For posterity, here is the permalink that @bradfitz referred to in his comment #15758 (comment) https://github.com/golang/net/blob/2491c5de3490fced2f6cff376127c667efeed857/http2/gotrack.go#L22 |
Another option here is to remove the doc and allow it. See #25467 |
@nhooyr, If we were to allow It is of course possible for the author of the test to plumb in some sort of cancellation or synchronization mechanism, but if they have done that, then they can just as easily invoke that mechanism instead of calling |
@bcmills We would change t.Fatal to report a error and exit whatever goroutine its called from. I don't think anyone would expect it to exit the main goroutine too. |
Change https://golang.org/cl/134395 mentions this issue: |
It turns out to be possible to propagate a |
See previously #3800. |
Change https://go.dev/cl/416555 mentions this issue: |
Note that one special case of calling |
The
testing
package is explicit about the following:However, this is easy to overlook and it is not immediately obvious that the code below is a bad test:
This currently outputs (what a user expects):
However, since t.Fatal is not safe to call outside of the Test function, this is poor feedback since it makes the user think that the test is correctly written when it is actually racy. Instead it should complain loudly that this is wrong.
As I'm debugging poor tests, I've noticed that the calling of t.Fatal in a goroutine is actually a fairly common phenomenon.
Related: #15674
/cc @mpvl @bradfitz
The text was updated successfully, but these errors were encountered: