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.

Sign up for GitHub

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

Jump to bottom

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

Closed
Antonboom opened this issue Oct 28, 2023 · 10 comments
Closed

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

Antonboom opened this issue Oct 28, 2023 · 10 comments
Assignees
@timothy-king
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Backlog

Comments

@Antonboom
Copy link

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
@gopherbot gopherbot added this to the Proposal milestone Oct 28, 2023
@Antonboom Antonboom changed the title proposal: go/analysis/passes/testinggoroutine: ignore subtests in gorouting proposal: go/analysis/passes/testinggoroutine: ignore subtests in goroutines Oct 30, 2023
@Antonboom

This comment was marked as duplicate.

Sign in to view
@timothy-king
Copy link
Contributor

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 Antonboom changed the title proposal: go/analysis/passes/testinggoroutine: ignore subtests in goroutines go/analysis/passes/testinggoroutine: ignore subtests in goroutines (false positive) Oct 31, 2023
@Antonboom
Copy link
Author

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

@findleyr findleyr changed the title go/analysis/passes/testinggoroutine: ignore subtests in goroutines (false positive) x/tools/go/analysis/passes/testinggoroutine: ignore subtests in goroutines (false positive) Oct 31, 2023
@findleyr findleyr added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Oct 31, 2023
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Oct 31, 2023
@findleyr findleyr removed the Proposal label Oct 31, 2023
@findleyr
Copy link
Contributor

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

@findleyr findleyr modified the milestones: Proposal, Backlog Oct 31, 2023
@prattmic
Copy link
Member

prattmic commented Nov 1, 2023

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
Copy link

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

@gopherbot
Copy link

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

@Antonboom Antonboom mentioned this issue Nov 2, 2023
Merged
gopherbot pushed a commit that referenced this issue Nov 2, 2023
@prattmic
os/signal: remove go t.Run from TestNohup
5622a4b 
Since CL 226138, TestNohup has a bit of a strange construction: it wants
to run the "uncaught" subtests in parallel with each other, and the
"nohup" subtests in parallel with each other, but also needs join
between "uncaught" and "nohop" so it can Stop notifying for SIGHUP.

It achieves this by doing `go t.Run` with a WaitGroup rather than using
`t.Parallel` in the subtest (which would make `t.Run` return immediately).

However, this makes things more difficult to understand than necessary.
As noted on https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks,
a second layer of subtest can be used to join parallel subtests.

Switch to this form, which makes the test simpler to follow
(particularly the cleanup that goes with "uncaught").

For #63799.

Change-Id: Ibfce0f439508a7cfca848c7ccfd136c9c453ad8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/538899
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Dec 7, 2023
@prattmic @cagedmantis
[release-branch.go1.21] os/signal: remove go t.Run from TestNohup
7dc67e8 
Since CL 226138, TestNohup has a bit of a strange construction: it wants
to run the "uncaught" subtests in parallel with each other, and the
"nohup" subtests in parallel with each other, but also needs join
between "uncaught" and "nohop" so it can Stop notifying for SIGHUP.

It achieves this by doing `go t.Run` with a WaitGroup rather than using
`t.Parallel` in the subtest (which would make `t.Run` return immediately).

However, this makes things more difficult to understand than necessary.
As noted on https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks,
a second layer of subtest can be used to join parallel subtests.

Switch to this form, which makes the test simpler to follow
(particularly the cleanup that goes with "uncaught").

For #63799.
For #63911.

Change-Id: Ibfce0f439508a7cfca848c7ccfd136c9c453ad8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/538899
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
(cherry picked from commit 5622a4b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/546021
gopherbot pushed a commit that referenced this issue Dec 7, 2023
@prattmic @cagedmantis
[release-branch.go1.20] os/signal: remove go t.Run from TestNohup
59ffd3b 
Since CL 226138, TestNohup has a bit of a strange construction: it wants
to run the "uncaught" subtests in parallel with each other, and the
"nohup" subtests in parallel with each other, but also needs join
between "uncaught" and "nohop" so it can Stop notifying for SIGHUP.

It achieves this by doing `go t.Run` with a WaitGroup rather than using
`t.Parallel` in the subtest (which would make `t.Run` return immediately).

However, this makes things more difficult to understand than necessary.
As noted on https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks,
a second layer of subtest can be used to join parallel subtests.

Switch to this form, which makes the test simpler to follow
(particularly the cleanup that goes with "uncaught").

For #63799.
For #63910.

Change-Id: Ibfce0f439508a7cfca848c7ccfd136c9c453ad8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/538899
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
(cherry picked from commit 5622a4b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/546375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timothy-king timothy-king

Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@timothy-king @prattmic @gopherbot @Antonboom @findleyr