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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Jun 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions suite/suite.go
View file
Open in desktop
Expand Up @@ -67,8 +67,12 @@ func (suite *Suite) Assert() *assert.Assertions {
return suite.Assertions
}

func failOnPanic(t *testing.T) {
func recoverAndFailOnPanic(t *testing.T) {
r := recover()
failOnPanic(t, r)
}

func failOnPanic(t *testing.T, r interface{}) {
if r != nil {
t.Errorf("test panicked: %v\n%s", r, debug.Stack())
t.FailNow()
Expand All @@ -91,7 +95,7 @@ func (suite *Suite) Run(name string, subtest func()) bool {
// Run takes a testing suite and runs all of the tests attached
// to it.
func Run(t *testing.T, suite TestingSuite) {
defer failOnPanic(t)
defer recoverAndFailOnPanic(t)

suite.SetT(t)

Expand Down Expand Up @@ -136,10 +140,12 @@ func Run(t *testing.T, suite TestingSuite) {
F: func(t *testing.T) {
parentT := suite.T()
suite.SetT(t)
defer failOnPanic(t)
defer recoverAndFailOnPanic(t)
defer func() {
r := recover()
Copy link
Collaborator

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

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

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

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)


if stats != nil {
passed := !t.Failed()
passed := !t.Failed() && r == nil
stats.end(method.Name, passed)
}

Expand All @@ -152,6 +158,7 @@ func Run(t *testing.T, suite TestingSuite) {
}

suite.SetT(parentT)
failOnPanic(t, r)
}()

if setupTestSuite, ok := suite.(SetupTestSuite); ok {
Expand Down
29 changes: 23 additions & 6 deletions suite/suite_test.go
View file
Open in desktop
Expand Up @@ -501,19 +501,36 @@ func (s *suiteWithStats) TestSomething() {
s.Equal(1, 1)
}

func (s *suiteWithStats) TestPanic() {
panic("oops")
}

func TestSuiteWithStats(t *testing.T) {
suiteWithStats := new(suiteWithStats)
Run(t, suiteWithStats)

testing.RunTests(allTestsFilter, []testing.InternalTest{
{
Name: "WithStats",
F: func(t *testing.T) {
Run(t, suiteWithStats)
},
},
})

assert.True(t, suiteWithStats.wasCalled)
assert.NotZero(t, suiteWithStats.stats.Start)
assert.NotZero(t, suiteWithStats.stats.End)
assert.True(t, suiteWithStats.stats.Passed())
assert.False(t, suiteWithStats.stats.Passed())

testStats := suiteWithStats.stats.TestStats

assert.NotZero(t, testStats["TestSomething"].Start)
assert.NotZero(t, testStats["TestSomething"].End)
assert.True(t, testStats["TestSomething"].Passed)

testStats := suiteWithStats.stats.TestStats["TestSomething"]
assert.NotZero(t, testStats.Start)
assert.NotZero(t, testStats.End)
assert.True(t, testStats.Passed)
assert.NotZero(t, testStats["TestPanic"].Start)
assert.NotZero(t, testStats["TestPanic"].End)
assert.False(t, testStats["TestPanic"].Passed)
}

// FailfastSuite will test the behavior when running with the failfast flag
Expand Down