From d6ca24477afa85a3ab559935faa4fed917911e4f Mon Sep 17 00:00:00 2001 From: Nobuki Fujii Date: Sun, 18 Sep 2022 11:52:07 +0900 Subject: [PATCH] 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 Reviewed-by: Ian Lance Taylor Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot Auto-Submit: Bryan Mills --- src/testing/testing.go | 20 ++++++++++++++++---- src/testing/testing_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/testing/testing.go b/src/testing/testing.go index 7e86faf950..b64286c005 100644 --- 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 08ae23991f..3616f04d5f 100644 --- 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") + }) + }) +} -- 2.50.0