From a004dbb59e1a21c04f28146a3700b5a26ebbc5e8 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Fri, 22 Dec 2023 16:43:02 +0100 Subject: [PATCH 01/18] add warning all elements to sampled_from are strategies, suggestive of one_of; close #3819 --- AUTHORS.rst | 1 + .../src/hypothesis/strategies/_internal/core.py | 4 ++++ hypothesis-python/tests/cover/test_lookup.py | 2 ++ hypothesis-python/tests/cover/test_sampled_from.py | 10 ++++++++++ 4 files changed, 17 insertions(+) diff --git a/AUTHORS.rst b/AUTHORS.rst index ff0c99ac00..e992d3dfb4 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -181,6 +181,7 @@ their individual contributions. * `Tyler Gibbons `_ (tyler.gibbons@flexport.com) * `Tyler Nickerson `_ * `Vidya Rani `_ (vidyarani.d.g@gmail.com) +* `Vince Reuter `_ (vince.reuter@gmail.com) * `Vincent Michel `_ (vxgmichel@gmail.com) * `Viorel Pluta `_ (viopluta@gmail.com) * `Vytautas Strimaitis `_ diff --git a/hypothesis-python/src/hypothesis/strategies/_internal/core.py b/hypothesis-python/src/hypothesis/strategies/_internal/core.py index 03653c61e1..bfa6e390bd 100644 --- a/hypothesis-python/src/hypothesis/strategies/_internal/core.py +++ b/hypothesis-python/src/hypothesis/strategies/_internal/core.py @@ -214,6 +214,10 @@ 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?" + ) if len(values) == 1: return just(values[0]) try: diff --git a/hypothesis-python/tests/cover/test_lookup.py b/hypothesis-python/tests/cover/test_lookup.py index 9801d6d3cf..5703851243 100644 --- a/hypothesis-python/tests/cover/test_lookup.py +++ b/hypothesis-python/tests/cover/test_lookup.py @@ -313,6 +313,8 @@ class Baz(Foo): @pytest.mark.parametrize( "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 8624fbc326..e9cbf42546 100644 --- a/hypothesis-python/tests/cover/test_sampled_from.py +++ b/hypothesis-python/tests/cover/test_sampled_from.py @@ -190,3 +190,13 @@ class AnnotationsInsteadOfElements(enum.Enum): def test_suggests_elements_instead_of_annotations(): with pytest.raises(InvalidArgument, match="Cannot sample.*annotations.*dataclass"): 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?", + ): + st.sampled_from(elements) From ea5412585ccdfccfc3b7f70d87aede0b1f336774 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Fri, 22 Dec 2023 17:38:38 +0100 Subject: [PATCH 02/18] try catching expected warning in test contexts --- hypothesis-python/tests/common/utils.py | 11 ++++ hypothesis-python/tests/cover/test_lookup.py | 29 ++++++--- .../tests/nocover/test_pretty_repr.py | 64 ++++++++++--------- .../tests/nocover/test_reusable_values.py | 39 +++++------ .../tests/nocover/test_strategy_state.py | 8 ++- 5 files changed, 90 insertions(+), 61 deletions(-) diff --git a/hypothesis-python/tests/common/utils.py b/hypothesis-python/tests/common/utils.py index 2cd5332857..d96bf85910 100644 --- a/hypothesis-python/tests/common/utils.py +++ b/hypothesis-python/tests/common/utils.py @@ -10,6 +10,7 @@ import contextlib import sys +import warnings from io import StringIO from types import SimpleNamespace @@ -85,6 +86,16 @@ def capture_out(): sys.stdout = old_out +class catch_sampled_from_strategies_warning(warnings.catch_warnings): + def __enter__(self): + warnings.filterwarnings( + action="ignore", + message="sample_from was given a collection of strategies; was one_of intended?", + category=UserWarning, + ) + super().__enter__() + + class ExcInfo: pass diff --git a/hypothesis-python/tests/cover/test_lookup.py b/hypothesis-python/tests/cover/test_lookup.py index 5703851243..4a1dc92b66 100644 --- a/hypothesis-python/tests/cover/test_lookup.py +++ b/hypothesis-python/tests/cover/test_lookup.py @@ -42,7 +42,11 @@ find_any, minimal, ) -from tests.common.utils import fails_with, temp_registered +from tests.common.utils import ( + catch_sampled_from_strategies_warning, + fails_with, + temp_registered, +) sentinel = object() BUILTIN_TYPES = tuple( @@ -311,23 +315,28 @@ class Baz(Foo): @pytest.mark.parametrize( - "var,expected", + "var,expected,exp_warn", [ # 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)), - (typing.TypeVar("V", bound=typing.Union[int, str]), (int, str)), - (typing.TypeVar("V", int, str), (int, str)), + (typing.TypeVar("V"), object, False), + (typing.TypeVar("V", bound=int), int, False), + (typing.TypeVar("V", bound=Foo), (Bar, Baz), True), + (typing.TypeVar("V", bound=typing.Union[int, str]), (int, str), True), + (typing.TypeVar("V", int, str), (int, str), False), ], ) @settings(suppress_health_check=[HealthCheck.too_slow]) @given(data=st.data()) -def test_typevar_type_is_consistent(data, var, expected): +def test_typevar_type_is_consistent(data, var, expected, exp_warn): strat = st.from_type(var) - v1 = data.draw(strat) - v2 = data.draw(strat) + if exp_warn: + with catch_sampled_from_strategies_warning(): + v1 = data.draw(strat) + v2 = data.draw(strat) + else: + v1 = data.draw(strat) + v2 = data.draw(strat) assume(v1 != v2) # Values may vary, just not types assert type(v1) == type(v2) assert isinstance(v1, expected) diff --git a/hypothesis-python/tests/nocover/test_pretty_repr.py b/hypothesis-python/tests/nocover/test_pretty_repr.py index fc0170e21c..3e0c120ad8 100644 --- a/hypothesis-python/tests/nocover/test_pretty_repr.py +++ b/hypothesis-python/tests/nocover/test_pretty_repr.py @@ -16,6 +16,8 @@ from hypothesis.control import reject from hypothesis.errors import HypothesisDeprecationWarning, InvalidArgument +from tests.common.utils import catch_sampled_from_strategies_warning + def foo(x): pass @@ -51,39 +53,39 @@ def splat(value): values = st.integers() | st.text() - -Strategies = st.recursive( - st.one_of( - st.sampled_from( - [ - st.none(), - st.booleans(), - st.randoms(use_true_random=True), - st.complex_numbers(), - st.randoms(use_true_random=True), - st.fractions(), - st.decimals(), - ] +with catch_sampled_from_strategies_warning(): + Strategies = st.recursive( + st.one_of( + st.sampled_from( + [ + st.none(), + st.booleans(), + st.randoms(use_true_random=True), + st.complex_numbers(), + st.randoms(use_true_random=True), + st.fractions(), + st.decimals(), + ] + ), + st.builds(st.just, values), + st.builds(st.sampled_from, st.lists(values, min_size=1)), + builds_ignoring_invalid(st.floats, st.floats(), st.floats()), ), - st.builds(st.just, values), - st.builds(st.sampled_from, st.lists(values, min_size=1)), - builds_ignoring_invalid(st.floats, st.floats(), st.floats()), - ), - lambda x: st.one_of( - builds_ignoring_invalid(st.lists, x, **size_strategies), - builds_ignoring_invalid(st.sets, x, **size_strategies), - builds_ignoring_invalid(lambda v: st.tuples(*v), st.lists(x)), - builds_ignoring_invalid(lambda v: st.one_of(*v), st.lists(x, min_size=1)), - builds_ignoring_invalid( - st.dictionaries, - x, - x, - dict_class=st.sampled_from([dict, OrderedDict]), - **size_strategies, + lambda x: st.one_of( + builds_ignoring_invalid(st.lists, x, **size_strategies), + builds_ignoring_invalid(st.sets, x, **size_strategies), + builds_ignoring_invalid(lambda v: st.tuples(*v), st.lists(x)), + builds_ignoring_invalid(lambda v: st.one_of(*v), st.lists(x, min_size=1)), + builds_ignoring_invalid( + st.dictionaries, + x, + x, + dict_class=st.sampled_from([dict, OrderedDict]), + **size_strategies, + ), + st.builds(lambda s, f: s.map(f), x, st.sampled_from(fns)), ), - st.builds(lambda s, f: s.map(f), x, st.sampled_from(fns)), - ), -) + ) strategy_globals = {k: getattr(st, k) for k in dir(st)} diff --git a/hypothesis-python/tests/nocover/test_reusable_values.py b/hypothesis-python/tests/nocover/test_reusable_values.py index ce3701a389..aa1d844eb8 100644 --- a/hypothesis-python/tests/nocover/test_reusable_values.py +++ b/hypothesis-python/tests/nocover/test_reusable_values.py @@ -13,6 +13,8 @@ from hypothesis import example, given, strategies as st from hypothesis.errors import InvalidArgument +from tests.common.utils import catch_sampled_from_strategies_warning + # Be aware that tests in this file pass strategies as arguments to @example. # That's normally a mistake, but for these tests it's intended. # If one of these tests fails, Hypothesis will complain about the @@ -45,24 +47,25 @@ def reusable(): """Meta-strategy that produces strategies that should have ``.has_reusable_values == True``.""" - return st.one_of( - # This looks like it should be `one_of`, but `sampled_from` is correct - # because we want this meta-strategy to yield strategies as its values. - st.sampled_from(base_reusable_strategies), - # This sometimes produces invalid combinations of arguments, which - # we filter out further down with an explicit validation check. - st.builds( - st.floats, - min_value=st.none() | st.floats(allow_nan=False), - max_value=st.none() | st.floats(allow_nan=False), - allow_infinity=st.booleans(), - allow_nan=st.booleans(), - ), - st.builds(st.just, st.builds(list)), - st.builds(st.sampled_from, st.lists(st.builds(list), min_size=1)), - st.lists(reusable).map(st.one_of), - st.lists(reusable).map(lambda ls: st.tuples(*ls)), - ) + with catch_sampled_from_strategies_warning(): + return st.one_of( + # This looks like it should be `one_of`, but `sampled_from` is correct + # because we want this meta-strategy to yield strategies as its values. + st.sampled_from(base_reusable_strategies), + # This sometimes produces invalid combinations of arguments, which + # we filter out further down with an explicit validation check. + st.builds( + st.floats, + min_value=st.none() | st.floats(allow_nan=False), + max_value=st.none() | st.floats(allow_nan=False), + allow_infinity=st.booleans(), + allow_nan=st.booleans(), + ), + st.builds(st.just, st.builds(list)), + st.builds(st.sampled_from, st.lists(st.builds(list), min_size=1)), + st.lists(reusable).map(st.one_of), + st.lists(reusable).map(lambda ls: st.tuples(*ls)), + ) def is_valid(s): diff --git a/hypothesis-python/tests/nocover/test_strategy_state.py b/hypothesis-python/tests/nocover/test_strategy_state.py index 97ab43e7a8..7cf527532c 100644 --- a/hypothesis-python/tests/nocover/test_strategy_state.py +++ b/hypothesis-python/tests/nocover/test_strategy_state.py @@ -34,6 +34,8 @@ tuples, ) +from tests.common.utils import catch_sampled_from_strategies_warning + AVERAGE_LIST_LENGTH = 2 @@ -183,7 +185,8 @@ def repr_is_good(self, strat): MAIN = __name__ == "__main__" -TestHypothesis = HypothesisSpec.TestCase +with catch_sampled_from_strategies_warning(): + TestHypothesis = HypothesisSpec.TestCase TestHypothesis.settings = settings( TestHypothesis.settings, @@ -193,4 +196,5 @@ def repr_is_good(self, strat): ) if MAIN: - TestHypothesis().runTest() + with catch_sampled_from_strategies_warning(): + TestHypothesis().runTest() From 347e43796310b757359626c43304c072e8bafd3a Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Fri, 22 Dec 2023 18:10:03 +0100 Subject: [PATCH 03/18] add explicit stacklevel argument to pass linting --- hypothesis-python/src/hypothesis/strategies/_internal/core.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hypothesis-python/src/hypothesis/strategies/_internal/core.py b/hypothesis-python/src/hypothesis/strategies/_internal/core.py index bfa6e390bd..4e6f83201d 100644 --- a/hypothesis-python/src/hypothesis/strategies/_internal/core.py +++ b/hypothesis-python/src/hypothesis/strategies/_internal/core.py @@ -216,7 +216,8 @@ def sampled_from( 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?" + "sample_from was given a collection of strategies; was one_of intended?", + stacklevel=1, ) if len(values) == 1: return just(values[0]) From 7a417cab6e83475b12daee9de9cfcaf17bcb2417 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Fri, 22 Dec 2023 18:23:01 +0100 Subject: [PATCH 04/18] generate patch release description file --- hypothesis-python/RELEASE.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 hypothesis-python/RELEASE.rst diff --git a/hypothesis-python/RELEASE.rst b/hypothesis-python/RELEASE.rst new file mode 100644 index 0000000000..674ee8cf49 --- /dev/null +++ b/hypothesis-python/RELEASE.rst @@ -0,0 +1,5 @@ +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 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 From 1ab904acdce848ade700e83a34eb9d7eaf82ee7b Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Tue, 26 Dec 2023 01:51:52 +0100 Subject: [PATCH 05/18] remove warning-compensatory changes to test suite --- hypothesis-python/tests/common/utils.py | 11 ---- hypothesis-python/tests/cover/test_lookup.py | 27 +++----- .../tests/nocover/test_pretty_repr.py | 64 +++++++++---------- .../tests/nocover/test_reusable_values.py | 39 ++++++----- .../tests/nocover/test_strategy_state.py | 8 +-- 5 files changed, 60 insertions(+), 89 deletions(-) diff --git a/hypothesis-python/tests/common/utils.py b/hypothesis-python/tests/common/utils.py index d96bf85910..2cd5332857 100644 --- a/hypothesis-python/tests/common/utils.py +++ b/hypothesis-python/tests/common/utils.py @@ -10,7 +10,6 @@ import contextlib import sys -import warnings from io import StringIO from types import SimpleNamespace @@ -86,16 +85,6 @@ def capture_out(): sys.stdout = old_out -class catch_sampled_from_strategies_warning(warnings.catch_warnings): - def __enter__(self): - warnings.filterwarnings( - action="ignore", - message="sample_from was given a collection of strategies; was one_of intended?", - category=UserWarning, - ) - super().__enter__() - - class ExcInfo: pass diff --git a/hypothesis-python/tests/cover/test_lookup.py b/hypothesis-python/tests/cover/test_lookup.py index 4a1dc92b66..910185c899 100644 --- a/hypothesis-python/tests/cover/test_lookup.py +++ b/hypothesis-python/tests/cover/test_lookup.py @@ -42,11 +42,7 @@ find_any, minimal, ) -from tests.common.utils import ( - catch_sampled_from_strategies_warning, - fails_with, - temp_registered, -) +from tests.common.utils import fails_with, temp_registered sentinel = object() BUILTIN_TYPES = tuple( @@ -319,24 +315,19 @@ class Baz(Foo): [ # Expect a warning exactly when the type constraint/bound should trigger # passing a strategy to sampled_from. - (typing.TypeVar("V"), object, False), - (typing.TypeVar("V", bound=int), int, False), - (typing.TypeVar("V", bound=Foo), (Bar, Baz), True), - (typing.TypeVar("V", bound=typing.Union[int, str]), (int, str), True), - (typing.TypeVar("V", int, str), (int, str), False), + (typing.TypeVar("V"), object), + (typing.TypeVar("V", bound=int), int), + (typing.TypeVar("V", bound=Foo), (Bar, Baz)), + (typing.TypeVar("V", bound=typing.Union[int, str]), (int, str)), + (typing.TypeVar("V", int, str), (int, str)), ], ) @settings(suppress_health_check=[HealthCheck.too_slow]) @given(data=st.data()) -def test_typevar_type_is_consistent(data, var, expected, exp_warn): +def test_typevar_type_is_consistent(data, var, expected): strat = st.from_type(var) - if exp_warn: - with catch_sampled_from_strategies_warning(): - v1 = data.draw(strat) - v2 = data.draw(strat) - else: - v1 = data.draw(strat) - v2 = data.draw(strat) + v1 = data.draw(strat) + v2 = data.draw(strat) assume(v1 != v2) # Values may vary, just not types assert type(v1) == type(v2) assert isinstance(v1, expected) diff --git a/hypothesis-python/tests/nocover/test_pretty_repr.py b/hypothesis-python/tests/nocover/test_pretty_repr.py index 3e0c120ad8..fc0170e21c 100644 --- a/hypothesis-python/tests/nocover/test_pretty_repr.py +++ b/hypothesis-python/tests/nocover/test_pretty_repr.py @@ -16,8 +16,6 @@ from hypothesis.control import reject from hypothesis.errors import HypothesisDeprecationWarning, InvalidArgument -from tests.common.utils import catch_sampled_from_strategies_warning - def foo(x): pass @@ -53,39 +51,39 @@ def splat(value): values = st.integers() | st.text() -with catch_sampled_from_strategies_warning(): - Strategies = st.recursive( - st.one_of( - st.sampled_from( - [ - st.none(), - st.booleans(), - st.randoms(use_true_random=True), - st.complex_numbers(), - st.randoms(use_true_random=True), - st.fractions(), - st.decimals(), - ] - ), - st.builds(st.just, values), - st.builds(st.sampled_from, st.lists(values, min_size=1)), - builds_ignoring_invalid(st.floats, st.floats(), st.floats()), + +Strategies = st.recursive( + st.one_of( + st.sampled_from( + [ + st.none(), + st.booleans(), + st.randoms(use_true_random=True), + st.complex_numbers(), + st.randoms(use_true_random=True), + st.fractions(), + st.decimals(), + ] ), - lambda x: st.one_of( - builds_ignoring_invalid(st.lists, x, **size_strategies), - builds_ignoring_invalid(st.sets, x, **size_strategies), - builds_ignoring_invalid(lambda v: st.tuples(*v), st.lists(x)), - builds_ignoring_invalid(lambda v: st.one_of(*v), st.lists(x, min_size=1)), - builds_ignoring_invalid( - st.dictionaries, - x, - x, - dict_class=st.sampled_from([dict, OrderedDict]), - **size_strategies, - ), - st.builds(lambda s, f: s.map(f), x, st.sampled_from(fns)), + st.builds(st.just, values), + st.builds(st.sampled_from, st.lists(values, min_size=1)), + builds_ignoring_invalid(st.floats, st.floats(), st.floats()), + ), + lambda x: st.one_of( + builds_ignoring_invalid(st.lists, x, **size_strategies), + builds_ignoring_invalid(st.sets, x, **size_strategies), + builds_ignoring_invalid(lambda v: st.tuples(*v), st.lists(x)), + builds_ignoring_invalid(lambda v: st.one_of(*v), st.lists(x, min_size=1)), + builds_ignoring_invalid( + st.dictionaries, + x, + x, + dict_class=st.sampled_from([dict, OrderedDict]), + **size_strategies, ), - ) + st.builds(lambda s, f: s.map(f), x, st.sampled_from(fns)), + ), +) strategy_globals = {k: getattr(st, k) for k in dir(st)} diff --git a/hypothesis-python/tests/nocover/test_reusable_values.py b/hypothesis-python/tests/nocover/test_reusable_values.py index aa1d844eb8..ce3701a389 100644 --- a/hypothesis-python/tests/nocover/test_reusable_values.py +++ b/hypothesis-python/tests/nocover/test_reusable_values.py @@ -13,8 +13,6 @@ from hypothesis import example, given, strategies as st from hypothesis.errors import InvalidArgument -from tests.common.utils import catch_sampled_from_strategies_warning - # Be aware that tests in this file pass strategies as arguments to @example. # That's normally a mistake, but for these tests it's intended. # If one of these tests fails, Hypothesis will complain about the @@ -47,25 +45,24 @@ def reusable(): """Meta-strategy that produces strategies that should have ``.has_reusable_values == True``.""" - with catch_sampled_from_strategies_warning(): - return st.one_of( - # This looks like it should be `one_of`, but `sampled_from` is correct - # because we want this meta-strategy to yield strategies as its values. - st.sampled_from(base_reusable_strategies), - # This sometimes produces invalid combinations of arguments, which - # we filter out further down with an explicit validation check. - st.builds( - st.floats, - min_value=st.none() | st.floats(allow_nan=False), - max_value=st.none() | st.floats(allow_nan=False), - allow_infinity=st.booleans(), - allow_nan=st.booleans(), - ), - st.builds(st.just, st.builds(list)), - st.builds(st.sampled_from, st.lists(st.builds(list), min_size=1)), - st.lists(reusable).map(st.one_of), - st.lists(reusable).map(lambda ls: st.tuples(*ls)), - ) + return st.one_of( + # This looks like it should be `one_of`, but `sampled_from` is correct + # because we want this meta-strategy to yield strategies as its values. + st.sampled_from(base_reusable_strategies), + # This sometimes produces invalid combinations of arguments, which + # we filter out further down with an explicit validation check. + st.builds( + st.floats, + min_value=st.none() | st.floats(allow_nan=False), + max_value=st.none() | st.floats(allow_nan=False), + allow_infinity=st.booleans(), + allow_nan=st.booleans(), + ), + st.builds(st.just, st.builds(list)), + st.builds(st.sampled_from, st.lists(st.builds(list), min_size=1)), + st.lists(reusable).map(st.one_of), + st.lists(reusable).map(lambda ls: st.tuples(*ls)), + ) def is_valid(s): diff --git a/hypothesis-python/tests/nocover/test_strategy_state.py b/hypothesis-python/tests/nocover/test_strategy_state.py index 7cf527532c..97ab43e7a8 100644 --- a/hypothesis-python/tests/nocover/test_strategy_state.py +++ b/hypothesis-python/tests/nocover/test_strategy_state.py @@ -34,8 +34,6 @@ tuples, ) -from tests.common.utils import catch_sampled_from_strategies_warning - AVERAGE_LIST_LENGTH = 2 @@ -185,8 +183,7 @@ def repr_is_good(self, strat): MAIN = __name__ == "__main__" -with catch_sampled_from_strategies_warning(): - TestHypothesis = HypothesisSpec.TestCase +TestHypothesis = HypothesisSpec.TestCase TestHypothesis.settings = settings( TestHypothesis.settings, @@ -196,5 +193,4 @@ def repr_is_good(self, strat): ) if MAIN: - with catch_sampled_from_strategies_warning(): - TestHypothesis().runTest() + TestHypothesis().runTest() From e50d6a42685c00b30a31a64855ef5f180b8654c5 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Tue, 26 Dec 2023 18:29:19 +0100 Subject: [PATCH 06/18] 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 674ee8cf49..43d876f294 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 ee9762525d..43ae192610 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -1586,6 +1586,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 4e6f83201d..03653c61e1 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 caf8d1ba9a..f0ea591117 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 910185c899..9801d6d3cf 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 e9cbf42546..ae13976072 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) + ) From 80337c1a0bda80f1f3d0441aa171bfe5213baee2 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Wed, 27 Dec 2023 01:11:37 +0100 Subject: [PATCH 07/18] remove unused helper method, add param ids, work better with staticmethod --- .../tests/cover/test_sampled_from.py | 69 +++++++++---------- 1 file changed, 34 insertions(+), 35 deletions(-) diff --git a/hypothesis-python/tests/cover/test_sampled_from.py b/hypothesis-python/tests/cover/test_sampled_from.py index ae13976072..24db6975df 100644 --- a/hypothesis-python/tests/cover/test_sampled_from.py +++ b/hypothesis-python/tests/cover/test_sampled_from.py @@ -195,12 +195,6 @@ def test_suggests_elements_instead_of_annotations(): 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(_): @@ -222,12 +216,12 @@ def kwargs_with_type_error_without_message_substring(_): raise TypeError("Substring not in message!") @staticmethod - @given(st.sampled_from(elements)) + @given(st.sampled_from((*elements, False, True))) 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)) + @given(all_strat_sample=st.sampled_from((*elements, False, True))) def type_error_but_not_all_strategies_kwargs(all_strat_sample): raise TypeError("SearchStrategy, but should not trigger note addition!") @@ -254,28 +248,34 @@ def kwargs_without_error(all_strat_sample): @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)], + pytest.param(f.__func__, err, msg_exp, id=f.__func__.__name__) + for f, err, msg_exp in [ + (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 @@ -297,8 +297,7 @@ def test_error_appropriate_error_note_3819( "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) - ) + if should_exp_msg: + assert msg_in_notes + else: + assert not msg_in_notes From 0a97a7cec1ca0b06dde19ac2e8a15a95be3cea49 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Fri, 29 Dec 2023 01:59:04 +0100 Subject: [PATCH 08/18] make implementation valid for more use cases --- hypothesis-python/src/hypothesis/core.py | 26 ++++++++----------- .../strategies/_internal/strategies.py | 9 ++++--- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/hypothesis-python/src/hypothesis/core.py b/hypothesis-python/src/hypothesis/core.py index 43ae192610..fe2bce4843 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -1024,7 +1024,17 @@ def _execute_once_for_engine(self, data: ConjectureData) -> None: # The test failed by raising an exception, so we inform the # engine that this test run was interesting. This is the normal # path for test runs that fail. - + if ( + isinstance(e, TypeError) + and "SearchStrategy" in str(e) + and getattr( + data, "sampling_is_from_a_collection_of_strategies", False + ) + ): + add_note( + e, + "sample_from was given a collection of strategies; was one_of intended?", + ) tb = get_trimmed_traceback() info = data.extra_information info._expected_traceback = format_exception(e, tb) # type: ignore @@ -1586,20 +1596,6 @@ 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/strategies.py b/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py index f0ea591117..91beac7733 100644 --- a/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py +++ b/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py @@ -477,9 +477,6 @@ 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 @@ -531,6 +528,12 @@ def _transform(self, element): def do_draw(self, data): result = self.do_filtered_draw(data) + if ( + isinstance(result, SearchStrategy) + and not hasattr(data, "sampling_is_from_a_collection_of_strategies") + and all(isinstance(x, SearchStrategy) for x in self.elements) + ): + data.sampling_is_from_a_collection_of_strategies = True if result is filter_not_satisfied: data.mark_invalid(f"Aborted test because unable to satisfy {self!r}") return result From 8d73152714fe5b1b8a40688e1c82faa31fd1d320 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Fri, 29 Dec 2023 02:01:23 +0100 Subject: [PATCH 09/18] add indirect tests --- .../tests/cover/test_sampled_from.py | 74 ++++++++----------- 1 file changed, 29 insertions(+), 45 deletions(-) diff --git a/hypothesis-python/tests/cover/test_sampled_from.py b/hypothesis-python/tests/cover/test_sampled_from.py index 24db6975df..12d8532d57 100644 --- a/hypothesis-python/tests/cover/test_sampled_from.py +++ b/hypothesis-python/tests/cover/test_sampled_from.py @@ -196,54 +196,29 @@ class TestErrorNoteBehavior3819: elements = (st.booleans(), st.decimals(), st.integers(), st.text()) @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") + @given(st.lists(st.sampled_from(elements))) + def indirect_without_error(_): + return @staticmethod - @given(st.sampled_from(elements)) - def args_with_type_error_without_message_substring(_): - raise TypeError("Substring not in message!") + @given(st.lists(st.sampled_from(elements))) + def indirect_with_non_type_error(_): + raise Exception("Contains SearchStrategy, but no note addition!") @staticmethod - @given(st.sampled_from(elements)) - def kwargs_with_type_error_without_message_substring(_): + @given(st.lists(st.sampled_from(elements))) + def indirect_with_type_error_without_substring(_): raise TypeError("Substring not in message!") @staticmethod - @given(st.sampled_from((*elements, False, True))) - 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, False, True))) - def type_error_but_not_all_strategies_kwargs(all_strat_sample): - raise TypeError("SearchStrategy, but should not trigger note addition!") + @given(st.lists(st.sampled_from((*elements, False, True)))) + def indirect_with_type_error_with_substring_but_not_all_strategies(_): + raise TypeError("Contains SearchStrategy, but no 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 + @given(st.lists(st.sampled_from(elements))) + def indirect_all_strategies_with_type_error_with_substring(_): + raise TypeError("Contains SearchStrategy in message") @pytest.mark.parametrize( ["func_to_call", "exp_err_cls", "should_exp_msg"], @@ -254,6 +229,7 @@ def kwargs_without_error(all_strat_sample): for f in ( args_with_type_error_with_message_substring, kwargs_with_type_error_with_message_substring, + indirect_all_strategies_with_type_error_with_substring, ) ] + [ @@ -261,20 +237,28 @@ def kwargs_without_error(all_strat_sample): for f in ( args_with_type_error_without_message_substring, kwargs_with_type_error_without_message_substring, + type_error_but_not_all_strategies_args, + type_error_but_not_all_strategies_kwargs, + indirect_with_type_error_without_substring, + indirect_with_type_error_with_substring_but_not_all_strategies, ) ] + [ - (f, TypeError, False) + (f, Exception, False) for f in ( - type_error_but_not_all_strategies_args, - type_error_but_not_all_strategies_kwargs, + non_type_error_args, + non_type_error_kwargs, + indirect_with_non_type_error, ) ] + [ - (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, + indirect_without_error, + ) ] - + [(f, None, False) for f in (args_without_error, kwargs_without_error)] ], ) def test_error_appropriate_error_note_3819( From 8e2e2795a9cdffe9b7ec54b60c0da8b9c34137ae Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Fri, 29 Dec 2023 13:14:37 +0100 Subject: [PATCH 10/18] ensure that err reprod doesn't squash error notes --- hypothesis-python/src/hypothesis/core.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hypothesis-python/src/hypothesis/core.py b/hypothesis-python/src/hypothesis/core.py index fe2bce4843..a3c2d1204d 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -1165,6 +1165,12 @@ def run_engine(self): err.__cause__ = err.__context__ = e errors_to_report.append((fragments, err)) except BaseException as e: + try: + notes = info._expected_exception.__notes__ + except AttributeError: + pass + else: + e.__notes__ = notes # If we have anything for explain-mode, this is the time to report. fragments.extend(explanations[falsifying_example.interesting_origin]) errors_to_report.append( @@ -1238,6 +1244,7 @@ def _raise_to_user(errors_to_report, settings, target_lines, trailer=""): if settings.verbosity >= Verbosity.normal: for line in target_lines: add_note(the_error_hypothesis_found, line) + raise the_error_hypothesis_found From 4fdeade0b4a2fa0faa18bd35ea76ef9b2f896aa9 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Fri, 29 Dec 2023 13:15:41 +0100 Subject: [PATCH 11/18] add direct draw tests for #3819; condense and narrow assertion logic --- .../tests/cover/test_sampled_from.py | 56 +++++++++++++------ 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/hypothesis-python/tests/cover/test_sampled_from.py b/hypothesis-python/tests/cover/test_sampled_from.py index 12d8532d57..7e479a0aaf 100644 --- a/hypothesis-python/tests/cover/test_sampled_from.py +++ b/hypothesis-python/tests/cover/test_sampled_from.py @@ -195,6 +195,35 @@ def test_suggests_elements_instead_of_annotations(): class TestErrorNoteBehavior3819: elements = (st.booleans(), st.decimals(), st.integers(), st.text()) + @staticmethod + @given(st.data()) + def direct_without_error(data): + data.draw(st.sampled_from((st.floats(), st.binary()))) + + @staticmethod + @given(st.data()) + def direct_with_non_type_error(data): + data.draw(st.sampled_from(st.characters(), st.floats())) + raise Exception("Contains SearchStrategy, but no note addition!") + + @staticmethod + @given(st.data()) + def direct_with_type_error_without_substring(data): + data.draw(st.sampled_from(st.booleans(), st.binary())) + raise TypeError("Substring not in message!") + + @staticmethod + @given(st.data()) + def direct_with_type_error_with_substring_but_not_all_strategies(data): + data.draw(st.sampled_from(st.booleans(), False, True)) + raise TypeError("Contains SearchStrategy, but no note addition!") + + @staticmethod + @given(st.data()) + def direct_all_strategies_with_type_error_with_substring(data): + data.draw(st.sampled_from((st.dates(), st.datetimes()))) + raise TypeError("This message contains SearchStrategy as substring!") + @staticmethod @given(st.lists(st.sampled_from(elements))) def indirect_without_error(_): @@ -216,9 +245,9 @@ def indirect_with_type_error_with_substring_but_not_all_strategies(_): raise TypeError("Contains SearchStrategy, but no note addition!") @staticmethod - @given(st.lists(st.sampled_from(elements))) - def indirect_all_strategies_with_type_error_with_substring(_): - raise TypeError("Contains SearchStrategy in message") + @given(st.lists(st.sampled_from(elements), min_size=1)) + def indirect_all_strategies_with_type_error_with_substring(objs): + raise TypeError("Contains SearchStrategy in message, trigger note!") @pytest.mark.parametrize( ["func_to_call", "exp_err_cls", "should_exp_msg"], @@ -227,18 +256,15 @@ def indirect_all_strategies_with_type_error_with_substring(_): for f, err, msg_exp in [ (f, TypeError, True) for f in ( - args_with_type_error_with_message_substring, - kwargs_with_type_error_with_message_substring, + direct_all_strategies_with_type_error_with_substring, indirect_all_strategies_with_type_error_with_substring, ) ] + [ (f, TypeError, False) for f in ( - args_with_type_error_without_message_substring, - kwargs_with_type_error_without_message_substring, - type_error_but_not_all_strategies_args, - type_error_but_not_all_strategies_kwargs, + direct_with_type_error_without_substring, + direct_with_type_error_with_substring_but_not_all_strategies, indirect_with_type_error_without_substring, indirect_with_type_error_with_substring_but_not_all_strategies, ) @@ -246,16 +272,14 @@ def indirect_all_strategies_with_type_error_with_substring(_): + [ (f, Exception, False) for f in ( - non_type_error_args, - non_type_error_kwargs, + direct_with_non_type_error, indirect_with_non_type_error, ) ] + [ (f, None, False) for f in ( - args_without_error, - kwargs_without_error, + direct_without_error, indirect_without_error, ) ] @@ -272,11 +296,9 @@ def test_error_appropriate_error_note_3819( else: assert True else: - with pytest.raises(Exception) as err_ctx: + with pytest.raises(exp_err_cls) as err_ctx: func_to_call() - err = err_ctx.value - assert type(err) is exp_err_cls - notes = getattr(err, "__notes__", []) + notes = getattr(err_ctx.value, "__notes__", []) msg_in_notes = ( "sample_from was given a collection of strategies; was one_of intended?" in notes From 195683e8cd6cdbf470641dfdb4670128f67b1971 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 31 Dec 2023 01:31:00 +0100 Subject: [PATCH 12/18] simplify no-error check in test; https://github.com/HypothesisWorks/hypothesis/pull/3820\#discussion_r1438466523 --- hypothesis-python/tests/cover/test_sampled_from.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/hypothesis-python/tests/cover/test_sampled_from.py b/hypothesis-python/tests/cover/test_sampled_from.py index 7e479a0aaf..3fa06db248 100644 --- a/hypothesis-python/tests/cover/test_sampled_from.py +++ b/hypothesis-python/tests/cover/test_sampled_from.py @@ -289,12 +289,8 @@ def test_error_appropriate_error_note_3819( self, func_to_call, exp_err_cls, should_exp_msg ): 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 + # Here we only care that no exception was raised, so nothing to assert. + func_to_call() else: with pytest.raises(exp_err_cls) as err_ctx: func_to_call() From dd36487f1ba8d8ff6580a30c15d144994d3f7af7 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 31 Dec 2023 01:52:58 +0100 Subject: [PATCH 13/18] more informative note for #3819 --- hypothesis-python/src/hypothesis/core.py | 21 +++++++++---------- .../strategies/_internal/strategies.py | 8 +++---- .../tests/cover/test_sampled_from.py | 15 +++++++------ 3 files changed, 20 insertions(+), 24 deletions(-) diff --git a/hypothesis-python/src/hypothesis/core.py b/hypothesis-python/src/hypothesis/core.py index a3c2d1204d..5de3314e7c 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -1024,17 +1024,16 @@ def _execute_once_for_engine(self, data: ConjectureData) -> None: # The test failed by raising an exception, so we inform the # engine that this test run was interesting. This is the normal # path for test runs that fail. - if ( - isinstance(e, TypeError) - and "SearchStrategy" in str(e) - and getattr( - data, "sampling_is_from_a_collection_of_strategies", False - ) - ): - add_note( - e, - "sample_from was given a collection of strategies; was one_of intended?", - ) + if isinstance(e, TypeError) and "SearchStrategy" in str(e): + try: + bad_strat_repr = data._sampled_from_strategy_repr + except AttributeError: + pass + else: + add_note( + e, + f"sample_from was given a collection of strategies: {bad_strat_repr}. Was one_of intended?", + ) tb = get_trimmed_traceback() info = data.extra_information info._expected_traceback = format_exception(e, tb) # type: ignore diff --git a/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py b/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py index 91beac7733..f483c49e7a 100644 --- a/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py +++ b/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py @@ -528,12 +528,10 @@ def _transform(self, element): def do_draw(self, data): result = self.do_filtered_draw(data) - if ( - isinstance(result, SearchStrategy) - and not hasattr(data, "sampling_is_from_a_collection_of_strategies") - and all(isinstance(x, SearchStrategy) for x in self.elements) + if isinstance(result, SearchStrategy) and all( + isinstance(x, SearchStrategy) for x in self.elements ): - data.sampling_is_from_a_collection_of_strategies = True + data._sampled_from_strategy_repr = repr(self) if result is filter_not_satisfied: data.mark_invalid(f"Aborted test because unable to satisfy {self!r}") return result diff --git a/hypothesis-python/tests/cover/test_sampled_from.py b/hypothesis-python/tests/cover/test_sampled_from.py index 3fa06db248..9ef648a487 100644 --- a/hypothesis-python/tests/cover/test_sampled_from.py +++ b/hypothesis-python/tests/cover/test_sampled_from.py @@ -295,11 +295,10 @@ def test_error_appropriate_error_note_3819( with pytest.raises(exp_err_cls) as err_ctx: func_to_call() notes = getattr(err_ctx.value, "__notes__", []) - msg_in_notes = ( - "sample_from was given a collection of strategies; was one_of intended?" - in notes - ) - if should_exp_msg: - assert msg_in_notes - else: - assert not msg_in_notes + matching_messages = [ + n + for n in notes + if n.startswith("sample_from was given a collection of strategies") + and n.endswith("Was one_of intended?") + ] + assert len(matching_messages) == (1 if should_exp_msg else 0) From ed50209c302f0b1ae8da0b6ce8a6e453461e62b7 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Sun, 31 Dec 2023 02:17:05 +0100 Subject: [PATCH 14/18] ignore mypy check for new attr --- hypothesis-python/src/hypothesis/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hypothesis-python/src/hypothesis/core.py b/hypothesis-python/src/hypothesis/core.py index 5de3314e7c..c8e1e13809 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -1026,7 +1026,7 @@ def _execute_once_for_engine(self, data: ConjectureData) -> None: # path for test runs that fail. if isinstance(e, TypeError) and "SearchStrategy" in str(e): try: - bad_strat_repr = data._sampled_from_strategy_repr + bad_strat_repr = data._sampled_from_strategy_repr # type: ignore[attr-defined] except AttributeError: pass else: From 2c37fb76ad290bd76d8cd6d658d2305f09a72031 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Wed, 3 Jan 2024 16:20:39 +0100 Subject: [PATCH 15/18] limit the number of elements shown for API use suggestion; #3819 --- hypothesis-python/src/hypothesis/core.py | 7 ++----- .../hypothesis/strategies/_internal/strategies.py | 14 +++++++++++++- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/hypothesis-python/src/hypothesis/core.py b/hypothesis-python/src/hypothesis/core.py index c8e1e13809..1ed2613228 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -1026,14 +1026,11 @@ def _execute_once_for_engine(self, data: ConjectureData) -> None: # path for test runs that fail. if isinstance(e, TypeError) and "SearchStrategy" in str(e): try: - bad_strat_repr = data._sampled_from_strategy_repr # type: ignore[attr-defined] + suggestion = data._sampled_from_all_strategies_elements_message except AttributeError: pass else: - add_note( - e, - f"sample_from was given a collection of strategies: {bad_strat_repr}. Was one_of intended?", - ) + add_note(e, suggestion) tb = get_trimmed_traceback() info = data.extra_information info._expected_traceback = format_exception(e, tb) # type: ignore diff --git a/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py b/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py index f483c49e7a..f2cc0925a5 100644 --- a/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py +++ b/hypothesis-python/src/hypothesis/strategies/_internal/strategies.py @@ -531,7 +531,19 @@ def do_draw(self, data): if isinstance(result, SearchStrategy) and all( isinstance(x, SearchStrategy) for x in self.elements ): - data._sampled_from_strategy_repr = repr(self) + max_num_strats = 3 + preamble = ( + f"(first {max_num_strats}) " + if len(self.elements) > max_num_strats + else "" + ) + strat_reprs = ", ".join( + map(get_pretty_function_description, self.elements[:max_num_strats]) + ) + data._sampled_from_all_strategies_elements_message = ( + "sample_from was given a collection of strategies: " + f"{preamble}{strat_reprs}. Was one_of intended?" + ) if result is filter_not_satisfied: data.mark_invalid(f"Aborted test because unable to satisfy {self!r}") return result From b61590ef125d0b151f070177fb069a9f5af09558 Mon Sep 17 00:00:00 2001 From: Vince Reuter Date: Wed, 3 Jan 2024 18:35:59 +0100 Subject: [PATCH 16/18] locally ignore undefined attribute for mypy --- hypothesis-python/src/hypothesis/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hypothesis-python/src/hypothesis/core.py b/hypothesis-python/src/hypothesis/core.py index 1ed2613228..0036b142fc 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -1026,7 +1026,7 @@ def _execute_once_for_engine(self, data: ConjectureData) -> None: # path for test runs that fail. if isinstance(e, TypeError) and "SearchStrategy" in str(e): try: - suggestion = data._sampled_from_all_strategies_elements_message + suggestion = data._sampled_from_all_strategies_elements_message # type: ignore[attr-defined] except AttributeError: pass else: From 82408d91579d3966189f00fe413c620db49859e9 Mon Sep 17 00:00:00 2001 From: Zac Hatfield-Dodds Date: Mon, 8 Jan 2024 16:06:03 +1100 Subject: [PATCH 17/18] tweak formatting + move note --- hypothesis-python/RELEASE.rst | 7 ++++-- hypothesis-python/src/hypothesis/core.py | 27 +++++++++++------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/hypothesis-python/RELEASE.rst b/hypothesis-python/RELEASE.rst index 43d876f294..f9a077a014 100644 --- a/hypothesis-python/RELEASE.rst +++ b/hypothesis-python/RELEASE.rst @@ -1,4 +1,7 @@ RELEASE_TYPE: patch -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`. +If a test uses :func:`~hypothesis.strategies.sampled_from` on a sequence of +strategies, and raises a ``TypeError``, we now :pep:`add a note <678>` asking +whether you meant to use :func:`~hypothesis.strategies.one_of`. + +Thanks to Vince Reuter for suggesting and implementing this hint! diff --git a/hypothesis-python/src/hypothesis/core.py b/hypothesis-python/src/hypothesis/core.py index 0036b142fc..c1b7f81247 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -917,7 +917,18 @@ def run(data): **dict(enumerate(map(to_jsonable, args))), **{k: to_jsonable(v) for k, v in kwargs.items()}, } - return test(*args, **kwargs) + + try: + return test(*args, **kwargs) + except TypeError as e: + # If we sampled from a sequence of strategies, AND failed with a + # TypeError, *AND that exception mentions SearchStrategy*, add a note: + if "SearchStrategy" in str(e): + try: + add_note(e, data._sampled_from_all_strategies_elements_message) # type: ignore + except AttributeError: + pass + raise # self.test_runner can include the execute_example method, or setup/teardown # _example, so it's important to get the PRNG and build context in place first. @@ -1024,13 +1035,6 @@ def _execute_once_for_engine(self, data: ConjectureData) -> None: # The test failed by raising an exception, so we inform the # engine that this test run was interesting. This is the normal # path for test runs that fail. - if isinstance(e, TypeError) and "SearchStrategy" in str(e): - try: - suggestion = data._sampled_from_all_strategies_elements_message # type: ignore[attr-defined] - except AttributeError: - pass - else: - add_note(e, suggestion) tb = get_trimmed_traceback() info = data.extra_information info._expected_traceback = format_exception(e, tb) # type: ignore @@ -1161,12 +1165,6 @@ def run_engine(self): err.__cause__ = err.__context__ = e errors_to_report.append((fragments, err)) except BaseException as e: - try: - notes = info._expected_exception.__notes__ - except AttributeError: - pass - else: - e.__notes__ = notes # If we have anything for explain-mode, this is the time to report. fragments.extend(explanations[falsifying_example.interesting_origin]) errors_to_report.append( @@ -1240,7 +1238,6 @@ def _raise_to_user(errors_to_report, settings, target_lines, trailer=""): if settings.verbosity >= Verbosity.normal: for line in target_lines: add_note(the_error_hypothesis_found, line) - raise the_error_hypothesis_found From 610191700792d26c7163bdb9c0b638c3954144fa Mon Sep 17 00:00:00 2001 From: Zac Hatfield-Dodds Date: Mon, 8 Jan 2024 16:32:16 +1100 Subject: [PATCH 18/18] remove unused ignore --- hypothesis-python/src/hypothesis/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hypothesis-python/src/hypothesis/core.py b/hypothesis-python/src/hypothesis/core.py index c1b7f81247..7c149d1222 100644 --- a/hypothesis-python/src/hypothesis/core.py +++ b/hypothesis-python/src/hypothesis/core.py @@ -925,7 +925,7 @@ def run(data): # TypeError, *AND that exception mentions SearchStrategy*, add a note: if "SearchStrategy" in str(e): try: - add_note(e, data._sampled_from_all_strategies_elements_message) # type: ignore + add_note(e, data._sampled_from_all_strategies_elements_message) except AttributeError: pass raise