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

tests: Fix flaky TestAccountSelected test #5788

Merged
merged 4 commits into from Oct 19, 2023

Conversation

ohill
Copy link
Contributor

@ohill ohill commented Oct 18, 2023

Summary

Go 1.20 introduced a change to math/rand that seeds the global RNG with a random value. This test TestAccountSelected was previously relying on the fixed-seed global RNG for deterministic behavior. There was already a non-global random generator used for this test in testingEnvMoreKeys, but it was not used for two usages of the global RNG to so different values would be produced for multiple iterations calling testingEnv.

Running with GODEBUG=randautoseed=0 and extra logging revealed the difference between the old and new behavior.

Test Plan

Existing tests should pass, and the number of leaders chosen in this test is unchanged, the same as when GODEBUG=randautoseed=0 is set.

Hardened these tests further by asserting the leader & committee sizes checked by these tests match the deterministic values used since #716 and #1094 (the last consensus-gated change to this package). The values being asserted came from extra logging added to checkouts of ec4d9b5 and 566855e on Ubuntu 18.04 with Go 1.12.17, and boost 1.65.1. These values were reproduced in this PR after adding a rand.New(rand.NewSource(1)) to generate the sequence of seeds, matching the previous behavior before Go 1.20.

@ohill ohill added the Bug-Fix label Oct 18, 2023
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #5788 (2a88dbd) into master (d3df476) will increase coverage by 0.01%.
Report is 3 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5788      +/-   ##
==========================================
+ Coverage   55.53%   55.55%   +0.01%     
==========================================
  Files         473      473              
  Lines       66815    66815              
==========================================
+ Hits        37105    37117      +12     
+ Misses      27192    27188       -4     
+ Partials     2518     2510       -8     

see 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

algorandskiy
algorandskiy previously approved these changes Oct 18, 2023
jasonpaulos
jasonpaulos previously approved these changes Oct 18, 2023
Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Makes sense!

@ohill ohill dismissed stale reviews from jasonpaulos and algorandskiy via 25cfc1f October 19, 2023 01:04
@ohill
Copy link
Contributor Author

ohill commented Oct 19, 2023

Hardened these tests further by asserting the leader & committee sizes checked by these tests match the deterministic values used since #716 and #1094 (the last consensus-gated change to this package). The values being asserted came from extra logging added to checkouts of ec4d9b5 and 566855e on Ubuntu 18.04 with Go 1.12.17, and boost 1.65.1 (running each test individually to avoid reuse of the global RNG between tests). These values were reproduced in this PR after adding a rand.New(rand.NewSource(1)) to generate the sequence of seeds, matching the previous behavior before Go 1.20.

Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

LGTM

@algorandskiy algorandskiy merged commit 51db0fa into algorand:master Oct 19, 2023
18 checks passed
@ohill ohill deleted the deterministic-TestAccountSelected branch October 19, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants