Skip to content
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

fix(suite) correctly set stats on test panic #1195

Merged
merged 1 commit into from Jun 23, 2022

Conversation

nicoche
Copy link
Contributor

@nicoche nicoche commented May 31, 2022

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

@nicoche nicoche changed the title suite: correctly set stats on test panic fix(suite) correctly set stats on test panic May 31, 2022
@@ -138,8 +138,10 @@ func Run(t *testing.T, suite TestingSuite) {
suite.SetT(t)
defer failOnPanic(t)
defer func() {
r := recover()
Copy link
Collaborator

@boyan-soubachov boyan-soubachov Jun 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@nicoche nicoche Jun 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Collaborator

@boyan-soubachov boyan-soubachov Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Contributor Author

@nicoche nicoche Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nicoche nicoche force-pushed the suite-fix-stats-on-panic branch from 0f1e373 to 9fcb1a4 Compare Jun 22, 2022
Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Looks good, thank you!

@boyan-soubachov boyan-soubachov merged commit 1b73601 into stretchr:master Jun 23, 2022
3 checks passed
@nicoche nicoche deleted the suite-fix-stats-on-panic branch Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants