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>

diff --git a/src/testing/sub_test.go b/src/testing/sub_test.go
index 55b14c3795ff35355f3f2b481393da60c13c53c2..1c23d054a06a325ae35f93347778e65ffb2b5c9d 100644 (file)
--- 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 166ebb7ab33537052c4ba06a4c6819d4c08136d2..d3822dfd578f803bf45cb196429f0ff997ce6016 100644 (file)
--- 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!")
+               })
+       })
+}