Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

testing: reject calls to Run within Cleanup callbacks #48515

Closed
bcmills opened this issue Sep 21, 2021 · 7 comments
Closed

testing: reject calls to Run within Cleanup callbacks #48515

bcmills opened this issue Sep 21, 2021 · 7 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 21, 2021

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
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 21, 2021
@bcmills bcmills added this to the Backlog milestone Sep 21, 2021
@rsc
Copy link
Contributor

rsc commented Sep 21, 2021

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

@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Sep 21, 2021
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 21, 2021
@bcmills bcmills changed the title testing: Cleanup runs in wrong order when a Cleanup function spawns a subtest testing: reject calls to Run within Cleanup callbacks Sep 21, 2021
@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Nov 16, 2022
@gopherbot
testing: in TestRacyOutput, ensure that the Run calls complete before…
8c17505 
… cleanup

Also tweak the failure message added in CL 352349.

Updates #18741.
Updates #48515.

Change-Id: I46ed84c6f498d7a68414cc3dab3c1cd55da69aa9
Reviewed-on: https://go-review.googlesource.com/c/go/+/451215
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Changkun Ou <mail@changkun.de>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@nelsam
Copy link

nelsam commented Mar 9, 2023

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
Copy link
Contributor

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

@gopherbot
Copy link

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

@nelsam
Copy link

nelsam commented Mar 9, 2023 •
edited

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.

gopherbot pushed a commit to golang/website that referenced this issue Mar 10, 2023
@ianlancetaylor @gopherbot
doc/go1.20: mention that calling T.Run from T.Cleanup will panic
7c7b745 
For golang/go#48515

Change-Id: I1c616144a58e92fe4022d0e86f4208d68dcce816
Reviewed-on: https://go-review.googlesource.com/c/website/+/475215
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Rob Pike <r@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
@nelsam nelsam mentioned this issue Mar 16, 2023
Merged
@golang golang locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@rsc @nelsam @ianlancetaylor @bcmills @gopherbot