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

statistics: migrate test-infra to testify for sample_test.go #28537

Merged

ti-chi-bot merged 10 commits into pingcap:master from feitian124:issue-28117

Oct 7, 2021

Contributor

feitian124 commented Oct 2, 2021

What problem does this PR solve?

Issue Number: close #28117

Release note

None

Member

ti-chi-bot commented Oct 2, 2021 •

edited

Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

pingyu
tisonkun

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 added release-note-none size/XXL labels

Contributor

sre-bot commented Oct 2, 2021

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what is changed

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

*: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

feitian124 changed the title ~~Issue 28117~~ statistics: migrate test-infra to testify for sample_test.go

Contributor Author

feitian124 commented Oct 2, 2021

/run-check_title

Contributor Author

feitian124 commented Oct 2, 2021

ti-chi-bot requested a review from tisonkun

October 2, 2021 15:30

pingyu reviewed

View reviewed changes

statistics/sample_serial_test.go Outdated

Comment on lines 221 to 223

    
              			if float64(cnt) < expFrequency/(1+delta) || float64(cnt) > expFrequency*(1+delta) {

              				require.Truef(t, false, "The frequency %v is exceed the Chernoff Bound", cnt)

              			}

Contributor

pingyu Oct 4, 2021 •

edited

Loading

Suggested change

      
            			if float64(cnt) < expFrequency/(1+delta) || float64(cnt) > expFrequency*(1+delta) {
          
            				require.Truef(t, false, "The frequency %v is exceed the Chernoff Bound", cnt)
          
            			}
          
            			require.Conditionf(t, func() bool { return float64(cnt) >= expFrequency/(1+delta) && float64(cnt) <= expFrequency*(1+delta) }, "The frequency %v is exceed the Chernoff Bound", cnt)

Contributor

tisonkun Oct 5, 2021

testify has a matcher named InDelta you can make use of.

Contributor Author

feitian124 Oct 5, 2021

thanks, updated.
as per https://stackoverflow.com/questions/45310329/difference-between-indelta-and-inepsilon

InDelta receives an absolute value and checks that the difference is less than that value.
InEpsilon receives a % of difference that would be acceptable.

used InEpsilon here.

statistics/sample_serial_test.go Outdated

Comment on lines 266 to 268

    
              			if float64(cnt) < expFrequency/(1+delta) || float64(cnt) > expFrequency*(1+delta) {

              				require.Truef(t, false, "the frequency %v is exceed the Chernoff Bound", cnt)

              			}

Contributor

pingyu Oct 4, 2021

Suggested change

      
            			if float64(cnt) < expFrequency/(1+delta) || float64(cnt) > expFrequency*(1+delta) {
          
            				require.Truef(t, false, "the frequency %v is exceed the Chernoff Bound", cnt)
          
            			}
          
            			require.Conditionf(t, func() bool { return float64(cnt) >= expFrequency/(1+delta) && float64(cnt) <= expFrequency*(1+delta) }, "the frequency %v is exceed the Chernoff Bound", cnt)

Contributor

tisonkun Oct 5, 2021

ditto above.

statistics/sample_serial_test.go Show resolved Hide resolved

statistics/sample_serial_test.go Show resolved Hide resolved

statistics/sample_serial_test.go Outdated Show resolved Hide resolved

statistics/sample_serial_test.go

    
              func SubTestCollectColumnStats(s *testSampleSuite) func(*testing.T) {

              	return func(t *testing.T) {

              		sc := mock.NewContext().GetSessionVars().StmtCtx

Contributor

pingyu Oct 4, 2021

Suggested change

      
            		sc := mock.NewContext().GetSessionVars().StmtCtx
          
            		t.Parallel()
          
            		sc := mock.NewContext().GetSessionVars().StmtCtx

It seems that all test cases can be paralleled.

Contributor Author

feitian124 Oct 5, 2021

after add t.Parallel() tests will fail, you can check, run mulit times.

Contributor

pingyu Oct 5, 2021

I found that the failure is caused by sharing and iterating testSampleSuite.rs in SubTestCollectColumnStats, SubTestMergeSampleCollector, and SubTestMergeSampleCollector.

I think we can keep these 3 tests in serial, but change the other 3 tests to be parallel, to make test cases as parallel as possible.

Contributor Author

feitian124 Oct 5, 2021

thanks, updated.

statistics/sample_serial_test.go Outdated Show resolved Hide resolved

feitian124 and others added 5 commits

October 5, 2021 21:00


          statistics: migrate test-infra to testify for sample_test.go

32660d0


          rename

931dc0b


          Update statistics/sample_serial_test.go

b44adde

Co-authored-by: Ping Yu <shuiyu.cn@gmail.com>


          Update statistics/sample_serial_test.go

6d19183

Co-authored-by: Ping Yu <shuiyu.cn@gmail.com>


          mv 3 tests to separate parallal test file as they do not depend on te…

3995d76

…stSampleSuite

feitian124 force-pushed the issue-28117 branch from 992d8af to 3995d76 Compare

October 5, 2021 13:01


          use require.InEpsilonf

96bf2ea

ti-chi-bot added size/XL and removed size/XXL labels

pingyu reviewed

View reviewed changes

statistics/sample_test.go Outdated

+              		}
+              		expFrequency := float64(sampleNum) * float64(loopCnt) / float64(rowNum)
+              		for _, cnt := range itemCnt {
+              			require.InEpsilonf(t, expFrequency, float64(cnt), 0.5, "The frequency %v is exceed the Chernoff Bound", cnt)

Contributor

pingyu Oct 5, 2021 •

edited

Loading

It seems that they are not completely equivalent.

InEpsilon(expFrequency, float64(cnt), 0.5) ==>
-0.5 < (float64(cnt) - expFrequency) / expFrequency < 0.5

float64(cnt) > expFrequency/(1+delta) && float64(cnt) < expFrequency*(1+delta) ==>
float64(cnt)/expFrequency > 1/(1+delta) && float64(cnt)/expFrequency < 1+delta ==>
float64(cnt)/expFrequency - 1 > -delta/(1+delta) && float64(cnt)/expFrequency - 1 < delta ==>
-delta/(1+delta) < float64(cnt)/expFrequency - 1 < delta ==>
-delta/(1+delta) < (float64(cnt) - expFrequency)/expFrequency < delta ==>
-0.333 < (float64(cnt) - expFrequency)/expFrequency < 0.5

Contributor Author

feitian124 Oct 5, 2021 •

edited

Loading

yes, you are right.
InEpsilon seems not suitable for this case, the original check actually not bad, reverted.


          Revert "use require.InEpsilonf"

b39f8d0

This reverts commit 96bf2ea.

ti-chi-bot added size/XXL and removed size/XL labels

pingyu approved these changes

View reviewed changes

Contributor

pingyu left a comment

LGTM

ti-chi-bot added the status/LGT1 label

tisonkun reviewed

View reviewed changes

statistics/sample_test.go Outdated

Comment on lines 29 to 33

+              func TestSample(t *testing.T) {
+              	t.Run("SubTestWeightedSampling", SubTestWeightedSampling())
+              	t.Run("SubTestDistributedWeightedSampling", SubTestDistributedWeightedSampling())
+              	t.Run("SubTestBuildStatsOnRowSample", SubTestBuildStatsOnRowSample())
               }

Contributor

tisonkun Oct 7, 2021

@feitian124 you don't have to wrap these tests in TestSample but

func TestWeightedSampling(t *testing.T) {
  t.Parallel()
  ...
}

and so on.

Golang ensures parallel tests only run along with each others.

Contributor Author

feitian124 Oct 7, 2021

thanks, updated.
i forgot to refactor after removing the suite dependency.

feitian124 added 2 commits

October 7, 2021 11:13


          do not use sub test

d9c4ff4


          Merge branch 'master' into issue-28117

472aac8

ti-chi-bot added size/XL and removed size/XXL labels

Contributor Author

feitian124 commented Oct 7, 2021 •

edited

Loading

ci failed due to #27889

[2021-10-07T03:23:32.292Z] FAIL: db_partition_test.go:2181: testIntegrationSuite5.TestTruncatePartitionAndDropTable
[2021-10-07T03:23:32.292Z] 
[2021-10-07T03:23:32.292Z] db_partition_test.go:2245:
[2021-10-07T03:23:32.292Z]     c.Assert(hasOldPartitionData, IsFalse, Commentf("take time %v", time.Since(startTime)))
[2021-10-07T03:23:32.292Z] ... obtained bool = true
[2021-10-07T03:23:32.292Z] ... take time 15.140463056s

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

Contributor Author

feitian124 commented Oct 7, 2021

/run-check_dev_2

tisonkun approved these changes

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels

Contributor

tisonkun commented Oct 7, 2021

cc @rebelice @mmyj

Could you help on merging this PR?

Member

mmyj commented Oct 7, 2021

/merge

Member

ti-chi-bot commented Oct 7, 2021

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

Commit hash: b39f8d0

ti-chi-bot added the status/can-merge label


          Merge branch 'master' into issue-28117

f877e3f

Member

ti-chi-bot commented Oct 7, 2021

@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 removed the status/can-merge label

Contributor

tisonkun commented Oct 7, 2021

@Mini256 why the bot cancel merge due to a merge commit?

@mmyj maybe you need to merge again...

Member

mmyj commented Oct 7, 2021

/merge

Member

ti-chi-bot commented Oct 7, 2021

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

Commit hash: f877e3f

ti-chi-bot added the status/can-merge label

ti-chi-bot merged commit 463cc34 into pingcap:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note-none size/XL status/can-merge status/LGT2