From 916ce7989ab322c7e3f122b5f6046d0e992f8929 Mon Sep 17 00:00:00 2001 From: Linus Barth Date: Mon, 16 Oct 2023 02:59:15 +0200 Subject: [PATCH 1/8] fix: SetupSubTest and TearDownSubTest execution order There were two problems with the order of execution in the Suite.Run() method: - One could not access the correct testing context ("s.T()") inside the SetupSubTest and TearDownSubTest methods. If the testing context was used for e.g. assertions of mocks in the TearDownSubTest, the results would not be "attached" to the correct test in the test output. - The behavior was different to the order of execution for "root" tests (see lines 167-201) regarding the SetupTest and TearDownTest methods. This could confuse users of the library. Also the logic to be deferred was joined together. This was fine beforehand because a panic in the TearDownSubTest would have had no influence on the "suite.SetT(oldT)". Now since a panic in the TearDownSubTest would lead to omitting the "suite.SetT(oldT)" this defer was split into two separate defers. --- suite/suite.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/suite/suite.go b/suite/suite.go index 8b4202d89..54efd4c9a 100644 --- a/suite/suite.go +++ b/suite/suite.go @@ -96,19 +96,23 @@ func failOnPanic(t *testing.T, r interface{}) { func (suite *Suite) Run(name string, subtest func()) bool { oldT := suite.T() - if setupSubTest, ok := suite.s.(SetupSubTest); ok { - setupSubTest.SetupSubTest() - } + return oldT.Run(name, func(t *testing.T) { + suite.SetT(t) + + defer func() { + suite.SetT(oldT) + }() - defer func() { - suite.SetT(oldT) - if tearDownSubTest, ok := suite.s.(TearDownSubTest); ok { - tearDownSubTest.TearDownSubTest() + if setupSubTest, ok := suite.s.(SetupSubTest); ok { + setupSubTest.SetupSubTest() } - }() - return oldT.Run(name, func(t *testing.T) { - suite.SetT(t) + defer func() { + if tearDownSubTest, ok := suite.s.(TearDownSubTest); ok { + tearDownSubTest.TearDownSubTest() + } + }() + subtest() }) } From 83e44bc83fc36796e430d941f7e6706c7d220e05 Mon Sep 17 00:00:00 2001 From: Linus Barth Date: Mon, 16 Oct 2023 02:59:16 +0200 Subject: [PATCH 2/8] improve: defer-style in Suite.Run() --- suite/suite.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/suite/suite.go b/suite/suite.go index 54efd4c9a..cca7533a7 100644 --- a/suite/suite.go +++ b/suite/suite.go @@ -99,19 +99,15 @@ func (suite *Suite) Run(name string, subtest func()) bool { return oldT.Run(name, func(t *testing.T) { suite.SetT(t) - defer func() { - suite.SetT(oldT) - }() + defer suite.SetT(oldT) if setupSubTest, ok := suite.s.(SetupSubTest); ok { setupSubTest.SetupSubTest() } - defer func() { - if tearDownSubTest, ok := suite.s.(TearDownSubTest); ok { - tearDownSubTest.TearDownSubTest() - } - }() + if tearDownSubTest, ok := suite.s.(TearDownSubTest); ok { + defer tearDownSubTest.TearDownSubTest() + } subtest() }) From 4657972ba325dd53c4c4d47a547a14dc6b32aa30 Mon Sep 17 00:00:00 2001 From: Linus Barth Date: Mon, 16 Oct 2023 02:59:16 +0200 Subject: [PATCH 3/8] test: call order of setup/teardown for subtests --- suite/suite_test.go | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/suite/suite_test.go b/suite/suite_test.go index d684f52b9..788d3648b 100644 --- a/suite/suite_test.go +++ b/suite/suite_test.go @@ -481,7 +481,7 @@ func (s *CallOrderSuite) SetupSuite() { func (s *CallOrderSuite) TearDownSuite() { s.call("TearDownSuite") - assert.Equal(s.T(), "SetupSuite;SetupTest;Test A;TearDownTest;SetupTest;Test B;TearDownTest;TearDownSuite", strings.Join(s.callOrder, ";")) + assert.Equal(s.T(), "SetupSuite;SetupTest;Test A;SetupSubTest;SubTest A1;TearDownSubTest;SetupSubTest;SubTest A2;TearDownSubTest;TearDownTest;SetupTest;Test B;SetupSubTest;SubTest B1;TearDownSubTest;SetupSubTest;SubTest B2;TearDownSubTest;TearDownTest;TearDownSuite", strings.Join(s.callOrder, ";")) } func (s *CallOrderSuite) SetupTest() { s.call("SetupTest") @@ -491,12 +491,32 @@ func (s *CallOrderSuite) TearDownTest() { s.call("TearDownTest") } +func (s *CallOrderSuite) SetupSubTest() { + s.call("SetupSubTest") +} + +func (s *CallOrderSuite) TearDownSubTest() { + s.call("TearDownSubTest") +} + func (s *CallOrderSuite) Test_A() { s.call("Test A") + s.Run("SubTest A1", func() { + s.call("SubTest A1") + }) + s.Run("SubTest A2", func() { + s.call("SubTest A2") + }) } func (s *CallOrderSuite) Test_B() { s.call("Test B") + s.Run("SubTest B1", func() { + s.call("SubTest B1") + }) + s.Run("SubTest B2", func() { + s.call("SubTest B2") + }) } type suiteWithStats struct { From aa01ed0935477cacd5943a7c0165a9f9e76ac2ea Mon Sep 17 00:00:00 2001 From: Linus Barth Date: Mon, 16 Oct 2023 02:59:16 +0200 Subject: [PATCH 4/8] test: testing.T correctness in subtests setup/teardown --- suite/suite_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/suite/suite_test.go b/suite/suite_test.go index 788d3648b..68263e01c 100644 --- a/suite/suite_test.go +++ b/suite/suite_test.go @@ -242,8 +242,8 @@ func (suite *SuiteTester) TestSubtest() { for _, t := range []struct { testName string }{ - {"first"}, - {"second"}, + {"first-subtest"}, + {"second-subtest"}, } { suiteT := suite.T() suite.Run(t.testName, func() { @@ -259,10 +259,14 @@ func (suite *SuiteTester) TestSubtest() { func (suite *SuiteTester) TearDownSubTest() { suite.TearDownSubTestRunCount++ + // We should get the *testing.T for the test that is to be torn down + suite.Contains(suite.T().Name(), "subtest") } func (suite *SuiteTester) SetupSubTest() { suite.SetupSubTestRunCount++ + // We should get the *testing.T for the test that is to be set up + suite.Contains(suite.T().Name(), "subtest") } type SuiteSkipTester struct { From ac5cd69d4baa639ccd1a95c52d627827e4ed242d Mon Sep 17 00:00:00 2001 From: Linus Barth Date: Mon, 16 Oct 2023 02:59:16 +0200 Subject: [PATCH 5/8] improve: move comment to msgAndArgs-param in test --- suite/suite_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/suite/suite_test.go b/suite/suite_test.go index 68263e01c..58c775fa2 100644 --- a/suite/suite_test.go +++ b/suite/suite_test.go @@ -259,14 +259,12 @@ func (suite *SuiteTester) TestSubtest() { func (suite *SuiteTester) TearDownSubTest() { suite.TearDownSubTestRunCount++ - // We should get the *testing.T for the test that is to be torn down - suite.Contains(suite.T().Name(), "subtest") + suite.Contains(suite.T().Name(), "subtest", "We should get the *testing.T for the test that is to be torn down") } func (suite *SuiteTester) SetupSubTest() { suite.SetupSubTestRunCount++ - // We should get the *testing.T for the test that is to be set up - suite.Contains(suite.T().Name(), "subtest") + suite.Contains(suite.T().Name(), "subtest", "We should get the *testing.T for the test that is to be set up") } type SuiteSkipTester struct { From c8efd3cb7744ceb3bf4e8f0b5dc2c1682923afe1 Mon Sep 17 00:00:00 2001 From: Linus Barth Date: Tue, 17 Oct 2023 16:36:33 +0200 Subject: [PATCH 6/8] improve: tests for asserting test names in subtests --- suite/suite_test.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/suite/suite_test.go b/suite/suite_test.go index 58c775fa2..05bca03e6 100644 --- a/suite/suite_test.go +++ b/suite/suite_test.go @@ -162,6 +162,9 @@ type SuiteTester struct { SetupSubTestRunCount int TearDownSubTestRunCount int + SetupSubTestNames []string + TearDownSubTestNames []string + SuiteNameBefore []string TestNameBefore []string @@ -242,8 +245,8 @@ func (suite *SuiteTester) TestSubtest() { for _, t := range []struct { testName string }{ - {"first-subtest"}, - {"second-subtest"}, + {"first"}, + {"second"}, } { suiteT := suite.T() suite.Run(t.testName, func() { @@ -258,13 +261,13 @@ func (suite *SuiteTester) TestSubtest() { } func (suite *SuiteTester) TearDownSubTest() { + suite.TearDownSubTestNames = append(suite.TearDownSubTestNames, suite.T().Name()) suite.TearDownSubTestRunCount++ - suite.Contains(suite.T().Name(), "subtest", "We should get the *testing.T for the test that is to be torn down") } func (suite *SuiteTester) SetupSubTest() { + suite.SetupSubTestNames = append(suite.SetupSubTestNames, suite.T().Name()) suite.SetupSubTestRunCount++ - suite.Contains(suite.T().Name(), "subtest", "We should get the *testing.T for the test that is to be set up") } type SuiteSkipTester struct { @@ -321,6 +324,12 @@ func TestRunSuite(t *testing.T) { assert.Contains(t, suiteTester.TestNameBefore, "TestSkip") assert.Contains(t, suiteTester.TestNameBefore, "TestSubtest") + assert.Contains(t, suiteTester.SetupSubTestNames, "TestRunSuite/TestSubtest/first") + assert.Contains(t, suiteTester.SetupSubTestNames, "TestRunSuite/TestSubtest/second") + + assert.Contains(t, suiteTester.TearDownSubTestNames, "TestRunSuite/TestSubtest/first") + assert.Contains(t, suiteTester.TearDownSubTestNames, "TestRunSuite/TestSubtest/second") + for _, suiteName := range suiteTester.SuiteNameAfter { assert.Equal(t, "SuiteTester", suiteName) } From 91478fede62af243db8c4582a77029261fd209a2 Mon Sep 17 00:00:00 2001 From: Linus Barth Date: Wed, 18 Oct 2023 03:22:36 +0200 Subject: [PATCH 7/8] fix: panic behavior for subtests This fix adds panic handling for subtests which will achieve: - subtests will fail for the correct test context when panicking - the test execution is not stopped; the next subtest will be executed --- suite/suite.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/suite/suite.go b/suite/suite.go index cca7533a7..f3aa9a16b 100644 --- a/suite/suite.go +++ b/suite/suite.go @@ -98,9 +98,10 @@ func (suite *Suite) Run(name string, subtest func()) bool { return oldT.Run(name, func(t *testing.T) { suite.SetT(t) - defer suite.SetT(oldT) + defer recoverAndFailOnPanic(t) + if setupSubTest, ok := suite.s.(SetupSubTest); ok { setupSubTest.SetupSubTest() } From c18759c67ad03b3bec71af0249356760c2e6e589 Mon Sep 17 00:00:00 2001 From: Linus Barth Date: Wed, 18 Oct 2023 16:04:50 +0200 Subject: [PATCH 8/8] test: add suite tests for panicking of subtests --- suite/suite_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/suite/suite_test.go b/suite/suite_test.go index 05bca03e6..fdeaee08e 100644 --- a/suite/suite_test.go +++ b/suite/suite_test.go @@ -648,3 +648,45 @@ func (s *FailfastSuite) Test_B_Passes() { s.call("Test B Passes") s.Require().True(true) } + +type subtestPanicSuite struct { + Suite + inTearDownSuite bool + inTearDownTest bool + inTearDownSubTest bool +} + +func (s *subtestPanicSuite) TearDownSuite() { + s.inTearDownSuite = true +} + +func (s *subtestPanicSuite) TearDownTest() { + s.inTearDownTest = true +} + +func (s *subtestPanicSuite) TearDownSubTest() { + s.inTearDownSubTest = true +} + +func (s *subtestPanicSuite) TestSubtestPanic() { + s.Run("subtest", func() { + panic("panic") + }) +} + +func TestSubtestPanic(t *testing.T) { + suite := new(subtestPanicSuite) + ok := testing.RunTests( + allTestsFilter, + []testing.InternalTest{{ + Name: "TestSubtestPanic", + F: func(t *testing.T) { + Run(t, suite) + }, + }}, + ) + assert.False(t, ok) + assert.True(t, suite.inTearDownSubTest) + assert.True(t, suite.inTearDownTest) + assert.True(t, suite.inTearDownSuite) +}