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

Fix a swarm-testing footgun #3894

Merged
merged 5 commits into from
Feb 24, 2024
Merged

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Feb 23, 2024

So, you know how we used swarm testing to choose a random subset of rules to enable on a RuleBasedStateMachine? Turns out that we could choose the empty set, which promptly fails the test because it's impossible to choose a rule from the empty set. Oops? This was still pretty rare even with a single-rule machine, but also a totally self-inflicted problem.

Also improves some reprs so that the status_reason in observability output looks nicer for stateful testing, closing #3845.

Also also fixes #3892, without docs because it's just improving the error message for something that already didn't work.

@Zac-HD Zac-HD added the performance go faster! use less memory! label Feb 23, 2024
@Zac-HD Zac-HD requested a review from tybug February 23, 2024 08:46
@tybug
Copy link
Member

tybug commented Feb 24, 2024

Following patch reproduces (some of?) the ci failures consistently, e.g. -k test_settings_decorator_applies_to_rule_based_state_machine_class:

diff --git a/hypothesis-python/src/hypothesis/strategies/_internal/featureflags.py b/hypothesis-python/src/hypothesis/strategies/_internal/featureflags.py
index 1e321e744..4b1e90e5b 100644
--- a/hypothesis-python/src/hypothesis/strategies/_internal/featureflags.py
+++ b/hypothesis-python/src/hypothesis/strategies/_internal/featureflags.py
@@ -53,6 +53,7 @@ class FeatureFlags:
         # of more features being enabled.
         if self.__data is not None:
             self.__p_disabled = data.draw_integer(0, 255) / 255.0
+            self.__p_disabled = 1
         else:
             # If data is None we're in example mode so all that matters is the
             # enabled/disabled lists above. We set this up so that everything

since we're forcing false but drawing true with probability 1. Possibly __p_disabled should be 254 / 255?

@tybug
Copy link
Member

tybug commented Feb 24, 2024

Found a weird one while debugging this. If your stateful test sets a step_count attribute, things can flake / go haywire, because hypothesis increments that in _repr_step while incorrectly assuming it's an internal attribute. I think this is an unused vestige of an old implementation, so I've just removed it.

reproducer
from hypothesis.stateful import RuleBasedStateMachine, invariant, rule

class NumberModifier(RuleBasedStateMachine):
    step_count = 0

    @rule()
    def count_step(self):
        self.step_count += 1

    @invariant()
    def divide_with_one(self):
        assert self.step_count % 2 == 0

TestCase = NumberModifier.TestCase

import unittest
unittest.main()

@Zac-HD Zac-HD force-pushed the efficient-stateful branch 2 times, most recently from 0d72399 to 16d0ee7 Compare February 24, 2024 04:50
@Zac-HD
Copy link
Member Author

Zac-HD commented Feb 24, 2024

Nice, I accidentally force-pushed over your commit (wish I could alias -f to --force-with-lease...) but that was very easy to fix from your description.

https://github.com/HypothesisWorks/hypothesis/actions/runs/8028323348/job/21933383800?pr=3894#step:7:98 is unrelated, but looks like it might be an IR flake? I think I've seen similar things one or twice before.

@tybug
Copy link
Member

tybug commented Feb 24, 2024

Interesting, I'll take a look. May turn out to need the same treatment as test_can_generate_hard_floats of "assert expected value, not expected buffer".

@Zac-HD Zac-HD merged commit 202d6af into HypothesisWorks:master Feb 24, 2024
48 checks passed
@Zac-HD Zac-HD deleted the efficient-stateful branch February 24, 2024 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance go faster! use less memory!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hypothesis.strategies.text fails if min_size is (unreasonably) large
2 participants