testing: simplify concurrency and cleanup logic

While investigating #60083, I found a couple of bugs (notably #61034)
that had slipped through code review in part because the concurrency
patterns used in the testing package were too complex for me to fully
reason about. This change adjusts those patterns to be more in line
with current idioms, and to reduce the number of special cases that
depend on details that should be orthogonal. (For example: the details
of how we invoke the Cleanup functions should not depend on whether
the test happened to run any parallel subtests.)

In the process, this change fixes a handful of bugs:

- Concurrent calls to Run (explicitly allowed by TestParallelSub)
  could previously drive the testcontext.running count negative,
  causing the number of running parallel tests to exceed the -parallel
  flag.

- The -failfast flag now takes effect immediately on failure. It no
  longer delays until the test finishes, and no longer misses failures
  during cleanup (fixing #61034).

- If a Cleanup function calls runtime.Goexit (typically via t.FailNow)
  during a panic, Cleanup functions from its parent tests are no
  longer skipped and buffered logs from its parent tests are now
  flushed.

- The time reported for a test with subtests now includes the time spent
  running those subtests, regardless of whether they are parallel.
  (Previously, non-parallel subtests were included but parallel subtests
  were not.)

- Calls to (*B).Run in iterations after the first are now diagnosed
  with a panic. (This diagnoses badly-behaved benchmarks: if Run is
  called during the first iteration, no subsequent iterations are
  supposed to occur.)

Fixes #61034.

Change-Id: I3797f6ef5210a3d2d5d6c2710d3f35c0219b02ea
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-linux-amd64-longtest-race,gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/506755
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

1 files changed, 48 insertions, 37 deletions

diff --git a/src/testing/panic_test.go b/src/testing/panic_test.go
index 6307b84a7a..82e7087025 100644
--- a/src/testing/panic_test.go
+++ b/src/testing/panic_test.go
@@ -211,57 +211,68 @@ func TestPanicHelper(t *testing.T) {
 }
 
 func TestMorePanic(t *testing.T) {
-	testenv.MustHaveExec(t)
+	subprocess := false
+	if os.Getenv("GO_WANT_HELPER_PROCESS") == "1" {
+		subprocess = true
+	} else {
+		testenv.MustHaveExec(t)
+		t.Parallel()
+	}
 
 	testCases := []struct {
+		issue int
 		desc  string
-		flags []string
+		f     func(*testing.T)
 		want  string
 	}{
 		{
-			desc:  "Issue 48502: call runtime.Goexit in t.Cleanup after panic",
-			flags: []string{"-test.run=^TestGoexitInCleanupAfterPanicHelper$"},
-			want: `panic: die
-	panic: test executed panic(nil) or runtime.Goexit`,
+			issue: 48502,
+			desc:  "runtime.Goexit in t.Cleanup after panic",
+			f: func(t *testing.T) {
+				t.Cleanup(func() {
+					t.Log("Goexiting in cleanup")
+					runtime.Goexit()
+				})
+				t.Parallel()
+				panic("die")
+			},
+			want: `panic: die [recovered]
+	panic: die`,
 		},
 		{
-			desc:  "Issue 48515: call t.Run in t.Cleanup should trigger panic",
-			flags: []string{"-test.run=^TestCallRunInCleanupHelper$"},
-			want:  `panic: testing: t.Run called during t.Cleanup`,
+			issue: 48515,
+			desc:  "t.Run in t.Cleanup should trigger panic",
+			f: func(t *testing.T) {
+				t.Cleanup(func() {
+					t.Run("in-cleanup", func(t *testing.T) {
+						t.Log("must not be executed")
+					})
+				})
+			},
+			want: `panic: testing: t.Run called during t.Cleanup`,
 		},
 	}
 
 	for _, tc := range testCases {
-		cmd := exec.Command(os.Args[0], tc.flags...)
-		cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1")
-		b, _ := cmd.CombinedOutput()
-		got := string(b)
-		want := tc.want
-		re := makeRegexp(want)
-		if ok, err := regexp.MatchString(re, got); !ok || err != nil {
-			t.Errorf("output:\ngot:\n%s\nwant:\n%s", got, want)
-		}
-	}
-}
+		tc := tc
+		t.Run(fmt.Sprintf("issue%v", tc.issue), func(t *testing.T) {
+			if subprocess {
+				tc.f(t)
+				return
+			}
 
-func TestCallRunInCleanupHelper(t *testing.T) {
-	if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" {
-		return
-	}
+			t.Parallel()
+			cmd := testenv.Command(t, os.Args[0], "-test.run="+t.Name())
+			cmd.Env = append(cmd.Environ(), "GO_WANT_HELPER_PROCESS=1")
+			b, _ := cmd.CombinedOutput()
+			got := string(b)
+			t.Logf("%v:\n%s", tc.desc, got)
 
-	t.Cleanup(func() {
-		t.Run("in-cleanup", func(t *testing.T) {
-			t.Log("must not be executed")
+			want := tc.want
+			re := makeRegexp(want)
+			if ok, err := regexp.MatchString(re, got); !ok || err != nil {
+				t.Errorf("wanted:\n%s", want)
+			}
 		})
-	})
-}
-
-func TestGoexitInCleanupAfterPanicHelper(t *testing.T) {
-	if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" {
-		return
 	}
-
-	t.Cleanup(func() { runtime.Goexit() })
-	t.Parallel()
-	panic("die")
 }