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

test: optimize dll test #13497

Merged
merged 2 commits into from
Nov 18, 2019
Merged

test: optimize dll test #13497

merged 2 commits into from
Nov 18, 2019

Conversation

glorv
Copy link
Contributor

@glorv glorv commented Nov 15, 2019

What problem does this PR solve?

make ddl unit test running faster

What is changed and how it works?

One loop in the hot path cost almost 10s to execute, and several test cases depends on this function. The loop run 100 times to randomly select a range of 3 to check whether the result is right, this is unneccssary. We can reduce it to 20 times with check the lower/upper boundary and some random inner case is enough to make sure the logic is right.

Here is the result:
Before change:

>  go test -v -vet=off -p 10 -race github.com/pingcap/tidb/ddl
ok  	ok  	github.com/pingcap/tidb/ddl	200.443s

After change:

> go test -v -vet=off -p 10 -race github.com/pingcap/tidb/ddl
ok  	github.com/pingcap/tidb/ddl	118.154s

Check List

Tests

  • unit tests

Code changes

Side effects

Related changes

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

Merging #13497 into master will decrease coverage by 0.0108%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #13497        +/-   ##
================================================
- Coverage   80.2375%   80.2267%   -0.0109%     
================================================
  Files           472        472                
  Lines        114328     114245        -83     
================================================
- Hits          91734      91655        -79     
+ Misses        15430      15421         -9     
- Partials       7164       7169         +5

@glorv
Copy link
Contributor Author

glorv commented Nov 15, 2019

@zimulala PTAL

@glorv
Copy link
Contributor Author

glorv commented Nov 15, 2019

/run-unit-tests

@glorv glorv force-pushed the ddl-test branch 2 times, most recently from c83a6c5 to 4394a93 Compare November 15, 2019 10:15
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added component/test status/LGT1 Indicates that a PR has LGTM 1. labels Nov 15, 2019
Copy link
Member

@sykp241095 sykp241095 left a comment

Choose a reason for hiding this comment

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

LGTM

@sykp241095 sykp241095 requested a review from bb7133 November 18, 2019 02:26
@zz-jason zz-jason added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 18, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 18, 2019

Your auto merge job has been accepted, waiting for 13511

@codecov-io
Copy link

codecov-io commented Nov 18, 2019

Codecov Report

Merging #13497 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13497   +/-   ##
===========================================
  Coverage   80.5384%   80.5384%           
===========================================
  Files           472        472           
  Lines        115895     115895           
===========================================
  Hits          93340      93340           
  Misses        15362      15362           
  Partials       7193       7193

@sre-bot
Copy link
Contributor

sre-bot commented Nov 18, 2019

/run-all-tests

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 18, 2019
@sykp241095 sykp241095 merged commit aa8f6df into pingcap:master Nov 18, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/test status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants