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

ddl/failtest: migrate test-infra to testify #27747

Merged

Conversation

karuppiah7890
Copy link
Contributor

@karuppiah7890 karuppiah7890 commented Sep 1, 2021

What problem does this PR solve?

Issue Number: close #27669

Problem Summary:

What is changed and how it works?

What's Changed: Migrated tests to use testify package instead of github.com/pingcap/check package for assertions and any test utils and matchers that github.com/pingcap/check provided

There are some small changes in ddl/ddl_test too

Note:

  • When I ran make gotest I noticed that two tests are timing out. I need help with figuring out why that's the case and if it's happening in others machines and in CI too or just my machine. The two tests are - TestT/TestAddIndexFailed and TestT/TestAddIndexWorkerNum. Log -
Logs (click to expand)
tidb $ make gotest
Running in native mode.
timeout-check
grep 'PASS:' gotest.log | go run tools/check/check-timeout.go || { $(find $PWD/ -type d | grep -vE "(\.git|tools)" | xargs tools/bin/failpoint-ctl disable); exit 1; }
The following test cases take too long to finish:
    --- PASS: TestT/TestAddIndexFailed (71.08s)
    --- PASS: TestT/TestAddIndexWorkerNum (55.20s)
PASS: builtin_time_vec_generated_test.go:6727: testVectorizeSuite1.TestVectorizedBuiltinTimeEvalOneVecGenerated	8.240s
PASS: builtin_time_vec_test.go:575: testVectorizeSuite2.TestVectorizedBuiltinTimeEvalOneVec	9.063s
--- PASS: TestClusterConfigInfoschema (6.09s)
--- PASS: TestCTEPreviewAndReport (6.78s)
exit status 252
make: *** [gotest] Error 1
tidb $ 
  • I put lot of commits. It will be easier to review I guess. But I can squash it too once all review comments are done. Either squash it to one commit or a few commits but currently there are 24 commits!
  • I added all the tests to run in a sequential manner, assuming SerialSuites of github.com/pingcap/check helps to run tests in series. Also, when I tried it in parallel with t.Parallel() there were errors which I didn't understand
  • I added some TODOs with some questions, please do check them
  • What is the purpose of this piece of code? -
colsStr := ""
idxsStr := ""
for _, col := range cols {
	colsStr += col.Name.L + " "
}
for _, idx := range tbl.Meta().Indices {
	idxsStr += idx.Name.L + " "
}

colsStr and idxsStr don't seem to be used anywhere else, so I'm curious if it's dead code or if some test code got removed by someone by mistake

Tests

  • Unit test

Release note

None

Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
…ub.com/pingcap/check package import

Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
… of github.com/pingcap/tidb/testkit package

Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
…tkit

Signed-off-by: Karuppiah Natarajan <karuppiah7890@users.noreply.github.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 1, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • tangenta
  • zimulala

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 ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 1, 2021
@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Sep 1, 2021
@ti-chi-bot ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 1, 2021
@ti-chi-bot
Copy link
Member

Welcome @karuppiah7890!

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. 😃

@karuppiah7890
Copy link
Contributor Author

cc @tisonkun could you please review or tag someone / some folks you know who can help with reviewing this?

@tisonkun
Copy link
Contributor

tisonkun commented Sep 7, 2021

Looks like the branch is out of date. Should I update it? With rebase

TiDB uses a "merge master" strategy which force keep up-to-date before PR merging and thus running tests with all commits.

However, it is not a strong requirement to keep up-to-date during reviewing since almost changes don't have conflicts. We sometimes click "Update branch" to keep up-to-date but after one merge commit, rebase is hard to perform.

Short answer, you shouldn't update it, especially mix up rebase and merge.

@karuppiah7890
Copy link
Contributor Author

@karuppiah7890 it's the PR author.

I think we need some sort of Co-authored-by: and put my name in co-author but your name should be in the commit as you are working on it. Or let's create a separate PR

@karuppiah7890
Copy link
Contributor Author

Short answer, you shouldn't update it, especially mix up rebase and merge.

Cool !

@tisonkun
Copy link
Contributor

tisonkun commented Sep 7, 2021

I think we need some sort of Co-authored-by: and put my name in co-author but your name should be in the commit as you are working on it. Or let's create a separate PR

So far, the commit message is handled by @ti-chi-bot . We're discussing to change the style https://internals.tidb.io/t/topic/361 . Maybe implement later - I don't have a concrete schedule, though.

@karuppiah7890
Copy link
Contributor Author

Cool ! But please ensure your name is on the commit as you are working on it. It would be nice to have both names with mine as co-author

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun requested a review from tangenta September 7, 2021 15:46
@tisonkun
Copy link
Contributor

tisonkun commented Sep 7, 2021

Cool ! But please ensure your name is on the commit as you are working on it. It would be nice to have both names with mine as co-author

I hope so. However, @ti-chi-bot hijacks the commit message. I created an issue ti-community-infra/tichi#733 to investigate whether we can properly respect multiple authors contribution.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 7, 2021
@tangenta
Copy link
Contributor

tangenta commented Sep 7, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 002b85e

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 7, 2021
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Sep 8, 2021
@tisonkun
Copy link
Contributor

tisonkun commented Sep 8, 2021

@tangenta @zimulala sorry I miss a gofmt issue. Please trigger another /merge phase.

@tisonkun
Copy link
Contributor

tisonkun commented Sep 8, 2021

/run-check_dev_2

@tangenta
Copy link
Contributor

tangenta commented Sep 8, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 6dd22c7

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 8, 2021
@ti-chi-bot
Copy link
Member

@karuppiah7890: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot ti-chi-bot merged commit cc04fdc into pingcap:master Sep 8, 2021
@karuppiah7890 karuppiah7890 deleted the migrate-ddl/failtest-to-testify branch September 17, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

migrate test-infra to testify for ddl/failtest pkg
6 participants