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

[Crypto] improve flaky random test #2509

Merged
merged 3 commits into from
Jun 2, 2022
Merged

[Crypto] improve flaky random test #2509

merged 3 commits into from
Jun 2, 2022

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented May 28, 2022

A rare flakiness has been seen in a statistical tests under crypto/random

  • use math rand and add logging for failing tests seeds
  • use higher sampling for a statistical test to avoid flakiness (the higher the sampling, the closer the result to theoretical distribution)

cc @durkmurder

@codecov-commenter
Copy link

Codecov Report

Merging #2509 (4999640) into master (d62a50b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2509   +/-   ##
=======================================
  Coverage   56.93%   56.93%           
=======================================
  Files         656      656           
  Lines       60328    60328           
=======================================
  Hits        34349    34349           
  Misses      23033    23033           
  Partials     2946     2946           
Flag Coverage Δ
unittests 56.93% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
consensus/hotstuff/eventloop/event_loop.go 71.73% <0.00%> (-1.45%) ⬇️
fvm/handler/contract.go 90.97% <0.00%> (+1.38%) ⬆️

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 d62a50b...4999640. Read the comment docs.

@gomisha
Copy link
Contributor

gomisha commented May 30, 2022

If I have the correct test (TestUint), this test has been consistently passing in the Flaky Test Monitor:

Grafana Dashboard

image

Where have you seen the flaky behavior?

@durkmurder
Copy link
Member

@gomisha this test started failed a few times on feature branch for Active Pacemaker. Here one if the evidences: https://github.com/onflow/flow-go/runs/6622170032?check_suite_focus=true

@tarakby
Copy link
Contributor Author

tarakby commented May 30, 2022

The test is probabilistic and there is always a low chance of failure, failures should happen very rarely. I'm also surprised it happens "a few times" on the feature branch.

Comment on lines -94 to +101
crand.Read(seed)
rand.Read(seed)
Copy link
Member

Choose a reason for hiding this comment

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

why replacing crypto/rand with math/rand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

math/rand produces deterministic randoms depending on the initializing seed, while crypto/rand doesn't allow that. This is useful when a test fails, and by logging the seed we can reproduce the fail.

Note that math/rand shouldn't be used outside of tests when secure entropy is needed.

@tarakby
Copy link
Contributor Author

tarakby commented Jun 1, 2022

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 2, 2022

Build succeeded:

@bors bors bot merged commit 2b3b2a4 into master Jun 2, 2022
@bors bors bot deleted the tarak/crypto-rand-tests branch June 2, 2022 00:16
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.

5 participants