paralleltest icon indicating copy to clipboard operation
paralleltest copied to clipboard

Incorrect linting of test functions with t.Parallel() inside closures returned by functions within t.Run()

Open wasaga opened this issue 1 year ago • 2 comments

the "good" example referenced in https://github.com/kunwardeep/paralleltest#missing-tparallel-in-the-range-method is incorrect, as it would always capture the last value of the variable tc

// good
func TestFunctionRangeMissingCallToParallel(t *testing.T) {
  t.Parallel()

  testCases := []struct {
    name string
  }{{name: "foo"}}

  for _, tc := range testCases {
    t.Run(tc.name, func(t *testing.T) {
      t.Parallel()
      // ^ call to t.Parallel()
      fmt.Println(tc.name)
    })
  }
}

add more then one case to testCases and run go test -v :

=== RUN   TestFunctionRangeMissingCallToParallel
=== PAUSE TestFunctionRangeMissingCallToParallel
=== CONT  TestFunctionRangeMissingCallToParallel
=== RUN   TestFunctionRangeMissingCallToParallel/foo
=== PAUSE TestFunctionRangeMissingCallToParallel/foo
=== RUN   TestFunctionRangeMissingCallToParallel/bar
=== PAUSE TestFunctionRangeMissingCallToParallel/bar
=== RUN   TestFunctionRangeMissingCallToParallel/baz
=== PAUSE TestFunctionRangeMissingCallToParallel/baz
=== CONT  TestFunctionRangeMissingCallToParallel/foo
baz
=== CONT  TestFunctionRangeMissingCallToParallel/baz
baz
=== CONT  TestFunctionRangeMissingCallToParallel/bar
baz
--- PASS: TestFunctionRangeMissingCallToParallel (0.00s)
    --- PASS: TestFunctionRangeMissingCallToParallel/foo (0.00s)
    --- PASS: TestFunctionRangeMissingCallToParallel/baz (0.00s)
    --- PASS: TestFunctionRangeMissingCallToParallel/bar (0.00s)

The correct implementation should be

func TestFunctionRangeMissingCallToParallel(t *testing.T) {
	t.Parallel()

	testCases := []struct {
		name string
	}{{name: "foo"}, {name: "bar"}, {name: "baz"}}

	for _, tc := range testCases {
		t.Run(tc.name, func(name string) func(t *testing.T) {
			return func(t *testing.T) {
				t.Parallel()
				// ^ call to t.Parallel()
				fmt.Println(name)
			}
		}(tc.name))
	}
}

which would yield

=== RUN   TestFunctionRangeMissingCallToParallel
=== PAUSE TestFunctionRangeMissingCallToParallel
=== CONT  TestFunctionRangeMissingCallToParallel
=== RUN   TestFunctionRangeMissingCallToParallel/foo
=== PAUSE TestFunctionRangeMissingCallToParallel/foo
=== RUN   TestFunctionRangeMissingCallToParallel/bar
=== PAUSE TestFunctionRangeMissingCallToParallel/bar
=== RUN   TestFunctionRangeMissingCallToParallel/baz
=== PAUSE TestFunctionRangeMissingCallToParallel/baz
=== CONT  TestFunctionRangeMissingCallToParallel/foo
foo
=== CONT  TestFunctionRangeMissingCallToParallel/bar
bar
=== CONT  TestFunctionRangeMissingCallToParallel/baz
baz
--- PASS: TestFunctionRangeMissingCallToParallel (0.00s)
    --- PASS: TestFunctionRangeMissingCallToParallel/foo (0.00s)
    --- PASS: TestFunctionRangeMissingCallToParallel/bar (0.00s)
    --- PASS: TestFunctionRangeMissingCallToParallel/baz (0.00s)

However that implementation would not pass the linter check:

Range statement for test TestFunctionRangeMissingCallToParallel missing the call to method parallel in test Run (paralleltest)
        for _, tc := range testCases {

wasaga avatar Apr 04 '23 19:04 wasaga

Hey, sorry for not getting to this in time. Would you please be able to raise a PR for this change?

kunwardeep avatar May 24 '23 00:05 kunwardeep

The same happens to me, I have the following scenario:

func TestMissingCallToSetenv(t *testing.T) {
    ...
    { // Just a block for scoping
        t.Setenv("XXX", "YYY")
        ...
    }
}

and because of that the linter accuses my test of requiring t.Parallel()

bkmeneguello avatar Jun 13 '24 14:06 bkmeneguello