Change random seed for txn stress tests on each run #5004
Conversation
There was a problem hiding this comment.
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
I generally hope stress tests are all moved from unit tests to a designated test suite, and run daily, not for every commit. We usually used fixed random seed in unit tests so that the results are more deterministic. Increasing randomness in unit tests may be counter productive. Think about someone else sends a PR for something totally unrelated, and see this test failure. They will not go ahead and debug the transaction stress test, and the randomness will only create problem rather than solving any problem. If it fails occasionally in contbuild, the oncall needs to investigate it but likely not going to find the bug. So it is not very efficient. If you really want you can introduce a gflag to turn on more randomness mode, so that you can run it manually if you want more coverage.
I'm approving it as you are already using non-deterministic random seed, so it won't be worse.
|
@maysamyabandeh has updated the pull request. Re-import the pull request |
|
The travis failure is for a flaky test: RateLimiterTest.Rate |
There was a problem hiding this comment.
@maysamyabandeh is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Currently the transaction stress tests use thread id as the seed. Since the thread ids are likely to be the same across multiple runs, the seed is thus going to be the same. The patch includes time in calculating the seed to help covering a very different part of state space in each run of the stress tests. To be able to reproduce the bug in case the stress tests failed, it also prints out the time that was used to calculate the seed value.