suite: fix SetupSubTest and TearDownSubTest execution order

[linusbarth]

Summary

The order of execution of SetupSubTest and TearDownSubTest methods was changed so that the testing context inside these methods is correct.

Changes

• The SetupSubTest was moved to after the SetT that set the child testing context
• The TearDownSubTest was moved to before the deferred SetT that reset the testing context (execution wise)
• The TearDownSubTest and SetT were deferred in one block. 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.

Motivation

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 regarding the SetupTest and TearDownTest methods (see lines 167-201). This could confuse users of the library.

Related issues

Closes #1465

[dolmen]

Move the if outside of the defer:

		if tearDownSubTest, ok := suite.s.(TearDownSubTest); ok {
			defer tearDownSubTest.TearDownSubTest()
		}

[linusbarth]

Done. Also I took the liberty to extract the defer-function in line 102. Please note if I should revert that. I think it should not produce noticeable change.

[dolmen]

We also need test that enforces the order. That test should fail before your fix and succeed after.

[linusbarth]

We also need test that enforces the order. That test should fail before your fix and succeed after.

done

[dolmen]

@linusbarth Please drop the last commit (merge commit). Use git rebase instead.

[dolmen]

Move this comment as 3rd argument of Contains below.

[linusbarth]

done

[dolmen]

Move this comment as 3rd argument of Contains below.

[linusbarth]

done

[tomhutch]

Nice, I'd be really happy to see this fix get in.

[tomhutch]

Updating the test suite attributes here and performing assertions alongside the others in the main test body will be more consistent with the current testing setup:

testify/suite/suite_test.go

Lines 154 to 172 in ac5cd69

	SetupSuiteRunCount int
	TearDownSuiteRunCount int
	SetupTestRunCount int
	TearDownTestRunCount int
	TestOneRunCount int
	TestTwoRunCount int
	TestSubtestRunCount int
	NonTestMethodRunCount int
	SetupSubTestRunCount int
	TearDownSubTestRunCount int
	SuiteNameBefore []string
	TestNameBefore []string
	SuiteNameAfter []string
	TestNameAfter []string
	TimeBefore []time.Time
	TimeAfter []time.Time

See this for reference:
https://github.com/stretchr/testify/pull/1393/files#diff-ca4ffdb7050ad570420ac5682fefad8481384709c2884051cd57071283ce9ca4

[linusbarth]

Thanks for your review! You're right, that is more consistent. I mostly just copied your referenced testing code now into my branch.