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.

Sign up for GitHub

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

Jump to bottom

owner: migrate test-infra to testify #26770

Merged
merged 25 commits into from
Aug 9, 2021
Merged

owner: migrate test-infra to testify #26770

merged 25 commits into from
Aug 9, 2021

Conversation

hu00yan
Copy link
Contributor

@hu00yan hu00yan commented Jul 31, 2021 •
edited by tisonkun
Loading

Signed-off-by: hu00yan wuyan19981@gmail.com

What problem does this PR solve?

Issue Number: close #26743

What is changed and how it works?

What's Changed: Removed pingcap/check for owner/, moved to testify.

Release note

None

@hu00yan
migrate test-infra to testify for owner pkg
f473cf7 
Signed-off-by: hu00yan <wuyan19981@gmail.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 31, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • xhebox
  • 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 do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 31, 2021
@ti-chi-bot
Copy link
Member

Welcome @hu00yan!

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

@sre-bot
Copy link
Contributor

sre-bot commented Jul 31, 2021

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jul 31, 2021
tisonkun
tisonkun reviewed Jul 31, 2021
View reviewed changes
owner/manager_test.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 31, 2021
@sre-bot
Copy link
Contributor

sre-bot commented Jul 31, 2021

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@hu00yan
Copy link
Contributor Author

hu00yan commented Aug 2, 2021

@tisonkun Excuse me, what should I do to resolve conflicts? Change code as my PR and leave comment here?

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2021
@tisonkun tisonkun changed the title migrate test-infra to testify for owner pkg owner: migrate test-infra to testify Aug 2, 2021
@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 2, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2021
@tisonkun
Copy link
Contributor

tisonkun commented Aug 2, 2021

@hu00yan I resolve conflict for you. Yes, just keep the comment.

/cc @tiancaiamao @xhebox

I'd like to ask @tiancaiamao whether we have a bug issue for that comments 3761e4f - TODO in code is very, very, likely to be ignored and last for a long time.

xhebox
xhebox reviewed Aug 2, 2021
View reviewed changes
owner/manager_test.go Outdated Show resolved Hide resolved
owner/manager_test.go Outdated Show resolved Hide resolved
xhebox
xhebox reviewed Aug 3, 2021
View reviewed changes
Copy link
Contributor

@xhebox xhebox left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should remove the old TestT, there is already the new TestMain.

owner/manager_test.go Outdated Show resolved Hide resolved
owner/manager_test.go Outdated Show resolved Hide resolved
owner/manager_test.go Outdated Show resolved Hide resolved
owner/manager_test.go Outdated Show resolved Hide resolved
owner/manager_test.go Outdated Show resolved Hide resolved
owner/manager_test.go Outdated Show resolved Hide resolved
@hu00yan
Copy link
Contributor Author

hu00yan commented Aug 3, 2021

You should remove the old TestT, there is already the new TestMain.

Should I rename it to TestInitLogger?

@xhebox
Copy link
Contributor

xhebox commented Aug 3, 2021

You should remove the old TestT, there is already the new TestMain.

Should I rename it to TestInitLogger?

No, it is not a test for init logger. TestT is the entry for all tests with go-check framework. InitLogger is some thing that needs to be inited before all tests. Move it to TestMain and remove TestT.

@hu00yan
Copy link
Contributor Author

hu00yan commented Aug 3, 2021

@xhebox Thank you for so kindly respond. One more question, how to assert InitLogger is success ? TestMain's argument can't be modified.

@xhebox
Copy link
Contributor

xhebox commented Aug 3, 2021

@xhebox Thank you for so kindly respond. One more question, how to assert InitLogger is success ? TestMain's argument can't be modified.

You can use log.Fatal from github.com/pingcap/log. TestMain is the entry and it is reasonable to be special.

@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Aug 4, 2021
@hu00yan
Copy link
Contributor Author

hu00yan commented Aug 5, 2021

/run-all-tests

@tisonkun
Copy link
Contributor

tisonkun commented Aug 5, 2021

Let me take a closer look

@tisonkun
Copy link
Contributor

tisonkun commented Aug 5, 2021 •
edited
Loading

It seems owner tests hang forever. We may try to redo the changes.

@tisonkun
Copy link
Contributor

tisonkun commented Aug 5, 2021

I'll try to push follow ups, this patch cannot pass locally, please test locally before you push commits.

@zhouqiang-cl
Copy link
Contributor

It caused by #24679

@tisonkun
fix fail tests
9b698a4 
Signed-off-by: tison <wander4096@gmail.com>
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 5, 2021
@tisonkun
Copy link
Contributor

tisonkun commented Aug 5, 2021

@zhouqiang-cl not really. These tests modify failpoints and global variable, I'd like to run them in serial at this point.

@tisonkun
Copy link
Contributor

tisonkun commented Aug 5, 2021

/cc @xhebox @zimulala

PTAL

xhebox
xhebox reviewed Aug 5, 2021
View reviewed changes
owner/manager_test.go Outdated
@@ -11,12 +11,13 @@
// See the License for the specific language governing permissions and
// limitations under the License.

// +build !windows
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the original is better: if someone adds another test that is not related to NewClusterV3, it is ignored. It is not likely tests in the packages will change, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@tisonkun
replace conditional compile with skip
1ea7c19 
Signed-off-by: tison <wander4096@gmail.com>
xhebox
xhebox reviewed Aug 5, 2021
View reviewed changes
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tisonkun
Copy link
Contributor

tisonkun commented Aug 6, 2021

@xhebox @zimulala may you help on merging this PR?

@xhebox
Copy link
Contributor

xhebox commented Aug 9, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 94f5c7b

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

@hu00yan: 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 558ffd9 into pingcap:master Aug 9, 2021
@hu00yan hu00yan deleted the issue-26743 branch August 11, 2021 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xhebox xhebox xhebox approved these changes

@tisonkun tisonkun tisonkun left review comments

@zimulala zimulala Awaiting requested review from zimulala

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. sig/sql-infra SIG: SQL Infra 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

migrate test-infra to testify for owner pkg
7 participants
@hu00yan @ti-chi-bot @sre-bot @tisonkun @xhebox @zimulala @zhouqiang-cl