Make SHUTTLE_RANDOM_SEED always override.
#174
Conversation
This file contains 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
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.
Does two things:
SHUTTLE_RANDOM_SEEDis set, then that seed will be used.SHUTTLE_RANDOM_SEED(vs just RandomScheduler previously)Regarding 2: We added the output seed and replay on seed in #161. @jorajeev originally wanted PCT as well, but this was for some reason left as is. I don't see why PCT shouldn't honor
SHUTTLE_RANDOM_SEED.Regarding 1: Currently it is kind of annoying and also just a bad user experience (because we say "set this env var", and then, depending on how the test is setup, might just ignore it) that the test has to use
check_randomforSHUTTLE_RANDOM_SEEDto be honored. This is made worse by the fact that we mostly don't usecheck_random, and also usually recommend creating runners/schedulers explicitly (just because this is more versatile).I believe the original arguments for putting it in
check_randomvs inside the constructor was that 1: one can always build a new constructor which honors the environment variable and 2: if you create a scheduler throughnew_from_seedthen it seems fair to expect that you'll get a scheduler initialized with this seed. My argument against these points is thatSHUTTLE_RANDOM_SEEDdoesn't get set by accident, and setting it and not getting the seed overridden is both worse and significantly more likely to occur. It also makes sense for thenew_from_seedto be overridden because what is output is the seed used for the last execution, but what is input is the seed for the first execution. Also,new_from_seedmostly exists such that the seed can be generated by something like proptest, in which case you for sure will want to override the seed if a failing one is found.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.