Skip to content

Fix unstable test cases in OccupiableBucketLeapArrayTest #687

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

Merged
merged 1 commit into from
Apr 20, 2019

Conversation

maoyiz
Copy link
Contributor

@maoyiz maoyiz commented Apr 19, 2019

Describe what this PR does / why we need it

Fix #685

Does this pull request fix one issue?

Describe how you did it

Pls see this blog for analysis: https://www.yuque.com/zhaixiaoxiang/cwrx8f/zzy18c

Describe how to verify it

Execute this test case many times to ensure it, because the BUG would happen by chance.

Special notes for reviews

@codecov-io
Copy link

codecov-io commented Apr 19, 2019

Codecov Report

Merging #687 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #687      +/-   ##
============================================
+ Coverage      40.8%   40.84%   +0.03%     
- Complexity     1215     1217       +2     
============================================
  Files           267      267              
  Lines          7911     7911              
  Branches       1072     1072              
============================================
+ Hits           3228     3231       +3     
+ Misses         4289     4288       -1     
+ Partials        394      392       -2
Impacted Files Coverage Δ Complexity Δ
...java/com/alibaba/csp/sentinel/log/LoggerUtils.java 66.66% <0%> (+19.99%) 4% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4acb907...8d729fd. Read the comment docs.

@sczyh30
Copy link
Member

sczyh30 commented Apr 19, 2019

Hi, thanks for contributing. Please make sure the email of your commits matches your GitHub email.


感谢贡献,请将 commit 对应的 email 调整成与 GitHub 的 email 相匹配。

@sczyh30 sczyh30 added area/test Issue or PR related to test cases to-review To review labels Apr 19, 2019
@maoyiz maoyiz force-pushed the issue#685 branch 2 times, most recently from f4d9227 to 222fe15 Compare April 19, 2019 03:15
@maoyiz
Copy link
Contributor Author

maoyiz commented Apr 20, 2019

@sczyh30 Is there anything else need improve ?

@sczyh30
Copy link
Member

sczyh30 commented Apr 20, 2019

@sczyh30 Is there anything else need improve ?

Since the AbstractTimeBasedTest will mock TimeUtil, for every test cases in this class, we may need to change long currentTime = TimeUtil.currentTimeMillis() to:

long currentTime = System.currentTimeMillis();
setCurrentMillis(currentTime);

Or the currentTime will be 0 by default.

@maoyiz
Copy link
Contributor Author

maoyiz commented Apr 20, 2019

@sczyh30 Thank you for your remind, I've modified.

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit cb93351 into alibaba:master Apr 20, 2019
@sczyh30
Copy link
Member

sczyh30 commented Apr 20, 2019

Thanks for contributing!

@sczyh30 sczyh30 removed the to-review To review label Apr 20, 2019
@maoyiz maoyiz deleted the issue#685 branch April 20, 2019 14:29
@sczyh30 sczyh30 changed the title Fix issue#685 Fix unstable test cases in OccupiableBucketLeapArrayTest Apr 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test Issue or PR related to test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The BUG of OccupiableBucketLeapArrayTest.testWindowAfterOneInterval()
3 participants