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:
|
There was a problem hiding this comment.
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.
Ok, it's getting wrapped with the "Test Panicked" message in failer.go. That looks good.
There was a problem hiding this comment.
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
testingpackage, 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)