Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

surface panic when calling Finish is implicit #478

Merged
merged 3 commits into from
Sep 11, 2020
Merged

surface panic when calling Finish is implicit #478

merged 3 commits into from
Sep 11, 2020

Conversation

codyoss
Copy link
Member

@codyoss codyoss commented Sep 1, 2020

When Finsh is called from Cleanup, it is not done so in a deffered
context. Because of this, we can't recover from panics. The
testing package will re-surface any panics but only after the
Cleanup functions is executed. Because we were calling t.Fatal we
were hiding panics from users of the library when they were using
Finish implicitly. Fatal calls runtime.Goexit() under the hood so
the panic never gets the chance to be handled by the testing
package.

Also, removed logging as after thinking about it more it seems a
little aggressive to push the feature onto users.

Note, when there is a panic and Finish is called from the Cleanup
function that a few extra lines will be logged from the calls to
ErrorF. This seems like a small price to pay to get the context
of the panic

Fixes: #428

When Finsh is called from Cleanup, it is not done so in a deffered
context. Because of this, we can't recover from panics. The
testing package will re-surface any panics but only after the
Cleanup functions is executed. Because we were calling t.Fatal we
were hiding panics from users of the library when they were using
Finish implicitly. Fatal calls runtime.Goexit() under the hood so
the panic never gets the chance to be handled by the testing
package.

Also, removed logging as after thinking about it more it seems a
little aggressive to push the feature onto users.

Note, when there is a panic and Finish is called from the Cleanup
function that a few extra lines will be logged from the calls to
ErrorF. This seems like a small price to pay to get the context
of the panic
@codyoss codyoss requested a review from cvgw September 1, 2020 22:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic in test is swallowed if there are failed expectations and t.Cleanup(ctrl.Finish)
2 participants