testing: record a test's failure for -failfast after cleanup runs

The -failfast flag stops a test binary from starting new tests after
the first failure.

When a test had a parallel subtest, its failure was recorded before
its cleanup functions ran. A failure inside a t.Cleanup was therefore
not counted, and -failfast kept starting new tests. Tests without a
parallel subtest run their cleanup earlier and were unaffected.

Record the failure after cleanup has run, so a cleanup failure counts
for -failfast regardless of whether the test has a parallel subtest.

Fixes #61034

Change-Id: Icea654cbaf3904936200566d7d7d72ca8ccc5958
GitHub-Last-Rev: e46ad5ad314c5cee2a835be3627f4a6455b81c32
GitHub-Pull-Request: golang/go#80165
Reviewed-on: https://go-review.googlesource.com/c/go/+/794541
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/cmd/go/testdata/script/test_fail_fast.txt

diff --git a/src/cmd/go/testdata/script/test_fail_fast.txt b/src/cmd/go/testdata/script/test_fail_fast.txt
index 1f169d6..89a3049 100644
--- a/src/cmd/go/testdata/script/test_fail_fast.txt
+++ b/src/cmd/go/testdata/script/test_fail_fast.txt
@@ -32,6 +32,13 @@
 ! go test ./failfast_test.go -run='TestParallelFailingSubtestsA' -failfast=true
 stdout -count=1 'FAIL - '
 
+# a failure in a cleanup of a test with a parallel subtest must stop failfast
+# (go.dev/issue/61034)
+! go test ./failfast_test.go -run='TestCleanupFailingWithParallelSubtestA|TestFailingB' -failfast=true
+stdout -count=1 'FAIL - '
+! go test ./failfast_test.go -run='TestCleanupFailingWithParallelSubtestA|TestFailingB' -failfast=false
+stdout -count=2 'FAIL - '
+
 # only parallels
 ! go test ./failfast_test.go -run='TestParallelFailing[AB]' -failfast=false
 stdout -count=2 'FAIL - '
@@ -109,6 +116,18 @@
 	})
 }
 
+func TestCleanupFailingWithParallelSubtestA(t *testing.T) {
+	// Regression test for go.dev/issue/61034: a failure in a Cleanup
+	// function must be counted for -failfast even when the test has a
+	// parallel subtest.
+	t.Cleanup(func() {
+		t.Errorf("FAIL - %s", t.Name())
+	})
+	t.Run("sub", func(t *testing.T) {
+		t.Parallel()
+	})
+}
+
 func TestFailingB(t *testing.T) {
 	t.Errorf("FAIL - %s", t.Name())
 }

src/testing/testing.go

diff --git a/src/testing/testing.go b/src/testing/testing.go
index 8b7742d..832d9e5 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -2037,11 +2037,6 @@
 	defer func() {
 		t.checkRaces()
 
-		// TODO(#61034): This is the wrong place for this check.
-		if t.Failed() {
-			numFailed.Add(1)
-		}
-
 		// Check if the test panicked or Goexited inappropriately.
 		//
 		// If this happens in a normal test, print output but continue panicking.
@@ -2170,6 +2165,14 @@
 		for root := &t.common; root.parent != nil; root = root.parent {
 			root.flushPartial()
 		}
+
+		// Record this test's failure for -failfast. This must happen after the
+		// test's cleanup functions have run, since a cleanup may call t.Fail.
+		// See go.dev/issue/61034.
+		if t.Failed() {
+			numFailed.Add(1)
+		}
+
 		t.report() // Report after all subtests have finished.
 
 		// Do not lock t.done to allow race detector to detect race in case