Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve pprinting of stateful examples #4266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions hypothesis-python/RELEASE.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
RELEASE_TYPE: patch

Improve the clarity of printing counterexamples in :doc:`stateful testing <stateful>`, by avoiding confusing :class:`~hypothesis.stateful.Bundle` references with equivalent values drawn from a regular strategy.

For example, we now print:

.. code-block: python

a_0 = state.add_to_bundle(a=0)
state.unrelated(value=0)

instead of

.. code-block: python

a_0 = state.add_to_bundle(a=0)
state.unrelated(value=a_0)

if the ``unrelated`` rule draws from a regular strategy such as :func:`~hypothesis.strategies.integers` instead of the ``a`` bundle.
5 changes: 0 additions & 5 deletions hypothesis-python/src/hypothesis/stateful.py
Original file line number Diff line number Diff line change
Expand Up @@ -391,11 +391,6 @@ def _repr_step(self, rule, data, result):
def _add_result_to_targets(self, targets, result):
for target in targets:
name = self._new_name(target)

def printer(obj, p, cycle, name=name):
return p.text(name)

self.__printer.singleton_pprinters.setdefault(id(result), printer)
self.names_to_values[name] = result
self.bundles.setdefault(target, []).append(VarReference(name))

Expand Down
4 changes: 1 addition & 3 deletions hypothesis-python/tests/cover/test_stateful.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
# v. 2.0. If a copy of the MPL was not distributed with this file, You can
# obtain one at https://mozilla.org/MPL/2.0/.

import re
from collections import defaultdict
from typing import ClassVar

Expand Down Expand Up @@ -131,8 +130,7 @@ def bunch(self, source):
raise RuntimeError("Expected an assertion error")
except AssertionError as err:
notes = err.__notes__
regularized_notes = [re.sub(r"[0-9]+", "i", note) for note in notes]
assert "state.bunch(source=[nodes_i])" in regularized_notes
assert "state.bunch(source=[[]])" in notes


class FlakyStateMachine(RuleBasedStateMachine):
Expand Down
136 changes: 126 additions & 10 deletions hypothesis-python/tests/nocover/test_stateful.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@
# v. 2.0. If a copy of the MPL was not distributed with this file, You can
# obtain one at https://mozilla.org/MPL/2.0/.

import inspect
from collections import namedtuple

import pytest

from hypothesis import settings as Settings
from hypothesis import Phase, settings as Settings, strategies as st
from hypothesis.stateful import (
Bundle,
RuleBasedStateMachine,
Expand All @@ -21,7 +22,25 @@
rule,
run_state_machine_as_test,
)
from hypothesis.strategies import booleans, integers, lists


def run_to_notes(TestClass):
TestCase = TestClass.TestCase
# don't add explain phase notes to the error
TestCase.settings = Settings(phases=set(Phase) - {Phase.explain})
try:
TestCase().runTest()
except AssertionError as err:
return err.__notes__

raise RuntimeError("Expected an assertion error")


def assert_runs_to_output(TestClass, output):
# remove the first line, which is always "Falsfying example:"
actual = "\n".join(run_to_notes(TestClass)[1:])
assert actual == inspect.cleandoc(output.strip())


Leaf = namedtuple("Leaf", ("label",))
Split = namedtuple("Split", ("left", "right"))
Expand All @@ -30,7 +49,7 @@
class BalancedTrees(RuleBasedStateMachine):
trees = Bundle("BinaryTree")

@rule(target=trees, x=booleans())
@rule(target=trees, x=st.booleans())
def leaf(self, x):
return Leaf(x)

Expand Down Expand Up @@ -81,7 +100,7 @@ def is_not_too_deep(self, check):
class RoseTreeStateMachine(RuleBasedStateMachine):
nodes = Bundle("nodes")

@rule(target=nodes, source=lists(nodes))
@rule(target=nodes, source=st.lists(nodes))
def bunch(self, source):
return source

Expand Down Expand Up @@ -149,7 +168,7 @@ def __init__(self):
# achieve "swarming" by by just restricting the alphabet for single byte
# decisions, which is a thing the underlying conjecture engine will
# happily do on its own without knowledge of the rule structure.
@rule(move=integers(0, 255))
@rule(move=st.integers(0, 255))
def ladder(self, move):
self.seen.add(move)
assert len(self.seen) <= 15
Expand Down Expand Up @@ -213,29 +232,29 @@ class TestMyStatefulMachine(MyStatefulMachine.TestCase):
def test_multiple_precondition_bug():
# See https://github.com/HypothesisWorks/hypothesis/issues/2861
class MultiplePreconditionMachine(RuleBasedStateMachine):
@rule(x=integers())
@rule(x=st.integers())
def good_method(self, x):
pass

@precondition(lambda self: True)
@precondition(lambda self: False)
@rule(x=integers())
@rule(x=st.integers())
def bad_method_a(self, x):
raise AssertionError("This rule runs, even though it shouldn't.")

@precondition(lambda self: False)
@precondition(lambda self: True)
@rule(x=integers())
@rule(x=st.integers())
def bad_method_b(self, x):
raise AssertionError("This rule might be skipped for the wrong reason.")

@precondition(lambda self: True)
@rule(x=integers())
@rule(x=st.integers())
@precondition(lambda self: False)
def bad_method_c(self, x):
raise AssertionError("This rule runs, even though it shouldn't.")

@rule(x=integers())
@rule(x=st.integers())
@precondition(lambda self: True)
@precondition(lambda self: False)
def bad_method_d(self, x):
Expand Down Expand Up @@ -266,3 +285,100 @@ def bad_invariant_d(self):
raise AssertionError("This invariant runs, even though it shouldn't.")

run_state_machine_as_test(MultiplePreconditionMachine)


class UnrelatedCall(RuleBasedStateMachine):
a = Bundle("a")

def __init__(self):
super().__init__()
self.calls = set()

@rule(target=a, a=st.integers())
def add_a(self, a):
self.calls.add("add")
return a

@rule(v=a)
def f(self, v):
self.calls.add("f")

@precondition(lambda self: "add" in self.calls)
@rule(value=st.integers())
def unrelated(self, value):
self.calls.add("unrelated")

@rule()
def invariant(self):
# force all three calls to be made in a particular order (with the
# `unrelated` precondition) so we always shrink to a particular counterexample.
assert len(self.calls) != 3


def test_unrelated_rule_does_not_use_var_reference_repr():
# we are specifically looking for state.unrelated(value=0) not being replaced
# with state.unrelated(value=a_0). The `unrelated` rule is drawing from
# st.integers, not a bundle, so the values should not be conflated even if
# they're both 0.
assert_runs_to_output(
UnrelatedCall,
"""
state = UnrelatedCall()
a_0 = state.add_a(a=0)
state.f(v=a_0)
state.unrelated(value=0)
state.invariant()
state.teardown()
""",
)


class SourceSameAsTarget(RuleBasedStateMachine):
values = Bundle("values")

@rule(target=values, value=st.lists(values))
def f(self, value):
assert len(value) == 0
return value


class SourceSameAsTargetUnclearOrigin(RuleBasedStateMachine):
values = Bundle("values")

def __init__(self):
super().__init__()
self.called = False

@rule(target=values, value=st.just([]) | st.lists(values))
def f(self, value):
assert len(value) == 0 or not self.called
# ensure we get two calls: one from st.just([]) as the simplest initial
# example, then the second one is the failure from st.lists(values) of
# [[]].
self.called = True
return value


@pytest.mark.parametrize(
"TestClass", [SourceSameAsTarget, SourceSameAsTargetUnclearOrigin]
)
def test_source_same_as_target_doesnt_replace_in_output(TestClass):
# one might think that we could track strategies that draw from a bundle and
# replace any references to that value with the var reference repr (like values_0).
# SourceSameAsTarget shows a case where this might seem desirable.
#
# The problem is that the strategy may be arbitrarily complicated and use a value
# with the same repr as values_0, but from a different source.
# SourceSameAsTargetUnclearOrigin shows that this can happen - st.lists(values)
# has chosen the value [] from the bundle as a list element, but this could
# have also been generated via st.just([]). What we do *not* want to display
# here is state.f(value=[values_0]).
assert_runs_to_output(
TestClass,
f"""
state = {TestClass.__name__}()
values_0 = state.f(value=[])
state.f(value=[[]])
Comment on lines +380 to +381
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we actually do want

Suggested change
values_0 = state.f(value=[])
state.f(value=[[]])
values_0 = state.f(value=[])
state.f(value=[values_0])

because e.g. lists are mutable and so it's valuable to show the variable name which communicates the identity of the value.

I realize that this is Quite A Can Of Worms, what with not wanting values_0 = state.f(value=values_0); your motivating case in the changelog is spot-on but here and in the changed test I think the change is probably for the worse :/

Copy link
Member Author

@tybug tybug Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree that this case is worse than it used to be, but it's also not a correct transformation to make in general; see SourceSameAsTargetUnclearOrigin, where we would get state.f(value=[values_0]) even if the inner empty list came from st.just([]). And one could imagine more complicated cases of building up the inner list via some alternative st.composite where this would be more misleading.

I think we have to decide which of the two evils is worse...I believe the only case worsened by this is strategies involving bundles, which isn't great but is less common than rules involving bundles. The latter case collides relatively frequently thanks to shrinking normalizing to e.g. lots of zeros.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we did self.__printer.singleton_pprinters.setdefault(id(result), printer), but only for objects which are not actually singletons? i.e. excluding None, True, False, small integers; we're still going to miss some cases but it seems closer to a strict improvement on the status quo.

state.teardown()
""",
)