From 770660627ea39333fc38e87eefcc773dda85976d Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Tue, 26 Dec 2023 18:29:19 +0100 Subject: [PATCH] narrow the severity and cases for alternate API use suggestion; close #3819 Revised per suggestion here: https://github.com/HypothesisWorks/hypothesis/pull/3820#issuecomment-1868135168 --- hypothesis-python/RELEASE.rst | 3 +- hypothesis-python/src/hypothesis/core.py | 14 +++ .../hypothesis/strategies/_internal/core.py | 5 - .../strategies/_internal/strategies.py | 3 + hypothesis-python/tests/cover/test_lookup.py | 4 +- .../tests/cover/test_sampled_from.py | 116 ++++++++++++++++-- 6 files changed, 128 insertions(+), 17 deletions(-) diff --git a/hypothesis-python/RELEASE.rst b/hypothesis-python/RELEASE.rst index 674ee8cf496..43d876f2945 100644 --- a/hypothesis-python/RELEASE.rst +++ b/hypothesis-python/RELEASE.rst @@ -1,5 +1,4 @@ RELEASE_TYPE: patch -This patch causes a warning to be issued when :func:`~hypothesis.strategies.sampled_from` is given a nonempty collection of all strategy values as the argument to its `strategies` parameter (:issue:`3819``). +This patch causes a [note to be included](https://peps.python.org/pep-0678/) when :func:`~hypothesis.strategies.sampled_from` is given a nonempty collection of all strategy values _and_ the `given`-decorated test fails with a `TypeError` (:issue:`3819``). This is because such a call is suggestive of intent to instead use :func:`~hypothesis.strategies.one_of`. -This closes \ No newline at end of file diff --git a/hypothesis-python/src/hypothesis/core.py b/hypothesis-python/src/hypothesis/core.py index 82e359fc2f8..d2140c2f3c4 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -1585,6 +1585,20 @@ def wrapped_test(*arguments, **kwargs): if isinstance(e, BaseExceptionGroup) else get_trimmed_traceback() ) + if ( + isinstance(e, TypeError) + and "SearchStrategy" in str(e) + and any( + getattr( + s, "sampling_is_from_a_collection_of_strategies", False + ) + for s in given_kwargs.values() + ) + ): + add_note( + e, + "sample_from was given a collection of strategies; was one_of intended?", + ) raise the_error_hypothesis_found if not (ran_explicit_examples or state.ever_executed): diff --git a/hypothesis-python/src/hypothesis/strategies/_internal/core.py b/hypothesis-python/src/hypothesis/strategies/_internal/core.py index 4e6f83201d9..03653c61e11 100644 --- a/hypothesis-python/src/hypothesis/strategies/_internal/core.py +++ b/hypothesis-python/src/hypothesis/strategies/_internal/core.py @@ -214,11 +214,6 @@ def sampled_from( "so maybe you tried to write an enum as if it was a dataclass?" ) raise InvalidArgument("Cannot sample from a length-zero sequence.") - elif all(isinstance(x, SearchStrategy) for x in values): - warnings.warn( - "sample_from was given a collection of strategies; was one_of intended?", - stacklevel=1, - ) if len(values) == 1: return just(values[0]) try: diff --git a/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py b/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py index caf8d1ba9aa..f0ea5911174 100644 --- a/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py +++ b/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py @@ -477,6 +477,9 @@ def __init__(self, elements, repr_=None, transformations=()): super().__init__() self.elements = cu.check_sample(elements, "sampled_from") assert self.elements + self.sampling_is_from_a_collection_of_strategies = all( + isinstance(x, SearchStrategy) for x in self.elements + ) self.repr_ = repr_ self._transformations = transformations diff --git a/hypothesis-python/tests/cover/test_lookup.py b/hypothesis-python/tests/cover/test_lookup.py index 910185c8994..9801d6d3cfb 100644 --- a/hypothesis-python/tests/cover/test_lookup.py +++ b/hypothesis-python/tests/cover/test_lookup.py @@ -311,10 +311,8 @@ class Baz(Foo): @pytest.mark.parametrize( - "var,expected,exp_warn", + "var,expected", [ - # Expect a warning exactly when the type constraint/bound should trigger - # passing a strategy to sampled_from. (typing.TypeVar("V"), object), (typing.TypeVar("V", bound=int), int), (typing.TypeVar("V", bound=Foo), (Bar, Baz)), diff --git a/hypothesis-python/tests/cover/test_sampled_from.py b/hypothesis-python/tests/cover/test_sampled_from.py index e9cbf425465..ae139760725 100644 --- a/hypothesis-python/tests/cover/test_sampled_from.py +++ b/hypothesis-python/tests/cover/test_sampled_from.py @@ -192,11 +192,113 @@ def test_suggests_elements_instead_of_annotations(): st.sampled_from(AnnotationsInsteadOfElements).example() -@pytest.mark.parametrize("wrap", [list, tuple]) -def test_warns_when_given_entirely_strategies_as_elements(wrap): - elements = wrap([st.booleans(), st.decimals(), st.integers(), st.text()]) - with pytest.warns( - UserWarning, - match="sample_from was given a collection of strategies; was one_of intended?", +class TestErrorNoteBehavior3819: + elements = (st.booleans(), st.decimals(), st.integers(), st.text()) + + def execute_expecting_error(test_func): + with pytest.raises(TypeError) as err_ctx: + test_func() + obs_notes = getattr(err_ctx.value, "__notes__", []) + assert obs_notes[-1] == exp_msg + + @staticmethod + @given(st.sampled_from(elements)) + def args_with_type_error_with_message_substring(_): + raise TypeError("SearchStrategy") + + @staticmethod + @given(all_strat_sample=st.sampled_from(elements)) + def kwargs_with_type_error_with_message_substring(all_strat_sample): + raise TypeError("SearchStrategy") + + @staticmethod + @given(st.sampled_from(elements)) + def args_with_type_error_without_message_substring(_): + raise TypeError("Substring not in message!") + + @staticmethod + @given(st.sampled_from(elements)) + def kwargs_with_type_error_without_message_substring(_): + raise TypeError("Substring not in message!") + + @staticmethod + @given(st.sampled_from(elements)) + def type_error_but_not_all_strategies_args(_): + raise TypeError("SearchStrategy, but should not trigger note addition!") + + @staticmethod + @given(all_strat_sample=st.sampled_from(elements)) + def type_error_but_not_all_strategies_kwargs(all_strat_sample): + raise TypeError("SearchStrategy, but should not trigger note addition!") + + @staticmethod + @given(st.sampled_from(elements)) + def non_type_error_args(_): + raise Exception("SearchStrategy, but should not trigger note addition!") + + @staticmethod + @given(all_strat_sample=st.sampled_from(elements)) + def non_type_error_kwargs(all_strat_sample): + raise Exception("SearchStrategy, but should not trigger note addition!") + + @staticmethod + @given(st.sampled_from(elements)) + def args_without_error(_): + return + + @staticmethod + @given(all_strat_sample=st.sampled_from(elements)) + def kwargs_without_error(all_strat_sample): + return + + @pytest.mark.parametrize( + ["func_to_call", "exp_err_cls", "should_exp_msg"], + [ + (f, TypeError, True) + for f in ( + args_with_type_error_with_message_substring, + kwargs_with_type_error_with_message_substring, + ) + ] + + [ + (f, TypeError, False) + for f in ( + args_with_type_error_without_message_substring, + kwargs_with_type_error_without_message_substring, + ) + ] + + [ + (f, TypeError, False) + for f in ( + type_error_but_not_all_strategies_args, + type_error_but_not_all_strategies_kwargs, + ) + ] + + [(f, Exception, False) for f in (non_type_error_args, non_type_error_kwargs)] + + [(f, None, False) for f in (args_without_error, kwargs_without_error)], + ) + def test_error_appropriate_error_note_3819( + self, func_to_call, exp_err_cls, should_exp_msg ): - st.sampled_from(elements) + if exp_err_cls is None: + try: + func_to_call() + except BaseException as e: + pytest.fail(f"Expected call to succeed but got error: {e}") + else: + assert True + else: + with pytest.raises(Exception) as err_ctx: + func_to_call() + err = err_ctx.value + assert type(err) is exp_err_cls + notes = getattr(err, "__notes__", []) + msg_in_notes = ( + "sample_from was given a collection of strategies; was one_of intended?" + in notes + ) + assert ( + should_exp_msg + and msg_in_notes + or (not should_exp_msg and not should_exp_msg) + )