testing: add regression tests for reentrant calls to T.Run

These tests represent two patterns of usage, found in Google-internal
tests, that deadlocked after CL 506755.

TestConcurrentRun is a minor variation on TestParallelSub, with the
additional expectation that the concurrent calls to Run (without
explicit calls to Parallel) proceed without blocking. It replaces
TestParallelSub.

TestParentRun is similar, but instead of calling Run concurrently it
calls Run from within the subtest body. It almost certainly represents
an accidental misuse of T.Run, but since that pattern used to run to
completion we don't want to break it accidentally. (Perhaps it should
be diagnosed with a vet check instead?)

While we are testing concurrency, this also cleans up
TestConcurrentCleanup to use a clearer synchronization pattern.

Fixes #64402.

Change-Id: I14fc7e7085a994c284509eac28190c3a8feb04cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/546019
Auto-Submit: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

2 files changed, 40 insertions, 20 deletions

diff --git a/src/testing/sub_test.go b/src/testing/sub_test.go
index 55b14c3795..1c23d054a0 100644
--- a/src/testing/sub_test.go
+++ b/src/testing/sub_test.go
@@ -767,22 +767,6 @@ func TestBenchmarkReadMemStatsBeforeFirstRun(t *T) {
 	})
 }
 
-func TestParallelSub(t *T) {
-	c := make(chan int)
-	block := make(chan int)
-	for i := 0; i < 10; i++ {
-		go func(i int) {
-			<-block
-			t.Run(fmt.Sprint(i), func(t *T) {})
-			c <- 1
-		}(i)
-	}
-	close(block)
-	for i := 0; i < 10; i++ {
-		<-c
-	}
-}
-
 type funcWriter struct {
 	write func([]byte) (int, error)
 }
@@ -910,18 +894,22 @@ func TestCleanup(t *T) {
 func TestConcurrentCleanup(t *T) {
 	cleanups := 0
 	t.Run("test", func(t *T) {
-		done := make(chan struct{})
+		var wg sync.WaitGroup
+		wg.Add(2)
 		for i := 0; i < 2; i++ {
 			i := i
 			go func() {
 				t.Cleanup(func() {
+					// Although the calls to Cleanup are concurrent, the functions passed
+					// to Cleanup should be called sequentially, in some nondeterministic
+					// order based on when the Cleanup calls happened to be scheduled.
+					// So these assignments to the cleanups variable should not race.
 					cleanups |= 1 << i
 				})
-				done <- struct{}{}
+				wg.Done()
 			}()
 		}
-		<-done
-		<-done
+		wg.Wait()
 	})
 	if cleanups != 1|2 {
 		t.Errorf("unexpected cleanup; got %d want 3", cleanups)
diff --git a/src/testing/testing_test.go b/src/testing/testing_test.go
index 166ebb7ab3..d3822dfd57 100644
--- a/src/testing/testing_test.go
+++ b/src/testing/testing_test.go
@@ -780,3 +780,35 @@ func parseRunningTests(out []byte) (runningTests []string, ok bool) {
 
 	return nil, false
 }
+
+func TestConcurrentRun(t *testing.T) {
+	// Regression test for https://go.dev/issue/64402:
+	// this deadlocked after https://go.dev/cl/506755.
+
+	block := make(chan struct{})
+	var ready, done sync.WaitGroup
+	for i := 0; i < 2; i++ {
+		ready.Add(1)
+		done.Add(1)
+		go t.Run("", func(*testing.T) {
+			ready.Done()
+			<-block
+			done.Done()
+		})
+	}
+	ready.Wait()
+	close(block)
+	done.Wait()
+}
+
+func TestParentRun(t1 *testing.T) {
+	// Regression test for https://go.dev/issue/64402:
+	// this deadlocked after https://go.dev/cl/506755.
+
+	t1.Run("outer", func(t2 *testing.T) {
+		t2.Log("Hello outer!")
+		t1.Run("not_inner", func(t3 *testing.T) { // Note: this is t1.Run, not t2.Run.
+			t3.Log("Hello inner!")
+		})
+	})
+}