Print the seed when shrinking is slow#4677
Print the seed when shrinking is slow#4677Liam-DeVoe merged 11 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!
There was a problem hiding this comment.
Took a bit to polish it up: link.
I want to add this to our not-yet-written contributor docs when I have time.
|
Any more to do here? The one failed check seems unrelated and random: |
There was a problem hiding this comment.
Thanks for sticking with this Ian! You're jumping right into the deep end of Hypothesis internals here 😄
Yeah, that error was unrelated and fixed in #4678.
I've tweaked the error messages. They are now, inside of pytest:
WARNING: Hypothesis has spent more than five minutes working to shrink a failing example, and stopped because it is making very slow progress. When you re-run your tests, shrinking will resume and may take this long before aborting again.
PLEASE REPORT THIS if you can provide a reproducing example, so that we can improve shrinking performance for everyone.
This test function exited early because it took too long to shrink. If desired for debugging, you can reproduce this by adding @seed(328080732294599304565776869572183617016) to this test, or by running pytest with --hypothesis-seed=328080732294599304565776869572183617016.
And outside of pytest:
WARNING: Hypothesis has spent more than five minutes working to shrink a failing example, and stopped because it is making very slow progress. When you re-run your tests, shrinking will resume and may take this long before aborting again.
PLEASE REPORT THIS if you can provide a reproducing example, so that we can improve shrinking performance for everyone.
This test function exited early because it took too long to shrink. If desired for debugging, you can reproduce this by adding @seed(228407748615581783739792359168298581158) to this test.
Excited to get this in!
| @property | ||
| def had_very_slow_shrinking(self) -> bool: | ||
| return ( | ||
| self._runner is not None | ||
| and self._runner.statistics.get("stopped-because") |
There was a problem hiding this comment.
we can guarantee self._runner is not None for all callers, since that's set in state.run_engine which happens at the start of when hypothesis runs a test function.
d38ab5d to
36c2cae
Compare
| @given(st.integers(min_value=0, max_value=2**64 - 1)) | ||
| def test(n): | ||
| time.sleep(10) | ||
| assert n <= 2**33 | ||
|
|
There was a problem hiding this comment.
note that we already monkeypatch time for our tests with _consistently_increment_time, so we can use time.sleep here instead of the monkeypatch you had.
|
Merged - thanks for this thoughtful contribution Ian! Polished error messages are felt and appreciated by users, even if it doesn't always feel like it. |
|
Thank you for the help and guidance Liam - from my end nothing is more appreciated than these kinds of helpful and thoughtful comments from maintainers of open source libraries (especially ones I regularly use) |
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