maybe common mistakes with table driven tests and t.Parallel()

[feitian124]

Bug Report

I am tring to figure out a strang problem in pr #27557, in which 2 tests fail if i add t.Parallel() and work well if i removed it. Then when i tried to find more info about t.Parallel(), i found that there maybe an common mistake in our test.

problem

func TestWithParallel(t *testing.T) {
	tests := []struct {
		name  string
		value int
	}{
		{name: "test 1", value: 4},
		{name: "test 2", value: 4},
		{name: "test 3", value: 4},
		{name: "test 4", value: 4},
	}
	for i, tc := range tests {
		t.Run(tc.name, func(t *testing.T) {
                         // uncomment below line will make 4 tests pass
			//t.Parallel()
			fmt.Printf("%+v == %+v \n", i+1, tc.value)
			require.Equal(t, i+1, tc.value)
		})
	}
}

for above table driven tests, test1, test2, test3 should fail, test 4 should pass, it works as expect.
but if we add t.Parallel() by uncomment the line, 4 test will pass, and the output is:

4 == 4 
4 == 4 
4 == 4 
4 == 4 

reason

the reason is explained well in Go common mistakes guide, 2 reason:

1. In Go, the loop iterator variable is a single variable that takes different values in each loop iteration
2. there is a very good chance that when you run this code you will see the last element printed for every iteration instead of each value in sequence, because the goroutines will probably not begin executing until after the loop

how to resolve

• adding val as a parameter to the closure

for _, val := range values {
	go func(val interface{}) {
		fmt.Println(val)
	}(val)
}

• Copy iterator variable into a new variable

 for i := 0; i < 3; i++ {
+	i := i // Copy i into a new variable.
 	out = append(out, &i)
 }

also, we may add linter to avoid such pitfall https://github.com/kunwardeep/paralleltest

todo

i seached repo, there already some wrong occurance there, such as request_builder_test.go , table/column_test.go,
i am not sure how many contributors are awared of this, the occurances ge merged means even some experienced reviewer may not aware of it.

so i submit a issue here, we need fix them and pay attention to this scenario, and may also add notice to https://pingcap.github.io/tidb-dev-guide/get-started/write-and-run-unit-tests.html

[feitian124]

@tisonkun please take a look and confirm, thanks

[tisonkun]

I think you can add the linter and trying to generate a report on how much we suffer from this problem.

For the PR #27557 , just fix it and move forward.

[feitian124]

paralleltest.txt

above is paralleltest run result, it checks several cases:

• Missing t.Parallel() in the test method
• t.Parallel() is called in the range method but testcase name not being used
• t.Parallel() is called in the range method and test case variable tc being used, but is not reinitialised
  ...

i will only pay attention to the third one and try fix them, about 14 occurrences

[feitian124]

more detail here: https://golang.org/doc/faq#closures_and_goroutines

[tiancaiamao]

Write t.Parallel() is not required now.
We use another way to execute the test cases in a single OS process, parallelly #30822