-
Notifications
You must be signed in to change notification settings - Fork 155
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
Make tests pass and improve shrinking for prop_flat_map #269
base: master
Are you sure you want to change the base?
Conversation
The tests didn't pass, and fixing them reliably seems impossible.
The previous shrinker always shrunk the inner strategy first. This seems to be the wrong thing to do in almost all non-trivial situations (e.g., generate a vector length and then a pair of vectors of this length). The new shrinker shrinks the outer strategy first (e.g., tries to minimize the vector length first and then minimizes the entries in the vectors). Given the current implementation of shrinking via simplify/complicate, I had a hard time doing this without cloning the inner strategy before simplifying it. So there's a breaking change here because prop_flat_map now requires the closure to produce a strategy that implements Clone. This could have been avoided by a change in how simplify/complicate operate, but this has the potential of breaking much more code than adding the requirement that the inner strategy is clonable.
### Breaking Changes | ||
|
||
- `prop_flat_map` can only be used with closures that produce a strategy that | ||
implements `Copy`. |
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'm concerned about this, I often use Strategies over String, sometimes even Just(String)
, which is Clone
but not Copy
.
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.
Is that perhaps a typo? The initial issue mentions Clone
.
Regardless, it seems a little risky to require Clone
as it might permanently break lots of proptests out there. Would it make sense to instead provide it under a new name, preserving the current prop_flat_map
? Not ideal either, I know...
I otherwise am happy to see some effort in trying to do something about the shrinking behavior for prop_flat_map
. Like @nzeh I've run into the issue of poor shrinking many times.
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.
Ugh. Yes, that's a typo. The updated implementation in the pull request requires Clone
able strategies, not ones that implement Copy
. I aggree with @Andlon that adding even the requirement that the strategy implement Clone
is suboptimal. After spending quite some time thinking about how to avoid it at the time, I concluded that doing what my shrink implementation does (shrink the outer strategy first and then the inner one) is not possible with the current architecture of the ValueTree
trait. I think it is possible if we completely overhaul the architecture of at least parts of proptest, but that was not something I intended to undertake at the time. Moreover, this overhaul in itself could lead to breakage of code that uses proptest because it would involve changing at least the ValueTree
trait.
return true; | ||
} else { |
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.
Dead code?
- `prop_flat_map` can only be used with closures that produce a strategy that | ||
implements `Copy`. | ||
|
||
- Half-bounded ranges (`x..`, `..x`, and `..=x`) are no longer supported for |
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.
This problem was fixed in #261 . These changes aren't needed anymore.
Sorry for the delay in getting to this. I think the overall suggestion is sound but I don't think a breaking change is the right approach at the moment. We already have 3 APIs for flat mapping ( I'm also interested in seeing if there's any way to change the behavior without the Clone bound. I'm not opposed to changing the underlying shrinking strategy of |
This pull request aims to address issues #268 and #181, alas by introducing some potentially but hopefully minor breaking changes. I'm more than happy to discuss whether there's a better way to implement these improvements, without breaking anything or at least breaking less.
The tests in
proptest
andproptest-derive
all pass again. This came at the cost of removing half-bounded ranges as strategies forf32
andf64
. Those were the failing tests forproptest
. After looking at how therand
crate generates floats in a range and howproptest
tries to use that generator, I concluded that there is no way to make the tests pass. Moreover, the tests are fairly representative of what should be a common use of half-bounded float ranges as strategies, so they would fail also in real-world use and simply shouldn't be supported.In my opinion, the current shrinking strategy for values produced by
prop_flat_map
is almost always a poor choice. It first tries to shrink the inner value and only when this fails, shrinks the outer value on which the inner value depends. I ran into this when trying to generate a graph with a number of vertices and a set of vectors that store attributes, one per vertex. The most natural way to do this is to generate the number of vertices and thenprop_flat_map
this to a strategy that generates a graph with that number of vertices and a set of vectors, each of this length. Shrinking is completely ineffective here because it will try tons of possibilities of messing with the vector entries and with the edge set of the graph before trying to shrink the number of vertices. As a result, shrinking will either take hours or will fail to produce a meaningfully shrunk input, depending on the bound on the number of shrinks.I reimplemented the shrinking strategy for
FlattenValueTree
so it shrinks the outer value first and only when that fails, refines the result by shrinking the inner value. Unfortunately, given the API in theStrategy
trait, I didn't see a way to do this without requiring that the strategy produced by the closure given toprop_flat_map
implementClone
, a requirement that wasn't there before. This may break some custom strategies, but I expect it to break less code than trying to mess with theStrategy
trait itself and, at least in my use cases, seems like a small price to pay for getting a useful shrinking strategy fromprop_flat_map
.As I said above, I expect some objections to these changes and am more than happy to discuss whether this can be done better. However, I think the change to the shrinking strategy of
FlattenValueTree
is crucial, only its implementation may be suboptimal.