testing: reject calls to Run within Cleanup callbacks

[bcmills]

What version of Go are you using (go version)?

go1.17.1

Does this issue reproduce with the latest release?

Yes.

What did you do?

https://play.golang.org/p/l8yW_gkioCx:

func TestCleanupOrder(t *testing.T) {
	t.Cleanup(func() {
		t.Logf("first cleanup")
	})

	t.Cleanup(func() {
		t.Logf("second cleanup")

		t.Run("in cleanup", func(t *testing.T) {
			t.Parallel()
			t.Logf("running parallel subtest")
		})
	})
}

What did you expect to see?

Per https://pkg.go.dev/testing#T.Cleanup:

Cleanup registers a function to be called when the test (or subtest) and all its subtests complete. Cleanup functions will be called in last added, first called order.

So a Cleanup callback should not be started while there are subtests of its parent test still outstanding. Thus, either:

• the call to t.Run within a Cleanup callback panics the test, or
• the first-added Cleanup callback is executed after all of the subtests spawned by the second-added Cleanup callback have completed.

That would give an output like:

=== RUN   TestCleanupOrder
    prog.go:11: second cleanup
=== RUN   TestCleanupOrder/in_cleanup
    prog.go:15: running parallel subtest
=== CONT  TestCleanupOrder
    prog.go:7: first cleanup
--- PASS: TestCleanupOrder (0.00s)
    --- PASS: TestCleanupOrder/in_cleanup (0.00s)
PASS

What did you see instead?

The first Cleanup callback is executed before the parallel subtests spawned by the second Cleanup callback are run.

=== RUN   TestCleanupOrder
    prog.go:11: second cleanup
=== RUN   TestCleanupOrder/in_cleanup
=== PAUSE TestCleanupOrder/in_cleanup
=== CONT  TestCleanupOrder
    prog.go:7: first cleanup
=== CONT  TestCleanupOrder/in_cleanup
    prog.go:15: running parallel subtest
--- PASS: TestCleanupOrder (0.00s)
    --- PASS: TestCleanupOrder/in_cleanup (0.00s)
PASS

CC @ianlancetaylor @changkun @rogpeppe @jayconrod

[rsc]

Don't think too hard about this. Make t.Run panic during t.Cleanup callbacks.

[gopherbot]

Change https://golang.org/cl/352349 mentions this issue: testing: reject calls to Run within Cleanup callbacks

[gopherbot]

Change https://go.dev/cl/451215 mentions this issue: testing: in TestRacyOutput, ensure that the Run calls complete before cleanup

[nelsam]

FWIW, this broke a BDD-style structured testing library I maintain. It caught me off-guard because it wasn't mentioned in the go 1.20 release notes. As soon as I updated to 1.20 all of my tests broke and I've had to fall back to 1.19.x.

In the library, for the moment I've had to add:

//go:build go1.20

package onpar

func init() {
    panic("onpar v2 is broken in go v1.20.")
}

[ianlancetaylor]

@nelsam Thanks. I sent https://go.dev/cl/475215 to mention this in the release notes.

[gopherbot]

Change https://go.dev/cl/475215 mentions this issue: doc/go1.20: mention that calling T.Run from T.Cleanup will panic

[nelsam]

Thanks! The docs weren't very clear if it was supported, so I tried it and it worked. And it felt a lot cleaner than our old defer o.Run(t) (which wasn't hard to forget). We've been relying on t.Cleanup since go 1.18.

I still don't know how I'm going to fix it, but I'm at least glad that the breaking change is in the release notes now.