Repository: stretchr/testify (go)

Issue:
suite: WithStats incorrect for tests that panic
Hey!

First, thanks for maintaining the lib.

I've stumbled upon an issue when using https://pkg.go.dev/github.com/stretchr/testify/suite#WithStats. It seems that tests that panic are marked as `Passed` even though they fail and they are are failures for `go test`.

repro:
```golang

package m

import (
	"fmt"
	"testing"
	"github.com/stretchr/testify/suite"
)

type ExampleTestSuite struct {
    suite.Suite
}

func (suite *ExampleTestSuite) HandleStats(suiteName string, stats *suite.SuiteInformation) {
	fmt.Printf("Test passed? %t\n", stats.TestStats["TestExample"].Passed)
}

func (suite *ExampleTestSuite) TestExample() {
	panic("x")
}

func TestExampleTestSuite(t *testing.T) {
    suite.Run(t, new(ExampleTestSuite))
}
```

output:

```
$ go test
Test passed? true
--- FAIL: TestExampleTestSuite (0.00s)
    --- FAIL: TestExampleTestSuite/TestExample (0.00s)
        suite.go:63: test panicked: x
            goroutine 20 [running]:
            runtime/debug.Stack()
            	/usr/local/go/src/runtime/debug/stack.go:24 +0x65
            github.com/stretchr/testify/suite.failOnPanic(0xc00011e4e0)
            	/Users/nicolas/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:63 +0x3e
            panic({0x132be20, 0x1414228})
            	/usr/local/go/src/runtime/panic.go:838 +0x207
            example.com/m.(*ExampleTestSuite).TestExample(0x0?)
            	/Users/nicolas/playground/m_test.go:18 +0x27
            reflect.Value.call({0xc0000a5140?, 0xc0000a2cc0?, 0x13?}, {0x138c249, 0x4}, {0xc000055e70, 0x1, 0x1?})
            	/usr/local/go/src/reflect/value.go:556 +0x845
            reflect.Value.Call({0xc0000a5140?, 0xc0000a2cc0?, 0x1310d3f?}, {0xc000042670, 0x1, 0x1})
            	/usr/local/go/src/reflect/value.go:339 +0xbf
            github.com/stretchr/testify/suite.Run.func1(0xc00011e4e0)
            	/Users/nicolas/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:158 +0x4b6
            testing.tRunner(0xc00011e4e0, 0xc0000f2090)
            	/usr/local/go/src/testing/testing.go:1439 +0x102
            created by testing.(*T).Run
            	/usr/local/go/src/testing/testing.go:1486 +0x35f
FAIL
exit status 1
FAIL	example.com/m	0.169s
```

It looks like it can be fixed by inverting those two defer: https://github.com/stretchr/testify/blob/master/suite/suite.go#L139-L155. 

Let me know if this looks like a legit bug to you or not; I'd be happy to contribute with a fix!

Pull request description:
fix(suite) correctly set stats on test panic
## 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 

Patch:
diff --git a/suite/suite.go b/suite/suite.go
index 1c402e8..8955918 100644
--- a/suite/suite.go
+++ b/suite/suite.go
@@ -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()
@@ -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)
 
@@ -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()
+
 					if stats != nil {
-						passed := !t.Failed()
+						passed := !t.Failed() && r == nil
 						stats.end(method.Name, passed)
 					}
 
@@ -152,6 +158,7 @@ func Run(t *testing.T, suite TestingSuite) {
 					}
 
 					suite.SetT(parentT)
+					failOnPanic(t, r)
 				}()
 
 				if setupTestSuite, ok := suite.(SetupTestSuite); ok {


Test patch:
diff --git a/suite/suite_test.go b/suite/suite_test.go
index 963a252..446029a 100644
--- a/suite/suite_test.go
+++ b/suite/suite_test.go
@@ -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