Don't use Math.random in tests for reproducability #36241
Conversation
Currently we use Math.random() in a few places in the tests which makes these tests not reproducable with the random seed mechanism that comes with ESTestCase. The change removes those instances.
cbuescher
added
>test
|
Pinging @elastic/es-search |
jtibshirani
left a comment
jtibshirani
left a comment
There was a problem hiding this comment.
It looks good to me, just left one comment.
| value = Math.random() * 10; | ||
| array[i] = value + ((randomFreq(0.5) ? 1 : -1) * Math.random() * tolerance); | ||
| if (frequently()) { | ||
| value = randomIntBetween(0, 10); |
There was a problem hiding this comment.
To match the old behavior I think this should be randomDoubleBetween(0, 10, true).
There was a problem hiding this comment.
Thanks, I missed this being a double. Upon further inspection I now think it should be randomDoubleBetween(0, 9, true) but including the edge values of the interval shouldn't really matter here.
|
run the gradle build tests 1 and the gradle build tests 2 |
|
@elasticmachine run the gradle build tests 2 |
|
Should it be a forbidden API for tests? |
|
@jpountz good idea, but I didn't go into how to add this solely for the tests, not sure if this is possible. I will check and open a separate PR if it is easily done. There are some usages of |
|
You can add forbidden APIs for tests only by adding them to |
| value = Math.random() * 10; | ||
| array[i] = value + ((randomFreq(0.5) ? 1 : -1) * Math.random() * tolerance); | ||
| if (frequently()) { | ||
| value = randomDoubleBetween(0, 9, true); |
There was a problem hiding this comment.
wasn't the test generating doubles between 0 and 10 before? (it might not matter)
|
@elasticmachine run the gradle build tests 2 |
1 similar comment
|
@elasticmachine run the gradle build tests 2 |
Currently we use Math.random() in a few places in the tests which makes these tests not reproducable with the random seed mechanism that comes with ESTestCase. The change removes those instances.
* master: (133 commits) SNAPSHOT: Increase Timeout to Stabilize Test (elastic#36294) Fix get certificates HLRC API (elastic#36198) Avoid shutting down the only master (elastic#36272) Fix typo in comment Fix total hits serialization of the search response (elastic#36290) Fix FullClusterRestartIT#testRollupIDSchemeAfterRestart Mute FullClusterRestartIT#testRollupIDSchemeAfterRestart as we await a fix. [Docs] Add Profile API limitations (elastic#36252) Make sure test don't use Math.random for reproducability (elastic#36241) Fix compilation ingest: support default pipeline through an alias (elastic#36231) Zen2: Rename tombstones to exclusions (elastic#36226) [Zen2] Hide not recovered state (elastic#36224) Test: mute testDataNodeRestartWithBusyMasterDuringSnapshot Test: mute testSnapshotAndRestoreWithNested Revert "Test: mute failing mtermvector rest test" Test: mute failing mtermvector rest test add version 6.5.3 (elastic#36268) Make hits.total an object in the search response (elastic#35849) [DOCS] Fixes broken link in execute watch ...
5 participants
Currently we use Math.random() in a few places in the tests which makes these
tests not reproducable with the random seed mechanism that comes with
ESTestCase. The change removes those instances.