TearDownTest is being executed while parallel sub tests are still running

[swithek]

Consider the following example:

type Suite struct {
	suite.Suite
}

func (s *Suite) TearDownSuite() {
	s.T().Log(">>>>>> suite tear down")
}

func (s *Suite) TearDownTest() {
	s.T().Log(">>>>>> single test tear down")
}

func (s *Suite) TestOne() {
	for _, v := range []string{"sub1", "sub2", "sub3"} {
		s.Run(v, func() {
			s.T().Parallel()
		})
	}
}

func (s *Suite) TestTwo() {
	for _, v := range []string{"sub1", "sub2", "sub3"} {
		s.Run(v, func() {
			s.T().Parallel()
		})
	}
}

func TestLogic(t *testing.T) {
	suite.Run(t, &Suite{})
}

After executing that you would get an output similar to this:

=== RUN   TestLogic
=== RUN   TestLogic/TestOne
=== RUN   TestLogic/TestOne/sub1
=== PAUSE TestLogic/TestOne/sub1
=== RUN   TestLogic/TestOne/sub2
=== PAUSE TestLogic/TestOne/sub2
=== RUN   TestLogic/TestOne/sub3
=== PAUSE TestLogic/TestOne/sub3
    TestLogic/TestOne: prog.go:18: >>>>>> single test tear down
=== CONT  TestLogic/TestOne/sub1
=== CONT  TestLogic/TestOne/sub3
=== CONT  TestLogic/TestOne/sub2 >>> test finished here
=== RUN   TestLogic/TestTwo
=== RUN   TestLogic/TestTwo/sub1
=== PAUSE TestLogic/TestTwo/sub1
=== RUN   TestLogic/TestTwo/sub2
=== PAUSE TestLogic/TestTwo/sub2
=== RUN   TestLogic/TestTwo/sub3
=== PAUSE TestLogic/TestTwo/sub3
    TestLogic/TestTwo: prog.go:18: >>>>>> single test tear down
=== CONT  TestLogic/TestTwo/sub1
=== CONT  TestLogic/TestTwo/sub3
=== CONT  TestLogic/TestTwo/sub2 >>> test finished here
    TestLogic: prog.go:14: >>>>>> suite tear down

Live example: https://play.golang.com/p/fPY3DZFNNok

It is clear that TearDownTest is being executed before all parallel sub tests finish. I see that a similar issue was addressed in #799 and #466 but not resolved in its entirety.

The version in use is 1.5.1

[boyan-soubachov]

Do you mind running a test and confirming whether this happens with version 1.4.0?

[swithek]

I've just tried running this test suite on 1.4.0 and the result was the same.

[boyan-soubachov]

Thank you, I will have a look.

[v3n]

This occurs for me with 1.6.1 as well.

[maroux]

It seems like this could be solved by just using t.Cleanup for both tearDownSuite and tearDownTest. Is there an obvious reason for not using cleanup? If not, I can put up a PR for this change.

[brackendawson]

t.Cleanup is a Go 1.14 feature, this would need us to update the go.mod to 1.14 and drop support of go 1.13. Not necessarily a bad thing, but needs to be done with intent. Is there an approach that works in 1.13?

[maroux]

t.Cleanup is a Go 1.14 feature, this would need us to update the go.mod to 1.14 and drop support of go 1.13. Not necessarily a bad thing, but needs to be done with intent. Is there an approach that works in 1.13?

oh I see, well, worst case, we can do a conditional fix for 1.14+ but let me dig into 1.13 code.

[maroux]

@brackendawson looks like the only option for <=1.13 is to use t.Run to create a new group within a suite and call all the tests within that group. This works because t.Run will not return until all sub-tests have finished which gives us a clean way to run teardown. The only downside I can think of is one additional level of nesting in tests, which mostly just affects output? tbf, I think all values within a suite must be run in a group anyway, since that aligns with stdlib's expectations, but perhaps there's a reason to not do this?

[maroux]

@brackendawson although, Suite.SetT also got me thinking - is Suite even concurrent-safe? Because it is being run from multiple routines here, and if there are multiple parallel tests within a suite, they will overwrite each other's T object.

[brackendawson]

So on the original bug, I'm thinking we could:

1. Stop using testing.InternalTest here and instead define that type privately as suite.internalTest.
2. Give suite.internalTest a new field done of type chan struct{}.
3. Defer a close of that channel at the top the function we use in suite.internalTest.F (F will need to close over the channel where it is defined outside of the struct literal).
4. After here range over the tests again and this time just read one value from their done channels.

That should stop us from calling teardowns early, do you agree?

The Suite.SetT() call inside F you point out is definitely not safe if the user calls s.T().Parallel() in their test. Fortunately Suite.t is unexported and guarded behind Suite.T(). Unfortunately all the tests are the same anonymous function so we genuinely have no way to tell which test called suite.T(). We also can't copy the struct users embed suite.Suite in because they can depend on any other fields they care to set. Then other functions like Suite.Require and Suite.Assert are also unsafe for concurrent calls. I'm a bit stumped, can you raise a new issue for this?