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: when using a custom TestMain, m.Run does not return if one of the tests it runs panics #37206
Comments
I've verified func testMain(m *testing.M) int {
defer func() {
if e := recover(); e != nil {
fmt.Println("there was a panic")
}
}()
setup()
defer teardown()
return m.Run()
} The string "there was a panic" doesn't get printed. It does get printed if there is a panic before func testMain(m *testing.M) int {
defer func() {
if e := recover(); e != nil {
fmt.Println("there was a panic")
}
}()
setup()
defer teardown()
panic("about to m.Run")
return m.Run()
} The code in This needs further investigation. /cc @mpvl @josharian per owners. |
#34129 is somewhat relevant, although the implementation for that issue probably will not address this one. (CC @changkun @abuchanan-nr) |
Change https://golang.org/cl/219977 mentions this issue: |
@bcmills Thanks for CC me. I basically agree with @dmitshur . The Go team must decide either document the behavior or allow testing.M.Run to return properly if there is a test panic. For the following code snippet:
outputs:
If we decide to allow testing.M.Run to return, then it will execute
|
We can never make this perfect. If some code in a test calls go func() { panic("die die die") }() then The only question here is how we should handle a test that panics in the goroutine used to run the test. Should we try to handle that specific case? Or should we just treat it like a panic in a different goroutine, which is in effect what we do now? I don't have a strong opinion about that. But the testing package is already fairly baroque. Is it really worth complicating it further to handle one specific case when we can't handle other similar cases? |
No, we can't do anything about it yet. I didn't write any test regarding user goroutine panics and left this case to the CL's reviewers, and I think this particular case can simply be ignored because this type of panic happens in a user goroutine. The testing package has done everything it could achieve (panics happens directly in tests and subtests). If we interested in handling this particular case, more investigation could be done with subsequent CLs. |
I suppose I'm asking for opinions. Is it worth complicating the testing package to return from |
Thank you for explaining the current situation and trade-offs in #37206 (comment), @ianlancetaylor. My opinion is that we should hold off on complicating the |
I was answering an opinion. This particular case can simply be ignored because this type of panic happens in a user goroutine. |
If I get CL 134395 cleaned up and merged, it will be fairly straightforward for users to use an Moreover, I suspect that the vast majority of tests do not explicitly spawn goroutines, let alone goroutines that may panic. It seems reasonable for users to expect that their best-effort cleanup using |
At least part of the complexity of the implementation seems to arise because the runtime does not provide a mechanism for a program to re-raise an existing panic without altering its stack trace. (I've discussed that deficiency with at least @aclements before, but I don't see an issue filed for it.) If we provided a general mechanism to re-panic without altering the stack trace, then I think the (CC @danscales) |
This might be already off-topic: Was there any existed discussion on registering global handler of panics? It seems (server-side) Go users nowadays avoid to use panic/recover pair, just because they do not want their service down by accident, and there is no way to capture a panic outside a goroutine. The runtime also does not have this privilege. |
I don't think a global panic handler would be an appropriate solution, here or in servers. An uncaught |
I don't see any way of doing it, handling all panics seems to be equivalent to the status in GC and mutators. That is to say, a user goroutine may panic intentionally, the runtime should not do anything with it by default. This basically matches what I said in the beginning: "The testing package has done everything it could achieve with the CL. If we interested in handling this particular panic case, more investigation could be done in subsequent CLs." cc @dmitshur |
Good point. Agree. I just randomly threw some virgin ideas, since I didn't experience much use cases regarding capturing a panic outside a goroutine except this, not suggesting anything :) |
I personally am not convinced by "let's do this now and figure out how to do more later." Sometimes that is the right approach, but sometimes it's important to at least understand how we could do more later. Right now people are confused because when they |
I don't think we do need to “figure out how to do more later”. We should structure our own code to propagate panics from non-main goroutines back to the main goroutine, and encourage users to do the same. Especially for non- |
I am also agreed with you about the part " ... 'let's do this now and figure out how to do more later.' ..." is not a good decision in this case. However, the current situation is I don't see any way of doing it better than the proposed CL because we cannot differentiate an accidental panic and an intentional panic in a spawned goroutine from the runtime unless people give a clear indication to the TestMain or equivalent. This knob makes the usage and behavior even more complex than the proposed CL. If we carefully think about panics propagate out from a test goroutine, things become more subtle. Go users were taught to fail a test by I personally argue this is the best action we can offer and there are no further thoughts from my side. |
Change https://golang.org/cl/221321 mentions this issue: |
It appears that the proposed solution would propagate the panic to TestMain. An alternative would be that a panic within a test instead triggers an immediate failure of that test but allows the other tests to continue, such that the panic is never propagated to TestMain but arguably never needs to be. On the downside, this may encourage test writers to deliberately call panic to fail -- but I'm not sure we're at risk of that becoming common practice. On the plus side, I think this alternative might be:
|
@progressnerd, an unexpected panic potentially leaves the program in an arbitrarily corrupted state. Swamping the panic in noise from subsequent test failures — or worse, deadlocks — would make the panic harder to diagnose, in addition to delaying the output of the panic, for at best marginal additional information from the test. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
I wrote a function, a test for it, and
TestMain
to perform setup and teardown for testing this function:main.go
p_test.go
main_test.go
What did you expect to see?
I expected to see "setting up" and "tearing down" at some point, and a panic "foo" message with a stack trace.
What did you see instead?
Only setup and panic output appeared:
The program probably called
os.Exit
at some point while running the tests, or panicked in a separate go routine with no way for me to recover. I would expect both setup and teardown functions to be called at some point.The text was updated successfully, but these errors were encountered: