Hypothesis: Regression in 3.66.11 with stateful rules containing oneofstategy in targets

[touilleMan]

Hi,

I've encountered a new but with version 3.6.11 and onward (see https://travis-ci.org/Scille/parsec-cloud/builds/411355571#L1315).

[...]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
venv/lib/python3.6/site-packages/hypothesis/stateful.py:114: in run_state_machine_as_test
    breaker.run(state_machine_factory(), print_steps=True)
venv/lib/python3.6/site-packages/hypothesis/stateful.py:611: in __init__
    self.__rules_strategy = RuleStrategy(self)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <hypothesis.stateful.RuleStrategy object at 0x7f19fed0c0b8>, machine = FileOperationsStateMachine({})

    def __init__(self, machine):
        SearchStrategy.__init__(self)
        self.machine = machine
        self.rules = list(machine.rules())

        # The order is a bit arbitrary. Primarily we're trying to group rules
        # that write to the same location together, and to put rules with no
        # target first as they have less effect on the structure. We order from
        # fewer to more arguments on grounds that it will plausibly need less
        # data. This probably won't work especially well and we could be
        # smarter about it, but it's better than just doing it in definition
        # order.
>       self.rules.sort(key=lambda rule: (
            sorted(rule.targets), len(rule.arguments),
            rule.function.__name__,
        ))
E       TypeError: '<' not supported between instances of 'OneOfStrategy' and 'str'

venv/lib/python3.6/site-packages/hypothesis/stateful.py:527: TypeError

The trouble is my stateful rules looks like this:

        @rule(target=Folders)
        def rule1(self):
            ...

        @rule(target=Folders | Files)
        def rule2(self):
            ...

So hypothesis has to compare bundles with OneOfStrategy of bundles which worked well before 3.66.11 :'-(

[DRMacIver]

I don't think your code ever did what you intended it to. What's happening there is that "Folders | Files" is being treated as the name of a bundle, and you're never reading from a bundle with that particular name so the result of rule2 is silently ignored.

The functionality you probably intended here could be achieved by targets=(Folders, Files) (which will put the results of rule2 in both the Files and Folders bundle).

We should definitely validate this better to give a good error message though!

[Zac-HD]

The following stateful test is sufficient to reproduce the problem:

class T(RuleBasedStateMachine):
     A, B = Bundle('A'), Bundle('B')

     @rule(target=A)
     def foo(self): ...

     @rule(target=A|B)
     def bar(self): ...

run_state_machine_as_test(T)

To fix this we should implement stronger argument validation and defensive design throughout the stateful interface, not just to give a better error for this case but also to prevent similar problems in future.

[touilleMan]

@DRMacIver I don't know how I end up thinking this was a find construction
I wanted to put the result of of the rule in Folder or Files depending of which bundle the input parameters (not shown in my example) are from. This is pretty silly given bundles end up being just strings.
So current behavior is better than old one given it prevents having this kind of wrong construction silently failing.

To improve this behavior a bit further, I guess we could have a check on the element provided as target to make sure it is 1) a string 2) a bundle instance, 3) an iterable of strings or bundle instances.
Do I miss something ?

[Zac-HD]

We should make sure the error message explains that the result is added to all targets then, and therefore if you want the result added to exactly one of several targets you need a rule for each.

Those checks sound right, though maybe we could also check that each string is the name of a valid Bundle?

[DRMacIver]

an iterable of strings or bundle instances

In this case you use the targets rather than the target argument, but otherwise yeah.

Those checks sound right, though maybe we could also check that each string is the name of a valid Bundle?

This is a good idea but is a bit fiddly to do. We'd have to validate that at initialization time rather than where the rule is defined, because we don't have easy access to the list of possible bundles then.

[DRMacIver]

Additional thought: Rather than validating the names, the easiest thing to do here might be to deprecate string arguments to target alltogether and require the arguments to be bundles.

[Zac-HD]

Sounds good. Perhaps we should also deprecate providing both targets and target?

[DRMacIver]

Sounds good. Perhaps we should also deprecate providing both targets and target?

I actually thought that was already an error (it's not). Sounds good to me.

[Zac-HD]

Closed by #1496.