ddl/table_test.go: migrate infra to testify #29288

clovis1122 · 2021-10-31T20:44:44Z

What problem does this PR solve?

Issue Number: close #29116

Problem Summary:

What is changed and how it works?

Uses testify for ddl/table_test.go. In the process, I had to make the same change across multiple test files for the tests to compile.

Could not fully run tests on my machine as it went out of memory very fast.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2021-10-31T20:44:46Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2021-10-31T20:44:56Z

Welcome @clovis1122!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to pingcap/tidb. 😃

clovis1122 · 2021-10-31T20:47:19Z

cc: @tisonkun, can you let me know if this is good enough?

tisonkun

Please take a look at #27747 to know how to migrate a test suite. After you migrate "pingcap/check" to "testify", suite magic doesn't work and you should change the pattern.

Also, a basic sanity check is that after the migration, the files in issue should not import "pingcap/check".

You should be able to run ddl test locally by

make failpoint-enable
cd ddl
go test

otherwise you make invalid changes.

clovis1122 · 2021-11-01T02:38:50Z

Got it, thanks yeah I was looking at earlier PRs to figure out how to proceed. I cannot run all the tests because my laptop runs out of memory (I'm using Windows w/ wsl, I have 16GB but limited WSL to I believe). I'm currently seeing some failed tests on the CI and got some ideas to try in order to fix it (will try later today or tomorrow).

Just wanted to check if it's OK for me to touch so many files. I hope it doesn't make code review hard 😅.

One thing that wasn't super obvious for me is... which tests should be parallel (call t.Parallel() at the top)?

Also, I hope it is OK to leave pingcap/check on the files that I didn't intend to fix... I mostly changed references outside ddl/table_test.go because the project wouldn't compile otherwise. This work should make it easier to migrate these files later.

tisonkun · 2021-11-01T02:43:28Z

Just wanted to check if it's OK for me to touch so many files. I hope it doesn't make code review hard 😅.

It's totally OK to touch multiple files. Never mind. But yes please keep necessary changes only :)

Also, I hope it is OK to leave pingcap/check on the files that I didn't intend to fix

Of course.

clovis1122 · 2021-11-02T00:06:26Z

ddl/ddl_test.go

-	c.Assert(err, IsNil)
+	require.NoError(t, err)
+
+	_, err = infosync.GlobalInfoSyncerInit(context.Background(), "t", func() uint64 { return 1 }, nil, true)


I may be overlooking something, but I had to add this initializer somewhere, otherwise, the Table* tests would fail after migrating it to Testify.

My guess is that infosync on this context was already being initialized somewhere else, but I could not figure it out... the other place I see is the TestT function in this same file, but that is not being used by the Table* tests.

clovis1122 · 2021-11-04T12:25:44Z

@tisonkun is there flakiness in the tests? It looks like all the ddl tests are passing by now... but this test on an unrelated file: handle_test.go:960: testSerialStatsSuite.TestAnalyzeGlobalStatsWithOpts2 is failing. When I run the test locally via cd statistics && go test -run TestAnalyzeGlobalStatsWithOpts2, it works.

tisonkun · 2021-11-04T13:19:44Z

@clovis1122 unfortunately yes. You can try to find all unstable tests #25899 and for the known TestAnalyzeGlobalStatsWithOpts2 #24679

clovis1122 · 2021-11-05T11:09:40Z

@tisonkun it sounds like I should trigger the tests until they pass or until I consistently get the same test error? How do I do that?

tisonkun · 2021-11-05T12:19:03Z

/run-all-tests

clovis1122 · 2021-11-05T19:59:43Z

Ohhh they finally passed! Can I get a review? @tisonkun

tisonkun · 2021-11-05T23:58:13Z

@clovis1122 you also touch other files beyond adjust methods but change every line from pingcap/check to testify. It makes a 1000 lines diff which is hard to review. Please consider revert unnecessary changes or take over relate issues and separate different purpose into dedicated commit.

tisonkun · 2021-11-06T00:04:51Z

ddl/column_change_test.go

 }

-func (s *testColumnChangeSuite) TestModifyAutoRandColumnWithMetaKeyChanged(c *C) {
+func TestModifyAutoRandColumnWithMetaKeyChanged(t *testing.T) {


For example, this file is not under ddl/table. If reviewers found a 1000 lines diff including non trivial change, most of them will simply give up to review.

Take a look at #26022 and see that we either run parallel tests, or serial test in xxx_serial_test.go.

tisonkun · 2021-11-06T00:07:25Z

ddl/ddl_test.go

-	c.Assert(historyJob.BinlogInfo.SchemaVersion, Equals, args.ver)
-	c.Assert(historyJob.BinlogInfo.DBInfo, DeepEquals, args.db)
+	require.Equal(t, historyJob.BinlogInfo.SchemaVersion, args.ver)
+	if !reflect.DeepEqual(historyJob.BinlogInfo.DBInfo, args.db) {


require.Equal is bitwise equality checks. BTW this file is also not under ddl/table. We break down subtasks for small changes easy to review and merge, and then repeat it for the next iteration.

From #29288 (comment)

It's totally OK to touch multiple files. Never mind. But yes please keep necessary changes only :)

Please notice please keep necessary changes only.

Thanks! Changed it to require.Equal.

tisonkun · 2021-11-06T00:09:03Z

And actually you mean ddl/table_test.go, not a ddl/table package...

tisonkun · 2021-11-06T00:12:43Z

Fine. I can see that due to the out of order code arrange in ddl tests ( cc @zimulala ) it's hard to separate tests and decide whether to touch. They are all tightly coupled.

clovis1122 · 2021-11-06T01:20:55Z

@tisonkun yes, it's exactly as you said :(. 7 different tests including ddl/table_test.go depends on the utility functions that are defined in ddl/ddl_test.go.

(There are more dependencies, this is just one of them).

We could temporally duplicate the code if the review is hard...

clovis1122 · 2021-11-06T01:23:45Z

/run-all-tests

zimulala · 2021-11-08T02:47:29Z

@tisonkun yes, it's exactly as you said :(. 7 different tests including ddl/table_test.go depends on the utility functions that are defined in ddl/ddl_test.go.

(There are more dependencies, this is just one of them).

We could temporally duplicate the code if the review is hard...

Is it possible to move the tests in ddl/ddl_worker_test.go file to the next PR? In this way, this PR and the next PR are relatively convenient to review.

clovis1122 · 2021-11-08T03:09:59Z

@zimulala not possible because ddl/ddl_worker_test.go:61 has a call to testNewDDLAndStart which resides in ddl/ddl_test.go:100 and is reused in many tests.

Some ideas:

I can move every file to a separate commit if that will make it easier to review?
I can duplicate the code, not sure if desirable...
I can refactor the methods that are reused in many places generic so that they do not depend on the test framework (will not take c *C or t *testing.T as arguments). I can see how methods like testNewDDLAndStart can return an error instead of performing assertions. But refactoring the methods that perform assertions inside goroutines may be more complicated...

clovis1122 · 2021-11-09T21:02:53Z

@zimulala @tisonkun what would you guys like to do with this?

xhebox · 2021-11-15T03:49:42Z

@zimulala @tisonkun what would you guys like to do with this?

I'll try to review it today.

tisonkun · 2021-11-15T04:43:02Z

@clovis1122 you can fix the conflict and I'll review it this week.

/cc

clovis1122 · 2021-11-19T13:23:07Z

I'll rebase and fix the conflicts later today!

clovis1122 · 2021-11-21T14:41:16Z

@tisonkun to save everybody's time, I think it may be best to start with smaller changes. I'll work on a series of small PRs to decouple the utils functions in ddl_test.go from the check argument. Hopefully, that will make this and future migrations easier.

I created another PR, #29964. I hope to re-open this PR when the change is small enough so that it can be reviewed and merged without problems.

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 31, 2021

tisonkun self-requested a review November 1, 2021 00:12

tisonkun requested changes Nov 1, 2021

View reviewed changes

clovis1122 force-pushed the use-testify branch 2 times, most recently from 5a6955c to af0d09c Compare November 2, 2021 00:02

clovis1122 commented Nov 2, 2021

View reviewed changes

clovis1122 force-pushed the use-testify branch 2 times, most recently from 4c7478e to 173b33d Compare November 2, 2021 05:23

xhebox self-requested a review November 3, 2021 07:28

clovis1122 force-pushed the use-testify branch from c402f31 to f5f8897 Compare November 4, 2021 10:54

clovis1122 requested a review from tisonkun November 5, 2021 10:54

tisonkun reviewed Nov 6, 2021

View reviewed changes

clovis1122 changed the title ~~ddl/table: migrate infra to testify~~ ddl/table_test.go: migrate infra to testify Nov 6, 2021

clovis1122 changed the title ~~ddl/table_test.go: migrate infra to testify~~ ddl/table_test: migrate infra to testify Nov 6, 2021

clovis1122 changed the title ~~ddl/table_test: migrate infra to testify~~ ddl/table_test.go: migrate infra to testify Nov 6, 2021

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 8, 2021

clovis1122 force-pushed the use-testify branch from 9b89968 to 9bb9a33 Compare November 9, 2021 21:02

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2021

ti-chi-bot requested a review from tisonkun November 15, 2021 04:43

clovis1122 force-pushed the use-testify branch from efa7164 to 0ca8779 Compare November 16, 2021 10:55

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2021

clovis1122 force-pushed the use-testify branch from 0ca8779 to be7e01b Compare November 16, 2021 10:59

clovis1122 added 4 commits November 21, 2021 10:04

Use testify for ddl/table_test.go

8f3adc3

Fix loop in reorg

4503228

Fix test in restart_test.go

5d7a49f

Replace reflect.DeepEqual check with require.Equal

0d3e9bf

clovis1122 force-pushed the use-testify branch from 518def9 to 0d3e9bf Compare November 21, 2021 14:04

Fix syntax error

28ae481

clovis1122 closed this Nov 21, 2021

tisonkun mentioned this pull request Jan 4, 2022

migrate test-infra to testify for ddl/schema_test.go #29111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl/table_test.go: migrate infra to testify #29288

ddl/table_test.go: migrate infra to testify #29288

clovis1122 commented Oct 31, 2021 •

edited

Loading

ti-chi-bot commented Oct 31, 2021

ti-chi-bot commented Oct 31, 2021

clovis1122 commented Oct 31, 2021

tisonkun left a comment •

edited

Loading

clovis1122 commented Nov 1, 2021 •

edited

Loading

tisonkun commented Nov 1, 2021 •

edited

Loading

clovis1122 Nov 2, 2021

clovis1122 commented Nov 4, 2021

tisonkun commented Nov 4, 2021

clovis1122 commented Nov 5, 2021

tisonkun commented Nov 5, 2021

clovis1122 commented Nov 5, 2021 •

edited

Loading

tisonkun commented Nov 5, 2021

tisonkun Nov 6, 2021

tisonkun Nov 6, 2021

clovis1122 Nov 6, 2021

tisonkun commented Nov 6, 2021

tisonkun commented Nov 6, 2021 •

edited

Loading

clovis1122 commented Nov 6, 2021 •

edited

Loading

clovis1122 commented Nov 6, 2021

zimulala commented Nov 8, 2021

clovis1122 commented Nov 8, 2021 •

edited

Loading

clovis1122 commented Nov 9, 2021

xhebox commented Nov 15, 2021

tisonkun commented Nov 15, 2021 •

edited

Loading

clovis1122 commented Nov 19, 2021

clovis1122 commented Nov 21, 2021

ddl/table_test.go: migrate infra to testify #29288

ddl/table_test.go: migrate infra to testify #29288

Conversation

clovis1122 commented Oct 31, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Oct 31, 2021

ti-chi-bot commented Oct 31, 2021

clovis1122 commented Oct 31, 2021

tisonkun left a comment • edited Loading

Choose a reason for hiding this comment

clovis1122 commented Nov 1, 2021 • edited Loading

tisonkun commented Nov 1, 2021 • edited Loading

clovis1122 Nov 2, 2021

Choose a reason for hiding this comment

clovis1122 commented Nov 4, 2021

tisonkun commented Nov 4, 2021

clovis1122 commented Nov 5, 2021

tisonkun commented Nov 5, 2021

clovis1122 commented Nov 5, 2021 • edited Loading

tisonkun commented Nov 5, 2021

tisonkun Nov 6, 2021

Choose a reason for hiding this comment

tisonkun Nov 6, 2021

Choose a reason for hiding this comment

clovis1122 Nov 6, 2021

Choose a reason for hiding this comment

tisonkun commented Nov 6, 2021

tisonkun commented Nov 6, 2021 • edited Loading

clovis1122 commented Nov 6, 2021 • edited Loading

clovis1122 commented Nov 6, 2021

zimulala commented Nov 8, 2021

clovis1122 commented Nov 8, 2021 • edited Loading

clovis1122 commented Nov 9, 2021

xhebox commented Nov 15, 2021

tisonkun commented Nov 15, 2021 • edited Loading

clovis1122 commented Nov 19, 2021

clovis1122 commented Nov 21, 2021

clovis1122 commented Oct 31, 2021 •

edited

Loading

tisonkun left a comment •

edited

Loading

clovis1122 commented Nov 1, 2021 •

edited

Loading

tisonkun commented Nov 1, 2021 •

edited

Loading

clovis1122 commented Nov 5, 2021 •

edited

Loading

tisonkun commented Nov 6, 2021 •

edited

Loading

clovis1122 commented Nov 6, 2021 •

edited

Loading

clovis1122 commented Nov 8, 2021 •

edited

Loading

tisonkun commented Nov 15, 2021 •

edited

Loading