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?

[maroux]

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

Nice, yeah I think that works.

[maroux]

Nice, yeah I think that works.

Never mind, spoke too soon, it leads to a deadlock because parallel sub-tests wait for parent test to complete, and parent test won't complete if its waiting for child tests to be done. I'll dig further, however: if parallel tests can't be run, this becomes a moot point. I'd rather document that Suite sub-tests shouldn't call s.T().Parallel() and leave it at that.

I'm a bit stumped, can you raise a new issue for this?

Yep, same, there has been some discussion in #187 on this very topic which covers what we talked about here. I suspect this will require a breaking change in the API. I'll do some more digging on this as well.

[brackendawson]

I'd rather document that Suite sub-tests shouldn't call s.T().Parallel() and leave it at that.

That might be best, what a gnarly issue.

I suspect this will require a breaking change in the API.

I think you're right, perhaps we can come up with a workable design that supports parallel testing for v2.0? Though I must admit I never actually use suite.

[maroux]

Fwiw, here's a couple of possible fixes for the tearDown ordering:

1. Group subtests under a single group within the test suite: https://github.com/stretchr/testify/compare/master...maroux:group_tests (go 1.6+)
2. Use T.Cleanup: https://github.com/stretchr/testify/compare/master...maroux:parallel_tests_cleanup (go 1.14+)

Although, as mentioned, I think we should just close this ticket and document that parallel testing is not supported for now.

[maroux]

workable design that supports parallel testing for v2.0

I imagine the easiest way is to allow test sub-methods to have a signature like so:

(_ *FooSuite) test_foo(s *suite.SuiteHelper) {
     s.Equal(...)
}

and move T, Assertions and Require out of Suite into a separate struct (named SuiteHelper) above.

[maroux]

Alright, https://github.com/stretchr/testify/compare/master...maroux:suite_copy also shows a possible implementation for concurrency-safe suite.

[Cirilus]

Any solution? It still doesn't work properly.

[Antonboom]

Covered by testifylint#suite-broken-parallel 👌