statistics: migrate test-infra to testify for statistics_test.go #28558

feitian124 · 2021-10-07T15:35:49Z

What problem does this PR solve?

Issue Number: close #28123

notice: serial tests and their helper methods moved to statistics_serial_test.go

Release note

None

ti-chi-bot · 2021-10-07T15:35:51Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

feitian124 · 2021-10-07T16:12:37Z

ci failed due to

[2021-10-07T15:38:04.703Z] ./tools/check/check-tidy.sh
[2021-10-07T15:38:04.703Z] testSuite
[2021-10-07T15:38:04.703Z] ./tools/check/check_testSuite.sh
[2021-10-07T15:38:04.703Z] testStatisticsSuite in ./statistics is not enabled
[2021-10-07T15:38:04.703Z] make: *** [testSuite] Error 1

feitian124 · 2021-10-08T12:05:27Z

ci fails due to #27889

[2021-10-08T11:58:21.339Z] FAIL: db_partition_test.go:2181: testIntegrationSuite5.TestTruncatePartitionAndDropTable
[2021-10-08T11:58:21.339Z] 
[2021-10-08T11:58:21.339Z] db_partition_test.go:2282:
[2021-10-08T11:58:21.339Z]     c.Assert(hasOldPartitionData, IsFalse)
[2021-10-08T11:58:21.339Z] ... obtained bool = true

https://ci.pingcap.net/blue/organizations/jenkins/tidb_ghpr_check_2/detail/tidb_ghpr_check_2/37185/pipeline

feitian124 · 2021-10-08T12:05:48Z

/run-check_dev_2

feitian124 · 2021-10-08T13:14:17Z

/cc @tisonkun

feitian124 · 2021-10-08T13:19:49Z

notice the last commit add an useless line to workaround for tools/check/check_testSuite.sh
not sure what is a better way, rename the struct name? disable the check?

tisonkun · 2021-10-12T05:37:48Z

notice the last commit add an useless line to workaround for tools/check/check_testSuite.sh not sure what is a better way, rename the struct name? disable the check?

Rename the suite could work, let me try to push a follow up for you to resolve conflict as well as workaround this check

tisonkun · 2021-10-12T05:41:53Z

@feitian124 but you should keep var _ = Suite(&testStatisticsSuite{}) for now as there is other tests in cmsketch_test.go & histogram_test.go rely on it.

tisonkun

Could you elaborate how you determinate serial tests? That is, how you decide a test should be run in serial or in parallel.

feitian124 · 2021-10-12T12:20:27Z

Could you elaborate how you determinate serial tests? That is, how you decide a test should be run in serial or in parallel.

check the first commit, you can find there are 2 suites, one is testSerialIntegrationSuite

tisonkun · 2021-10-12T12:30:22Z

@feitian124 I don't find one. Do you mix it up with #28693 ? I find all tests under testStatisticsSuite.

feitian124 · 2021-10-12T12:38:02Z

statistics/main_test.go

+var _ = check.Suite(&testStatisticsSuite{})
+
+func TestT(t *testing.T) {
+	config.UpdateGlobal(func(conf *config.Config) {


config.UpdateGlobal part i had added in TestStatistics, please check.

add here is package level, add in TestStatistics is more adhere to origin suite scope.

but in my memory some other tests also has config.UpdateGlobal, not sure if package level is better.

and could you explain why it need config.UpdateGlobal, not quit understand.

It is not package level, but TestT level. But since we only had one TestT, it becomes global/package level.

This function itself it to avoid race, but it does not always work.

thanks, get it, TestingT is a special function in pingcap/check package, which runs all test suites registered with the Suite function.

as almost all original tests were suite based, i think make it active for all tests in this package is reasonable.
i will mv it to TestMain.

tisonkun · 2021-10-15T02:19:43Z

Thanks for your updating @feitian124 ! I'm reviewing now.

tisonkun

LGTM.

ping @winoros please take a look.

tisonkun · 2021-10-15T06:12:46Z

/cc @rebelice

Could you please take a look at this PR?

feitian124 · 2021-10-18T16:32:36Z

/cc @winoros @xhebox @tiancaiamao

sorry for disturb as seems they are busy, could you help review?

xhebox

Rest LGTM

xhebox · 2021-10-19T04:53:07Z

statistics/main_test.go

+var _ = check.Suite(&testStatisticsSuite{})
+
+func TestT(t *testing.T) {
+	config.UpdateGlobal(func(conf *config.Config) {


It is not package level, but TestT level. But since we only had one TestT, it becomes global/package level.

This function itself it to avoid race, but it does not always work.

feitian124 · 2021-10-19T16:04:52Z

statistics/main_test.go

@@ -30,8 +29,6 @@ import (
 	"go.uber.org/goleak"
 )

-var _ = check.Suite(&testStatisticsSuite{})


this line is here since that time other tests like cmsketch_test.go did not migrate so need it.

now #28552 is merged so ok to delete.

but after delete, will lead to suite not used error, so i renamed the old struct name from testStatisticsSuite to testStatisticsSamples to avoid it.

feitian124 · 2021-10-19T16:07:24Z

updated as your suggestion, please review agian, thanks.

@xhebox

xhebox · 2021-10-20T10:46:48Z

/merge

ti-chi-bot · 2021-10-20T10:46:51Z

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

Commit hash: 7402844

ti-chi-bot · 2021-10-20T10:47:00Z

@feitian124: 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 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 7, 2021

feitian124 added 11 commits October 8, 2021 19:50

tmp

d13be9c

check

2293032

fix 2 type issue

551738e

sub test

fa740cd

add sub test

e92b6ab

3 add parallal

9672bed

1 add parallal

f41c3ad

no error

fb0c546

separate serial

9485158

always use require

c717d26

// workaround for tools/check/check_testSuite.sh

1947b4f

feitian124 force-pushed the issue-28123 branch from 2aa83ad to 1947b4f Compare October 8, 2021 11:51

ti-chi-bot requested a review from tisonkun October 8, 2021 13:14

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

Merge branch 'master' into issue-28123

3be386e

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

tisonkun reviewed Oct 12, 2021

View reviewed changes

feitian124 commented Oct 12, 2021

View reviewed changes

ti-chi-bot requested review from tisonkun and winoros October 13, 2021 16:28

tisonkun approved these changes Oct 15, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 15, 2021

ti-chi-bot requested a review from rebelice October 15, 2021 06:12

feitian124 added 2 commits October 18, 2021 22:35

Merge branch 'master' into issue-28123

ac93c63

Merge branch 'master' into issue-28123

efbba37

ti-chi-bot requested review from tiancaiamao and xhebox October 18, 2021 16:32

xhebox reviewed Oct 19, 2021

View reviewed changes

feitian124 added 5 commits October 19, 2021 23:30

mv config.UpdateGlobal to TestMain

c8e7ddd

Merge branch 'master' into issue-28123

678d01b

rm useless codes

ebed0ca

rename struct name to avoid suite name check error

1c9f074

rename function name as well

7402844

feitian124 commented Oct 19, 2021

View reviewed changes

xhebox approved these changes Oct 20, 2021

View reviewed changes

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 Oct 20, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 20, 2021

Merge branch 'master' into issue-28123

0497616

ti-chi-bot merged commit f520dea into pingcap:master Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: migrate test-infra to testify for statistics_test.go #28558

statistics: migrate test-infra to testify for statistics_test.go #28558

feitian124 commented Oct 7, 2021

ti-chi-bot commented Oct 7, 2021 •

edited

Loading

feitian124 commented Oct 7, 2021

feitian124 commented Oct 8, 2021

feitian124 commented Oct 8, 2021

feitian124 commented Oct 8, 2021

feitian124 commented Oct 8, 2021

tisonkun commented Oct 12, 2021

tisonkun commented Oct 12, 2021

tisonkun left a comment •

edited

Loading

feitian124 commented Oct 12, 2021

tisonkun commented Oct 12, 2021

feitian124 Oct 12, 2021

xhebox Oct 19, 2021

feitian124 Oct 19, 2021

tisonkun commented Oct 15, 2021

tisonkun left a comment

tisonkun commented Oct 15, 2021

feitian124 commented Oct 18, 2021 •

edited

Loading

xhebox left a comment

xhebox Oct 19, 2021

feitian124 Oct 19, 2021 •

edited

Loading

feitian124 commented Oct 19, 2021

xhebox commented Oct 20, 2021

ti-chi-bot commented Oct 20, 2021

ti-chi-bot commented Oct 20, 2021

statistics: migrate test-infra to testify for statistics_test.go #28558

statistics: migrate test-infra to testify for statistics_test.go #28558

Conversation

feitian124 commented Oct 7, 2021

What problem does this PR solve?

Release note

ti-chi-bot commented Oct 7, 2021 • edited Loading

feitian124 commented Oct 7, 2021

feitian124 commented Oct 8, 2021

feitian124 commented Oct 8, 2021

feitian124 commented Oct 8, 2021

feitian124 commented Oct 8, 2021

tisonkun commented Oct 12, 2021

tisonkun commented Oct 12, 2021

tisonkun left a comment • edited Loading

Choose a reason for hiding this comment

feitian124 commented Oct 12, 2021

tisonkun commented Oct 12, 2021

feitian124 Oct 12, 2021

Choose a reason for hiding this comment

xhebox Oct 19, 2021

Choose a reason for hiding this comment

feitian124 Oct 19, 2021

Choose a reason for hiding this comment

tisonkun commented Oct 15, 2021

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun commented Oct 15, 2021

feitian124 commented Oct 18, 2021 • edited Loading

xhebox left a comment

Choose a reason for hiding this comment

xhebox Oct 19, 2021

Choose a reason for hiding this comment

feitian124 Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

feitian124 commented Oct 19, 2021

xhebox commented Oct 20, 2021

ti-chi-bot commented Oct 20, 2021

ti-chi-bot commented Oct 20, 2021

ti-chi-bot commented Oct 7, 2021 •

edited

Loading

tisonkun left a comment •

edited

Loading

feitian124 commented Oct 18, 2021 •

edited

Loading

feitian124 Oct 19, 2021 •

edited

Loading