Tracking issue for restructure tests

[tisonkun]

Brief Motivation

This issue tracks the efforts to restructure TiDB tests, focusing on unit tests and integration tests, especially on separating integration_test.go as well as unifying test infrastructure in testify.

For the detailed design, see also:

• Create design doc for restructure tests #26023

Join force

We are currently on the first phase on the effort and focus on migrating tests to testify. If you want to participant the movement, you can directly comment under any open issues below without assignee to require assignment.

You can take a look at #26375 as an example to do the migration.

Note

Before writing code, please take a look at PRs merged already and keep consistent code style.

• parallel
  • As mentioned in test: make all the tests run in serial #30692, we decided to run all tests in serial.
  • Keep test cases as parallel as possible.
  • When some test cases should be run in serial, take use of go testing subtests (t.Run(name string, f func(t *T))) and parallel the parent test only. In this way, test cases in the same subtests set run in serial.
  • Tracking issue for restructure tests #26022 (comment)
  • When test cases should be run in serial at top level, move them into xxx_serial_test.go file.
• Avoid using suite of testify as discussed errno: migrate test-infra to testify #26082 (comment) and migrate test-infra to testify for br/pkg #27178 (comment)
• Make use of testify checkers when the original code writes plain go testing cases.
• c.Assert is almost require.Xxx

Migration

• Introduce testify dependency #26025
• migrate test-infra to testify for bindinfo pkg #27177
• migrate test-infra to testify for br/pkg #27178
• migrate test-infra to testify for cmd pkg #27844
• migrate test-infra to testify for config pkg #27179
• migrate test-infra to testify for ddl pkg #27180
• migrate test-infra to testify for distsql pkg #26156
• migrate test-infra to testify for domain pkg #26306
• migrate test-infra to testify for domain/infosync pkg #26155
• migrate test-infra to testify for errno pkg #26081
• migrate test-infra to testify for executor #26854
• migrate test-infra to testify for expression #26855
• migrate test-infra to testify for infoschema #28319
• migrate test-infra to testify for infoschema/perfschema pkg #26436
• migrate test-infra to testify for kv pkg #26100
• migrate test-infra to testify for meta pkg #26740
• migrate test-infra to testify for meta/autoid pkg #26741
• migrate test-infra to testify for metrics pkg #26742
• migrate test-infra to testify for owner pkg #26743
• migrate test-infra to testify for parser #28887
• migrate test-infra to testify for planner/cascades #26856
• migrate test-infra to testify for planner/core #26857
• migrate test-infra to testify for planner/implementation #26858
• migrate test-infra to testify for planner/memo #26860
• migrate test-infra to testify for planner/property #26861
• migrate test-infra to testify for planner/util #26862
• migrate test-infra to testify for plugin pkg #26744
• migrate test-infra to testify for plugin/conn_ip_example_test.go #26745
• migrate test-infra to testify for privilege/privileges pkg #26746
• migrate test-infra to testify for server #26863
• migrate test-infra to testify for session #26864
• migrate test-infra to testify for sessionctx #27140
• migrate test-infra to testify for statistics #27141
• migrate test-infra to testify for store #27142
• migrate test-infra to testify for tests folder #27874
• migrate test-infra to testify for tidb-binlog/driver/reader pkg #33461
• migrate test-infra to testify for tidb-binlog/node pkg #33462
• migrate test-infra to testify for tidb-binlog/pump_client pkg #33463
• migrate test-infra to testify for tidb-server #27143
• migrate test-infra to testify for structure #27139
• migrate test-infra to testify for table #26865
• migrate test-infra to testify for table/tables #26866
• migrate test-infra to testify for tablecodec #26867
• migrate test-infra to testify for telemetry pkg #26537
• migrate test-infra to testify for types pkg #26747
• migrate test-infra to testify for types/json pkg #26374
• migrate test-infra to testify for types/driver pkg #26521
• migrate test-infra to testify for util folder #33453

Kits

• Implement flags bridge to workaround tests fail with undefined flags #26106
• Adopt go.uber.org/goleak for leak tests #26107
• Implement testkits for server tests #26108
• limit the time of a single unit test case #26289

Reference

• testify suffers from parallelizing tests in suite Running Parallel Tests in Suite stretchr/testify#187

[jyz0309]

A little question, if we use testify to instead pingcap/check, maybe we need to restructure the /utils/testkit first? It seem that we use TestKit.MustExec to exec sql in TiDB test, and MustExec need to use pingcap/check.

[tisonkun]

@jyz0309 Yes of course. That is in the plan and we will create testkit with go testing & testify. We use go-check for matchers almost and it won't be too complex.

Tracked as #26108

[tisonkun]

@jyz0309 BTW #26102 & #26103 are still free to be assigned. If you want to take one of them, feel free to comment there.

[tisonkun]

In an offline discussion with @tiancaiamao it seems we'd better consider test timeout in this effort also.

gocheck provides a check.timeout mechanism but the current state seems unsatisfied (why?). golang itself doesn't provides timeout per function.

After a little investigation, I write a helper for this requirement

func RunWithTimeout(t *testing.T, d time.Duration, f func(*testing.T)) {
	timeout := time.After(d)
	done := make(chan bool)
	go func() {
		defer func() {
			done <- true
		}()
		f(t)
	}()
	select {
	case <-timeout:
		t.Fatalf("timeout: %s", d.String())
	case <-done:
	}
}

... which is used as

func TestName(t *testing.T) {
	testkit.RunWithTimeout(t, 1 * time.Second, func(t *testing.T) {
		require.Equal(t, 2, 2)
		time.Sleep(2 * time.Second)
	})
}

.. one more level of ident but works well.

However, @tiancaiamao means the original issue is that a panic from a previous test cause the whole test hang. I don't know what does it mean, since a main goroutine panic will fail the test immediately.

[tiancaiamao]

the original issue is that a panic from a previous test cause the whole test hang. I don't know what does it mean

It's a problem of gocheck, it spawn new goroutines to run the test cases, so a panic does not make the process exit immediately. @tisonkun

[PragmaTwice]

Hi @tisonkun, I'd like to migrate some tests under utils pkg, is there any task still free to assign?

[tisonkun]

@PragmaTwice most of util/xxx are free to assign, you may just jump to subtasks listed above.

[beyoung]

Hi @tisonkun ,I'd like to migrate util/plancodec pkg , which is not listed above, please create a subtask and assign to me . Thx.

[tisonkun]

Hi @tisonkun ,I'd like to migrate util/plancodec pkg , which is not listed above, please create a subtask and assign to me . Thx.

Aha! That's it. Let me create one for you.

[tisonkun]

@tisonkun It seems failpoints conflict with each others. Let me batch domain_test to run in serial.
@xhebox failpoints and global config are both serial-safe, maybe mention it in the tracking issue.

#26752

[w169q169]

@tisonkun You idea is same as me, tools could do easy work and more complex work need be checked manually .
Review is always necessary, bad/corner cases that I dont know such as c.Check(len(bindData.Bindings), Equals, 0) and c.Assert(err, IsNil), I have added feature to fix the tools .

[tisonkun]

As mentioned in #30692, we decided to run all tests in serial.

Thanks to @tangenta for driving the decision and I've updated the description in this issue. Thanks for all of your participants.

[dveeden]

@tisonkun I assume https://pingcap.github.io/tidb-dev-guide/get-started/write-and-run-unit-tests.html#parallel needs updating? Should that section be replaced by an explanation about why we want to run things in serial? or keep it?

[tisonkun]

So...

As #33296 got merged, we have new subtasks to resolve.

[tisonkun]

Closed as all subtasks resolved. Thanks for everyone help in this thread!