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

[bcmills]

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.

[gopherbot]

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