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

Fix intermittent validator_exit test failure #23594

Merged
merged 5 commits into from
Mar 25, 2022

Conversation

HaoranYi
Copy link
Contributor

@HaoranYi HaoranYi commented Mar 10, 2022

Problem

Due to shared resources between validator_exit and validator_parallel_exit, when they are executed in parallel by the rust test executor, these tests hangs.

Summary of Changes

Use one test function to run both tests.

Fixes #

@HaoranYi HaoranYi force-pushed the serial_validator_exit_tests branch from 67ed13d to bcf062c Compare March 24, 2022 20:24
@HaoranYi HaoranYi changed the title Running validator_exit_test sequentially Fix intermittent validator_exit test failure Mar 24, 2022
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #23594 (80db282) into master (2da4e3e) will decrease coverage by 0.1%.
The diff coverage is 75.6%.

❗ Current head 80db282 differs from pull request most recent head 79524ba. Consider uploading reports for the commit 79524ba to get more accurate results

@@            Coverage Diff            @@
##           master   #23594     +/-   ##
=========================================
- Coverage    81.8%    81.6%   -0.2%     
=========================================
  Files         581      583      +2     
  Lines      158312   159173    +861     
=========================================
+ Hits       129518   130044    +526     
- Misses      28794    29129    +335     

@t-nelson
Copy link
Contributor

Can you remove (or move to a separate commit) the code shuffle in 9f56e89? It hides the logical changes

@HaoranYi
Copy link
Contributor Author

Can you remove (or move to a separate commit) the code shuffle in 9f56e89? It hides the logical changes

Yes. good idea.

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm. just a curiosity to entertain

core/src/validator.rs Show resolved Hide resolved
@HaoranYi HaoranYi merged commit 01af40d into solana-labs:master Mar 25, 2022
@jstarry
Copy link
Member

jstarry commented Apr 1, 2022

Is it true that these two tests actually share resources? From my understanding they shouldn't be. I think we should revert this change in favor of the real fix (#24007 + #24036)

@HaoranYi
Copy link
Contributor Author

HaoranYi commented Apr 1, 2022

@jstarry Sure. Good point. I will create another pull request to revert this change. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants