-
Notifications
You must be signed in to change notification settings - Fork 78
Deterministic input fuzzing #349
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
Conversation
|
We should probably merge #348 first. I based this PR off that one. I assumed Github would do something smart and only show the new changes w.r.t. the last one, but upon further thoughts, I don't know what I expected, really. |
problemtools/verifyproblem.py
Outdated
| return (desc, p.search, lambda text: p.sub(repl, text)) | ||
|
|
||
|
|
||
| rng = random.Random(42) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer not creating a variable rng for this, but instead do random.Random(42).choice(...) down where you use this variable.
If we create rng and multiple modifiers start using it, we lose determinism if they are reordered or removed (probably no big deal, but if the point of this PR is determinism... :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to be a little careful: doing random.Random(42).choice(...) will make the same choice 200 times. I found that lambdas are expressive enough to do what we want inline!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch. I didn't think about the context with how it's called.
|
Yeah, github really could do better in dealing with follow-up PR:s. I'll hold off reviewing this until we merge #347 (as I think it's very close to mergeable). |
3662710 to
5cfac90
Compare
|
Uhh... accidentally deleted this branch when cleaning up old branches because I thought it was merged. Whoops. |
|
Closed; see #351 instead |
As a follow-up to #347, make input fuzzing deterministic.