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

cmd/vet: check for improper use of t.Run and t.Parallel #18106

Closed
dsnet opened this issue Nov 29, 2016 · 5 comments

Comments

Projects
None yet
6 participants
@dsnet
Copy link
Member

commented Nov 29, 2016

Consider the following test:

func Test(t *testing.T) {
	for _, name := range []string{"foo", "bar", "baz"} {
		data := name // This line is necessary
		t.Run(name, func(t *testing.T) {
			t.Parallel()
			t.Log(data)
		})
	}
}

Without the name := name line to capture the name variable, the subtest will execute with the wrong values as the testdata. Worst yet, the tests will run with the correct names, but the wrong data, completely misleading the user that the test is executing as expected.

The blog on subtests explicitly warns that this must be done:

Note that we need to capture the range variable to ensure that tc gets bound to the correct instance.

I propose that vet check for this mis-use. One possible algorithm:

  • If t.Parallel is called within a t.Run
  • and the body of t.Run following the t.Parallel references a variable scoped to the range of some outer loop (i.e., not captured)
  • then flag as potential issue.

\cc @dominikh

@quentinmit quentinmit added this to the Go1.9Maybe milestone Nov 29, 2016

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2016

What is the frequency with which this occurs?

@minux

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

@gonzaloserrano

This comment has been minimized.

Copy link

commented Mar 17, 2017

For the record it just happened to us at Social Point. Maybe a rule in https://github.com/dominikh/go-staticcheck would make it too if in vet is not possible. cc/ @dominikh

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2017

#16520 is the generalization of this. This is marked as NeedsDecision but really it needs a concrete proposal of what gets checked and how, and an argument for why that will have approximately no false positives.

@rsc rsc added the WaitingForInfo label Jun 12, 2017

@rsc rsc modified the milestones: Go1.10, Go1.9Maybe Jun 12, 2017

@rsc rsc removed the NeedsDecision label Jun 12, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Jul 12, 2017

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@gopherbot gopherbot closed this Jul 12, 2017

@golang golang locked and limited conversation to collaborators Jul 12, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.