testing: fail if T.Setenv is called via T.Run in a parallel test

The existing implementation can call to T.Setenv in T.Run even after
calling to T.Parallel, so I changed it to cause a panic in that case.

Fixes #55128

Change-Id: Ib89d998ff56f00f96a5ca218af071bd35fdae53a
Reviewed-on: https://go-review.googlesource.com/c/go/+/431101
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>

diff --git a/src/testing/testing.go b/src/testing/testing.go
index 7e86faf9508cee2c310b31fce6503022c26d305f..b64286c00596acbb378aa224e8069f8e48afca78 100644 (file)
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -547,6 +547,7 @@ type common struct {
        hasSub     atomic.Bool    // whether there are sub-benchmarks.
        raceErrors int            // Number of races detected during test.
        runner     string         // Function name of tRunner running the test.
+       isParallel bool           // Whether the test is parallel.
 
        parent   *common
        level    int       // Nesting depth of test or benchmark.
@@ -823,9 +824,8 @@ var _ TB = (*B)(nil)
 // may be called simultaneously from multiple goroutines.
 type T struct {
        common
-       isParallel bool
-       isEnvSet   bool
-       context    *testContext // For running tests and subtests.
+       isEnvSet bool
+       context  *testContext // For running tests and subtests.
 }
 
 func (c *common) private() {}
@@ -1326,7 +1326,19 @@ func (t *T) Parallel() {
 //
 // This cannot be used in parallel tests.
 func (t *T) Setenv(key, value string) {
-       if t.isParallel {
+       // Non-parallel subtests that have parallel ancestors may still
+       // run in parallel with other tests: they are only non-parallel
+       // with respect to the other subtests of the same parent.
+       // Since SetEnv affects the whole process, we need to disallow it
+       // if the current test or any parent is parallel.
+       isParallel := false
+       for c := &t.common; c != nil; c = c.parent {
+               if c.isParallel {
+                       isParallel = true
+                       break
+               }
+       }
+       if isParallel {
                panic("testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests")
        }
 

diff --git a/src/testing/testing_test.go b/src/testing/testing_test.go
index 08ae23991f18f6484e0cd10162ec879c5ca9bc90..3616f04d5fcc425f83bedef008ff08a381b8b55e 100644 (file)
--- a/src/testing/testing_test.go
+++ b/src/testing/testing_test.go
@@ -200,3 +200,35 @@ func TestSetenvWithParallelBeforeSetenv(t *testing.T) {
 
        t.Setenv("GO_TEST_KEY_1", "value")
 }
+
+func TestSetenvWithParallelParentBeforeSetenv(t *testing.T) {
+       t.Parallel()
+
+       t.Run("child", func(t *testing.T) {
+               defer func() {
+                       want := "testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests"
+                       if got := recover(); got != want {
+                               t.Fatalf("expected panic; got %#v want %q", got, want)
+                       }
+               }()
+
+               t.Setenv("GO_TEST_KEY_1", "value")
+       })
+}
+
+func TestSetenvWithParallelGrandParentBeforeSetenv(t *testing.T) {
+       t.Parallel()
+
+       t.Run("child", func(t *testing.T) {
+               t.Run("grand-child", func(t *testing.T) {
+                       defer func() {
+                               want := "testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests"
+                               if got := recover(); got != want {
+                                       t.Fatalf("expected panic; got %#v want %q", got, want)
+                               }
+                       }()
+
+                       t.Setenv("GO_TEST_KEY_1", "value")
+               })
+       })
+}