fix(suite) correctly set stats on test panic

[nicoche]

Summary

In testify, in tests that panic(), WithStats is not correctly ended. See #1189 for details

Changes

probably not the prettiest fix but it works. Let me know what you think

Motivation

Looks like a bug

Related issues

Closes #1189

[boyan-soubachov]

Wouldn't this break the recover() in failOnPanic(t) that gets called later?

IIRC defers are stored in a stack, so this function would be executed first and executing this recover() without 're-panicing' would break the subsequent, failOnPanic function?

[nicoche]

This is a good remark.

First, failOnPanic(t) is a no-op if it is called in a context where it does not panic.

Then, you are right: in case a test panics, this function takes precedence over the failOnPanic(t) L139 and failOnPanic(t) will be a no-op. But I added the body of failOnpanic(t) L157:L162, so this way, we don't change the current behavior:

t.Errorf("test panicked: %v\n%s", r, debug.Stack())
t.FailNow()

We can define a subfunction for those two lines if needed.

Note that the failOnPanic(t) is still relevant because the current function may panic. That is because it may run user code via AfterTest() or TearDownTestSuite()

[boyan-soubachov]

Would it make sense to refactor said failOnPanic() body (L159 - L160) out into a common function that can then be called from here and failOnPanic()?

[nicoche]

Good point. I renamed failOnPanic(t) to recoverAndFailOnPanic(t) and called the common function failOnPanic(t, r)

[boyan-soubachov]

Looks good, thank you!