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

executor/cte_test.go: migrate test-infra to testify #27103

Merged
merged 21 commits into from
Aug 23, 2021
Merged

executor/cte_test.go: migrate test-infra to testify #27103

merged 21 commits into from
Aug 23, 2021

Conversation

unconsolable
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #27097

Problem Summary:

What is changed and how it works?

What's Changed:

  • Introduce func ExecToErr to testkit
  • Migrate test-infra to testify for executor/cte_test.go

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 11, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • mmyj
  • xhebox

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 release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 11, 2021
@github-actions github-actions bot added the sig/execution SIG execution label Aug 11, 2021
@unconsolable unconsolable marked this pull request as draft August 11, 2021 07:13
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2021
@unconsolable unconsolable marked this pull request as ready for review August 11, 2021 09:32
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2021
@unconsolable
Copy link
Contributor Author

/run-all-tests

@unconsolable
Copy link
Contributor Author

/cc @tisonkun PTAL

@ti-chi-bot
Copy link
Member

@unconsolable: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @tisonkun PTAL

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 kubernetes/test-infra repository.

@unconsolable
Copy link
Contributor Author

/run-check_release_note

@unconsolable
Copy link
Contributor Author

/build

@unconsolable
Copy link
Contributor Author

/uncc @tisonkun
/cc @mmyj @tiancaiamao
PTAL.

@ti-chi-bot ti-chi-bot removed the request for review from tisonkun August 16, 2021 10:52
@ti-chi-bot
Copy link
Member

@unconsolable: GitHub didn't allow me to request PR reviews from the following users: mmyj.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/uncc @tisonkun
/cc @mmyj @tiancaiamao
PTAL.

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 kubernetes/test-infra repository.

mmyj
mmyj reviewed Aug 17, 2021
View reviewed changes
executor/cte_test.go Outdated

"github.com/pingcap/tidb/domain"
"github.com/pingcap/tidb/kv"
"github.com/pingcap/tidb/session"
"github.com/pingcap/tidb/sessionctx"
"github.com/stretchr/testify/require"

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this empty line and format the imports.

executor/cte_test.go Outdated
cteTestSuite = SetUpSuite(t)
defer cteTestSuite.close()

t.Run("TestBasicCTE", BasicCTE)
Copy link
Member

@mmyj mmyj Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, we don't need the global variable cteTestSuite and all subtests can be paralleled, e.g.

func TestBasicCTE(t *testing.T) {
	t.Parallel()
	cteTestSuite := SetUpSuite(t)
	defer cteTestSuite.close()
	tk := testkit.NewTestKit(t, cteTestSuite.store)
	tk.MustExec("use test")
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #26712 , it is pointed out that

In the past, one test suite create one store/domain.
If after the refactor, each test case create one store/domain, the operation is heavier than before...
session.BootstrapSession is not cheap.

So it might be better to create the suite only once, but a global variable is introduced.
If you think it is unnecessary, I will remove it.

Copy link
Member

Choose a reason for hiding this comment

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

So it might be better to create the suite only once seems reasonable.
I think remove the global variable is better, we can create cteTestSuite once and pass it to subtest as an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Could you please take a look ?

@ti-chi-bot ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 20, 2021
@mmyj
Copy link
Member

mmyj commented Aug 20, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: a9e298f

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

mmyj commented Aug 20, 2021

/run-check_dev_2

@mmyj
Copy link
Member

mmyj commented Aug 20, 2021

/run-check_dev_2

@unconsolable
Copy link
Contributor Author

/hold

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2021
@zhouqiang-cl zhouqiang-cl removed the status/can-merge Indicates a PR has been approved by a committer. label Aug 20, 2021
@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 20, 2021
@zhouqiang-cl
Copy link
Contributor

/merge cancel

@ti-chi-bot
Copy link
Member

@zhouqiang-cl: /merge cancel is only allowed for the PR author and the committers in list.

In response to this:

/merge cancel

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.

@tisonkun
Copy link
Contributor

/run-check_dev_2

@unconsolable
Copy link
Contributor Author

/unhold

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2021
@unconsolable
Copy link
Contributor Author

/hold

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2021
@unconsolable
Copy link
Contributor Author

/run-unit-test

@unconsolable
Copy link
Contributor Author

/run-check_dev_2

@unconsolable
Copy link
Contributor Author

/unhold

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2021
@ti-chi-bot ti-chi-bot merged commit 4cc2423 into pingcap:master Aug 23, 2021
@unconsolable unconsolable deleted the issue-27097 branch August 23, 2021 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tisonkun tisonkun tisonkun left review comments

@mmyj mmyj mmyj approved these changes

@xhebox xhebox xhebox approved these changes

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. sig/execution SIG execution size/L Denotes a PR that changes 100-499 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 executor/cte_test.go
6 participants
@unconsolable @ti-chi-bot @mmyj @tisonkun @zhouqiang-cl @xhebox