Use deterministic latency in tests to avid intermittent failures #269
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #269 +/- ##
==========================================
+ Coverage 71.99% 72.00% +0.01%
==========================================
Files 32 32
Lines 2389 2390 +1
==========================================
+ Hits 1720 1721 +1
Misses 537 537
Partials 132 132
|
|
Fuzz test failed on commit 5fc1170. To troubleshoot locally, download the seed corpus using GitHub CLI by running: gh run download 9195742636 -n testdataAleternatively, download directly from here. |
masih
force-pushed
the
masih/fix-intermittent-test-failures
branch
from
5fc1170 to
6af1dc2
Compare
Stebalien
approved these changes
May 22, 2024
Member
Stebalien
left a comment
Stebalien
left a comment
There was a problem hiding this comment.
LGTM although I'd consider breaking this into two PRs:
- One to change from a latency model to latency modeler.
- One to change the tests (parallel, naming, etc.).
masih
force-pushed
the
masih/fix-intermittent-test-failures
branch
from
6af1dc2 to
c683815
Compare
Member
Author
Sure. Test refactors and extra parallelism increase is now removed from this one. Next one coming up shortly. |
The intermittent failures always occur for async tests, in form of going beyond the maximum number of rounds allowed in tests (mostly 10). The number of rounds is directly impacted by the order of message delivery to each node, dictated by the latency model. The Async test options use a latency model that is instantiated when the simulation _options_ are instantiated, not when the simulation itself is instantiated and run. This results in the reuse of the same latency model object across multiple tests. In go tests may be run in parallel, but the same test never runs in parallel with itself when repeated multiple times via `-count=n` flag. When test are run in parallel using the same latency object across tests becomes the root cause of indeterministic behaviour, where at times a test case can require more rounds than allowed to complete. The changes here introduce a latency model instantiator, called `latency.Modeler` which encapsulates the responsibility of constructing a latency model object used by a simulation. The simulation is then adopted to take modelers as option, not already instantiated models. This change dramatically reduces the scope of error that was previously occurring where latency models with unclean RNG state were being used by multiple test and assures that the order of message delivery in tests is deterministic regardless of the test execution parallelism. Fixes #262
masih
force-pushed
the
masih/fix-intermittent-test-failures
branch
from
c683815 to
a18a596
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The intermittent failures always occur for async tests, in form of going beyond the maximum number of rounds allowed in tests (mostly 10). The number of rounds is directly impacted by the order of message delivery to each node, dictated by the latency model.
The Async test options use a latency model that is instantiated when the simulation options are instantiated, not when the simulation itself is instantiated and run. This results in the reuse of the same latency model object across multiple tests.
In go tests may be run in parallel, but the same test never runs in parallel with itself when repeated multiple times via
-count=nflag. When test are run in parallel using the same latency object across tests becomes the root cause of indeterministic behaviour, where at times a test case can require more rounds than allowed to complete.The changes here introduce a latency model instantiator, called
latency.Modelerwhich encapsulates the responsibility of constructing a latency model object used by a simulation. The simulation is then adopted to take modelers as option, not already instantiated models. This change dramatically reduces the scope of error that was previously occurring where latency models with unclean RNG state were being used by multiple test and assures that the order of message delivery in tests is deterministic regardless of the test execution parallelism.Additionally, maximum test parallelism is enabled across all table tests and fuzz tests to reduce the total execution runtime.
Fixes #262