-
Notifications
You must be signed in to change notification settings - Fork 588
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
Add guide to designing strategies for good shrinking behaviour. #1790
Conversation
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.
Thanks for doing this. It generally looks very good!
One thing that is missing that I think is worth including as the most common problem is the use of length parameters: Things like integers(0, 10).flatmap(lambda n: st.lists(blah, min_size=n, max_size=n))
. They work in the sense that Hypothesis makes certain guarantees about being able to shrink them, but they result in shrinking being much slower than it otherwise would be.
Instead of choosing the midpoint, we draw a *random* point between our known | ||
endpoints, and repeat this until we find a satisfactory moment. This allows | ||
the shrinker to delete all the intermediate draws - and appear lucky enough | ||
to find the moment we were looking for on the first guess! |
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'd actually forgotten this trick. It's a good trick.
That's covered under "keep things local", though the only examples there are in the |
(misclick...) |
CC @DRMacIver - I've overhauled the last section on internal APIs Anything else you think needs to be handled before merging? |
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 haven't actually done more than a brief skim of a rereview, but based on the fact that a) I'm not likely to this week due to other considerations and b) This is already much better than the status quo of not having it, I figured we might as well merge. Thanks for writing it!
Closes #1705.
I'd appreciate lots of feedback on this as I certainly don't think it's perfect - but hope it's better than nothing.