-
-
Notifications
You must be signed in to change notification settings - Fork 650
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
panic(nil)
in a test results in it immediately passing
#167
Conversation
Yes we should fix this, even if it breaks existing suites (which are likely incorrectly passing) Do you think you have the bandwidth for a PR? |
Yep, happy to help! |
- Does not apply to goroutines that "defer GinkgoRecover()"
Done. Let me know what you think. A few points:
|
Ω(didRun).Should(BeTrue()) | ||
|
||
Ω(outcome).Should(Equal(types.SpecStatePanicked)) | ||
Ω(failure.ForwardedPanic).Should(Equal("<nil>")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this preferable to a more explicit, hardcoded error like test executed panic(nil) or runtime.Goexit
in the standard library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's getting wrapped with the "Test Panicked" message in failer.go
. That looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on whether this message should be more verbose. On one hand, <nil>
is consistent with how Ginkgo normally behaves when you panic (and as you mentioned, Ginkgo already indicates that the test panicked). On the other hand, you'll get this same message when you call runtime.Goexit()
, and that could be confusing.
yep not much to be done with |
`panic(nil)` in a test results in it immediately passing
This behavior was identical to that of the built-in
testing
package, but it changed over a year ago: golang/go#6546Relevant sections:
This makes it easy to unknowingly skip assertions, so it seems pretty dangerous. That said, fixing it may break some people's test suites.
(CC @rosenhouse)