WIP: Merge Bundle as subclass of SampledFromStrategy
#4084
Conversation
reaganjlee
force-pushed
the
consumes
branch
2 times, most recently
from
6c08dc0
to
e2e7f93
Compare
|
👋 thanks for taking this on Reagan! Quick note: I generally review PRs when either CI is green, or the contributor explicitly requests a review; happy to do that here too. (although I'll be offline from later this week until early September for a camping trip and thus not reviewing then) |
|
Of course and thanks for the note! I'll make a few more attempts at this final test and let you know otherwise. Either way hope you have a good time on your trip! |
|
@Zac-HD Mind taking a look to see if it's going in the right direction? Please excuse all the extra |
hypothesis-python/src/hypothesis/strategies/_internal/strategies.py
Outdated
Show resolved
Hide resolved
|
Don't think I fully understand the logs |
There was a problem hiding this comment.
Forgive the somewhat rambly notes, I figured reviewed was better than not 🙂
Overall looks pretty good, though I think it'd be helpful to have a test where we define a consumes bundle which can recursively draw from itself (🤯). The These lines were always and only run by failing examples thing is from Phase.explain; if you disable that in the settings for these tests it'll stop.
Finally, thanks again Reagan - this has turned out to be a lot tricker than I was expecting, and I really appreciate your contribution here! I'm sure many users of stateful testing will agree with me too.
| self.elements = bundle | ||
| self.reference_to_value = partial( | ||
| self.reference_to_val_func, machine.names_to_values | ||
| ) | ||
|
|
||
| ref = super().do_draw(data) | ||
| if self.consume: | ||
| bundle.remove(ref) # pragma: no cover # coverage is flaky here |
There was a problem hiding this comment.
I'd be inclined to restore our placeholder self.elements here.
I'm also wondering what happens if you have a bundle which holds a tree of some kind. Is it possible to get an indexing error from recursive references if something is consumed at the wrong time? I'll need to think about this; we might be fine or we might need something a touch more complicated for consumes bundles...
There was a problem hiding this comment.
I think there's also a latent bug here where bundle.remove(ref) can remove a different object to the one we selected, if they're equal, which could be a really subtle problem for someone.
The solution is probably to do elements = range(len(bundle))[::-1] and modify reference_to_val_func to also accept the bundle list and do a two-step lookup + record the index, and finally pop from that index. Ouch.
hypothesis-python/src/hypothesis/strategies/_internal/strategies.py
Outdated
Show resolved
Hide resolved
| res = self.elements[i] | ||
| if self.reference_to_value: | ||
| value = self.reference_to_value(res) | ||
| if self._transform(value) is filter_not_satisfied: | ||
| return filter_not_satisfied | ||
| return res | ||
| return self._transform(res) |
There was a problem hiding this comment.
I think we still want to self._transform on line 564, right? So something like
| res = self.elements[i] | |
| if self.reference_to_value: | |
| value = self.reference_to_value(res) | |
| if self._transform(value) is filter_not_satisfied: | |
| return filter_not_satisfied | |
| return res | |
| return self._transform(res) | |
| value = self.elements[i] | |
| if self.reference_to_value: | |
| value = self.reference_to_value(value) | |
| return self._transform(value) |
| speculative_index = data.draw_integer( | ||
| 0, max_good_indices - 1, shrink_towards=self.shrink_towards | ||
| ) |
There was a problem hiding this comment.
Hmm... having gone to all this effort to plumb through shrink_towards; I wonder if we actually just want a reverse_elems boolean so that our speculation will efficiently work from the end that we want to shrink to.
Or. Having said that, we could just self.elements = bundle[::-1] in Bundle.do_draw 🤦♂️
Co-authored-by: Zac Hatfield-Dodds <[email protected]>
|
Thanks so much for the fast review! I'll continue editing tomorrow but hope you have a great time on your trip! |
|
After taking some time to think about it, I can't seem to make a useful recursive test. Any thoughts here? Maybe a potential use case would be helpful and I can see if I can finish the rest. Also the last failing tests seems unrelated |
Seems like the initial values of the states get reversed with the new implementation where e.g.
state.fail_fast(a1=a_5, a2=a_4, a3=a_3, a4=a_2, a5=a_1, a6=a_0, b1=b_2, b2=b_1, b3=b_0)become...(a1=a_0, a2=a_1, a3=a_2, a4=a_3, a5=a_4, a6=a_5, b1=b_0, b2=b_1, b3=b_2). I'll probably just need to just spend more time looking into theRule strategyandSampledFromStrategyCloses #3944