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 |
Zac-HD
left a comment
Zac-HD
left a comment
There was a problem hiding this comment.
looks pretty good overall! obviously not done yet, but seems promising 🙂
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 (🤯) no, that makes no sense, bundles just pick a known element so they can't be recursive.
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.
hypothesis-python/src/hypothesis/strategies/_internal/strategies.py
Outdated
Show resolved
Hide resolved
hypothesis-python/src/hypothesis/strategies/_internal/strategies.py
Outdated
Show resolved
Hide resolved
hypothesis-python/src/hypothesis/strategies/_internal/strategies.py
Outdated
Show resolved
Hide resolved
|
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 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Having just tried to sketch a 'recursive bundle'... there's no such thing, bundles draw from a flat list of values that can't include invoking bundle draws 🤦. So I think all we need is to see CI passing without those weird timeouts, and then I'll do a from-scratch review to see where we're up to - probably pretty close! If you want to get something out sooner, I'd be happy to take a refactoring-only PR which does the "delete BundleReference" etc cleanups; that should be pretty straightforward and would shrink this one a bit for easier handling and review. Not doing that is also fine of course! |
reaganjlee
force-pushed
the
consumes
branch
7 times, most recently
from
2cef0f5 to
cefcea8
Compare
|
Should be good for review now! |
reaganjlee
changed the title
Bundle as subclass of SampledFromStrategyBundle as subclass of SampledFromStrategy
Zac-HD
left a comment
Zac-HD
left a comment
There was a problem hiding this comment.
Another short review, sorry! Tests look good, though we should add one to check that mixing bundles, consumes, map, and filter all work as expected.
hypothesis-python/src/hypothesis/strategies/_internal/strategies.py
Outdated
Show resolved
Hide resolved
|
@Zac-HD This should be good for another look! |
|
(I haven't forgotten this! Still on my to-review queue, unfortunately just large and complicated enough that I haven't yet found a good time to sit down with it...) |
Zac-HD
left a comment
Zac-HD
left a comment
There was a problem hiding this comment.
OK, I think we're almost there - and again, I'm sorry this has taken me so long. Once we've handled the comments below and added some more tests, I'll run some manual tests to confirm that everything works 'in the wild', and then... 🚀
| if not self.draw_references: | ||
| return self.get_transformed_value(reference) | ||
|
|
||
| # we need both reference and the value itself to pretty-print deterministically | ||
| # and maintain any transformations that is bundle-specific | ||
| return VarReferenceMapping(reference, self.get_transformed_value(reference)) |
There was a problem hiding this comment.
Note that SampledFromStrategy._transform can return a special constant filter_not_satisfied; we'll need to handle that possibility - and write some tests that we never actually return that value to the user.
Zac-HD
left a comment
Zac-HD
left a comment
There was a problem hiding this comment.
we'll need to rebase so I've skimmed over the tests again and will review implementation after that.
| a = Bundle("a") | ||
|
|
||
| @initialize(target=a) | ||
| def initialize(self): | ||
| return multiple(1, 2, 3) | ||
|
|
||
| @rule( | ||
| a1=a.filter(lambda x: x < 2), | ||
| a2=a.filter(lambda x: x > 2), | ||
| a3=a, | ||
| ) |
There was a problem hiding this comment.
For clarity, I suggest naming the bundle val and the arguments a, b, c - it's easier to misread the error logs when every single name matches a_?\d!
| assert isinstance(bun, int) | ||
| assert bun >= 0 | ||
|
|
||
| Machine.TestCase.settings = Settings(stateful_step_count=5, max_examples=10) |
There was a problem hiding this comment.
I'd leave the default max_examples= here; because swarm testing (disabling a random subset of rules) makes coverage less reliable than you might think for small n.
|
@Zac-HD This should be good |
|
Edit: Should be good! No pressure on when to review though |
Liam-DeVoe
left a comment
Liam-DeVoe
left a comment
There was a problem hiding this comment.
I've brought this up to date with master. There's still a few things I haven't had time to look into yet:
- it doesn't feel like
Bundleshould have to definefilterandmap BundleReferenceStrategy.do_drawis not thread safe (which was not a requirement when this PR was written 😄, but is now)
3 participants
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