Golang testing: failures in Cleanup functions do not trigger -failfast if the test includes a parallel subtest

When run with -failfast, the following test should run only Test1, which fails. Instead, it reports the failure from Test1 and then proceeds to run Test2.

https://go.dev/play/p/OiVQBuQ7fJl:

package main

import "testing"

func Test1(t *testing.T) {
    t.Cleanup(func() {
        t.Errorf("fail in Cleanup")
    })

    t.Run("sub", func(t *testing.T) {
        t.Parallel()
    })
}

func Test2(t *testing.T) {
    t.Fatalf("This test should not run with -failfast.")
}

Without the call to t.Parallel(), it skips Test2 as expected.

This is because the numFailed variable used for -failfast is incremented before cleanup, only for tests that have parallel subtests.

I do not see a compelling reason for the difference in codepaths between tests with parallel subtests and those without. But I also don't see why we need to keep track of the number of failed tests explicitly — it seems like we could instead just set a boolean when any test fails.

Comment From: gopherbot

Change https://go.dev/cl/506755 mentions this issue: testing: simplify concurrency and cleanup

Comment From: bcmills

Reopening because the fix was reverted (#64402).

Comment From: ansel1

The CL originally submitted for this (which was later reverted) also fixes #49929