From 30719fe26f577f5aa75ccb7c85482e68d0f0e893 Mon Sep 17 00:00:00 2001 From: harjoth Date: Thu, 25 Jun 2026 19:56:02 +0000 Subject: [PATCH] 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 Reviewed-by: Carlos Amedee LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com --- src/cmd/go/testdata/script/test_fail_fast.txt | 19 +++++++++++++++++++ src/testing/testing.go | 13 ++++++++----- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/cmd/go/testdata/script/test_fail_fast.txt b/src/cmd/go/testdata/script/test_fail_fast.txt index 1f169d6da8717b..89a304941e2739 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 @@ stdout -count=5 'FAIL - ' ! 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 TestFailingSubtestsA(t *testing.T) { }) } +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()) } diff --git a/src/testing/testing.go b/src/testing/testing.go index 8b7742d85edd9a..832d9e597fb4a2 100644 --- a/src/testing/testing.go +++ b/src/testing/testing.go @@ -2037,11 +2037,6 @@ func tRunner(t *T, fn func(t *T)) { 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 @@ func tRunner(t *T, fn func(t *T)) { 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