Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bcmills opened this issue Jun 27, 2023 · 3 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Jun 27, 2023

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.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 27, 2023
@bcmills bcmills added this to the Backlog milestone Jun 27, 2023
@gopherbot
Copy link

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

@bcmills bcmills modified the milestones: Backlog, Go1.22 Jun 28, 2023
@bcmills bcmills self-assigned this Jun 28, 2023
@bcmills bcmills reopened this Nov 29, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Nov 29, 2023

Reopening because the fix was reverted (#64402).

@ansel1
Copy link

ansel1 commented Jan 9, 2024

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

@bcmills bcmills removed their assignment Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants