Print the seed when shrinking is slow#4677
Print the seed when shrinking is slow#4677ianhi wants to merge 8 commits intoHypothesisWorks:masterfrom
Conversation
There was a problem hiding this comment.
Thanks Ian! This message deserves some love, and I'm glad you're giving it some 😃
The seed is the right thing to report here. We can't report the @reproduce_failure blob, because the shrinker is probabilistic and depends on the seed, even if the failure doesn't.
The reason we don't include the seed by default is because @seed can be dramatically worse than @reproduce_failure. The former replays the entire history of the test up to the reproducing test case, while the latter replays only the reproducing test case. Hypothesis does a good enough job at printing the seed where appropriate (which you're improving right now!) that I don't think we should print the seed by default.
| ): | ||
| # See https://github.com/HypothesisWorks/hypothesis/issues/2340 | ||
| from hypothesis.control import current_build_context | ||
| seed = current_build_context().wrapped_test._hypothesis_internal_use_generated_seed |
There was a problem hiding this comment.
@Liam-DeVoe Following your comment this is what I came up with . But in my test I now always hit the is None branch here
hypothesis/hypothesis-python/src/hypothesis/control.py
Lines 95 to 99 in 76f2fa2
Is there a better way to get the seed down into the engine, or to otherwise expose it here?
There was a problem hiding this comment.
Hmm, right. StateForActualGivenExecution.execute_once is what enters build_context, and ConjectureRunner sits above execute_once.
All of the following are true, some are conflicting:
- We do not want to require passing the
test_functionwhich has_hypothesis_internal_use_seedtoConjectureRunner. AConjectureRunnertakes a test function, but it is an important invariant of our API that it need not be the one with_hypothesis_internal_use_seed. BuildContextis scoped narrowly to only and exactly the execution of a single test case. E.g.is_finalonly makes sense in this context. This implies we cannot extend its scope to include our slow shrink error.- It is unfortunate to have parts of the same logical error message defined in different places.
Arguably ConjectureRunner is the right level of abstraction to exit on slow shrinking, but not the right level to warn about slow shrinking. Perhaps we should move the entire message to StateForActualGivenExecution. I am weakly against this on intuition.
I would switch to your original approach, with split error messages, but reworded in a way that makes it clear and non-embarrassing if the messages are interrupted by an intermediate print or otherwise mangled. Perhaps accepting some duplication, like "This test function exited early because it took too long to shrink. You can reproduce this with @seed({seed})".
There was a problem hiding this comment.
Done.
I'm not sure I fully grasp what build context is. Is there a good place to read. about how information is meant to flow through the various objects and who controls what? I read https://github.com/HypothesisWorks/hypothesis/blob/master/guides/internals.rst but it wasn't quite what I was looking for
There was a problem hiding this comment.
This is a totally reasonable thing to want, and there is also not currently a good place to read about this. We have long wanted to add a "contributor documentation" section to the docs, but writing good docs is time-consuming. I have a mermaid diagram of the Hypothesis internal lifecycle that you might find useful - I'll find it and share later!
This is pretty much a fix for: #4670
I say pretty much because I think this would havhe solved it for me, but maybe there is potential for something even better?
An alternative I considered is to just always print the seed - is there a downside there? I understand that reproduce_failure should be preferred so hiding the seed protects the user a little bit. But always showing it protects from any other cases we haven't thought about where having the seed would be helpful.
So connect the failure. info with where the seed is (i.e. to avoid passing the seed to the engine) I used the same property strategy as in #4676