x/tools/go/analysis/passes/testinggoroutine: ignore subtests in goroutines (false positive)

[Antonboom]

Hi!

Looks like invalid checker behaviour:

func TestGoSubtest(t *testing.T) {
	done := make(chan struct{})
	go func() {
		defer close(done)
		t.Run("", func(t *testing.T) {
			t.Fatal()
		})
	}()
	<-done
}

$ go vet -testinggoroutine some_test.go
# command-line-arguments_test
..._test.go:56:4: call to (*T).Fatal from a non-test goroutine

[timothy-king]

https://pkg.go.dev/testing#T.Run

Run runs f as a subtest of t called name. It runs f in a separate goroutine and blocks until f returns or calls t.Parallel to become a parallel test. Run reports whether f succeeded (or at least did not fail before calling t.Parallel).
Run may be called simultaneously from multiple goroutines, but all such calls must return before the outer test function for t returns.

I would say based on the documentation of T.Run the first example is a false positive. IIUC this can be treated as a bug and does not need to go through the proposal process.

The second example is a request to improve the precision on a false negative. Unless we believe this is an important escape hatch, I am inclined to believe that this also does not need to go through a proposal. That said my gut is that the frequency for this is low in practice. If someone is interested in contributing this, I can review it.

[Antonboom]

Changed the title and moved the second in the separate issue:
#63849

[findleyr]

Thanks, this looks like it's just a bug, so no need for a proposal.

[prattmic]

I tripped over this with t.Skip in https://go.dev/cl/538698, which has a test that does go t.Run(...). I worked around with t.Logf + return.

# os/signal
# [os/signal]
os/signal/signal_test.go:490:3: call to (*T).Skip from a non-test goroutine
FAIL    os/signal [build failed]
FAIL

[gopherbot]

Change https://go.dev/cl/538698 mentions this issue: os/signal: skip nohup tests on darwin builders

[gopherbot]

Change https://go.dev/cl/538899 mentions this issue: os/signal: remove go t.Run from TestNohup

[gopherbot]

Change https://go.dev/cl/541155 mentions this issue: go/analysis/passes/testinggoroutine: report by enclosing regions

[gopherbot]

Change https://go.dev/cl/546375 mentions this issue: [release-branch.go1.20] os/signal: remove go t.Run from TestNohup

[gopherbot]

Change https://go.dev/cl/546021 mentions this issue: [release-branch.go1.21] os/signal: remove go t.Run from TestNohup