stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
23.15k stars 1.59k forks source link

suite.TestTeardown fail/panic is shadowing the cause panic #1494

Closed to6ka closed 11 months ago

to6ka commented 11 months ago

If someone has problems with test setup, causing panic he highly likely will get problems with teardown, causing fail or panic too and current implementation is shadowing the first panic

import (
    "errors"
    "log"
    "testing"

    "github.com/stretchr/testify/suite"
)

func TestSuite(t *testing.T) {
    suite.Run(t, &testSuite{})
}

type testSuite struct {
    suite.Suite
}

func (s *testSuite) SetupTest() {
    panic("reason panic")
}

func (s *testSuite) TearDownTest() {
    err := errors.New("test teardown error")
    s.Require().NoError(err)
    // or use panic
    // panic("secondary panic")
}

func (s *testSuite) Test_Run() {
    log.Print("do something")
}
go test -v ./test/test_test/... 2>&1 | tee test-one.log
=== RUN   TestSuite
=== RUN   TestSuite/Test_Run
    test_test.go:25: 
                Error Trace:    /home/user/git/project/test/test_test/test_test.go:25
                                                        /home/user/go-path/pkg/mod/github.com/stretchr/testify@v1.8.3/suite/suite.go:179
                                                        /home/user/go-root/src/runtime/panic.go:914
                                                        /home/user/git/project/test/test_test/test_test.go:20
                                                        /home/user/go-path/pkg/mod/github.com/stretchr/testify@v1.8.3/suite/suite.go:187
                Error:          Received unexpected error:
                                test teardown error
                Test:           TestSuite/Test_Run
--- FAIL: TestSuite (0.00s)
    --- FAIL: TestSuite/Test_Run (0.00s)
FAIL
FAIL    github.com/to6ka/prj/test/test_test      0.006s
FAIL
go test -v ./test/test_test/... 2>&1 | tee test-one.log
=== RUN   TestSuite
=== RUN   TestSuite/Test_Run
    suite.go:87: test panicked: secondary panic
        goroutine 7 [running]:
        runtime/debug.Stack()
                /home/user/go-root/src/runtime/debug/stack.go:24 +0x5e
        github.com/stretchr/testify/suite.failOnPanic(0xc0000f4ea0, {0x75daa0, 0x858870})
                /home/user/go-path/pkg/mod/github.com/stretchr/testify@v1.8.3/suite/suite.go:87 +0x39
        github.com/stretchr/testify/suite.recoverAndFailOnPanic(0xc000077af0?)
                /home/user/go-path/pkg/mod/github.com/stretchr/testify@v1.8.3/suite/suite.go:82 +0x35
        panic({0x75daa0?, 0x858870?})
                /home/user/go-root/src/runtime/panic.go:914 +0x21f
        github.com/to6ka/prj/test/test_test_test.(*testSuite).TearDownTest(0x77be60?)
                /home/user/git/project/test/test_test/test_test.go:26 +0x25
        github.com/stretchr/testify/suite.Run.func1.1()
                /home/user/go-path/pkg/mod/github.com/stretchr/testify@v1.8.3/suite/suite.go:179 +0x21d
        panic({0x75daa0?, 0x858860?})
                /home/user/go-root/src/runtime/panic.go:914 +0x21f
        github.com/to6ka/prj/test/test_test_test.(*testSuite).SetupTest(0xa00280?)
                /home/user/git/project/test/test_test/test_test.go:19 +0x25
        github.com/stretchr/testify/suite.Run.func1(0xc0000f4ea0)
                /home/user/go-path/pkg/mod/github.com/stretchr/testify@v1.8.3/suite/suite.go:187 +0x1ee
        testing.tRunner(0xc0000f4ea0, 0xc000192090)
                /home/user/go-root/src/testing/testing.go:1595 +0xff
        created by testing.(*T).Run in goroutine 6
                /home/user/go-root/src/testing/testing.go:1648 +0x3ad
--- FAIL: TestSuite (0.00s)
    --- FAIL: TestSuite/Test_Run (0.00s)
FAIL
FAIL    github.com/to6ka/prj/test/test_test      0.006s
FAIL

Also, t.Failed() returning false in this case

I am not sure, but i think the simplest way here may be to report both panics/fails separately

dolmen commented 11 months ago

Please review #1474 that I recently merged.

to6ka commented 11 months ago

Please review #1474 that I recently merged.

Hi, thanks for your reply.
Did you mean https://github.com/stretchr/testify/pull/1471 ?
I checked it with last commit in master, and unfortunately this pr did not fix the issue :(