HypothesisWorks / hypothesis

Hypothesis is a powerful, flexible, and easy to use library for property-based testing.
https://hypothesis.works
Other
7.55k stars 587 forks source link

Better behaviour when explicitly setting linked settings #197

Closed DRMacIver closed 6 years ago

DRMacIver commented 9 years ago

Hypothesis exposes three settings:

Obviously these are all connected and certain combinations make no sense: You can't reasonably have min_satisfying_examples > max_examples, or max_examples > max_iterations.

The current behaviour, which is undocumented but should be considered tantamount to implicit public API because changing it too much will break peoples' code:

The problem is that this means that code like this is subtly wrong:

Settings(max_iterations=10)

The above code actually effectively sets max_iterations to 200 (or whatever Settings.default.max_examples currently is) because 10 < 200.

What I propose is the following change in behaviour:

  1. Explicitly setting a value forces the other values to change. So setting max_iterations explicitly forces max_examples to go down if it's greater. This should be the same both with mutable assign and setting in the constructor
  2. Setting two linked values simultaneously will work fine if they are in the correct order. If they are not in the correct order then this will emit a deprecation warning (this will become an invalid argument later) and default them in the current direction: min_satisfying_examples shrinks, max_iterations grows.

I believe this will not break any reasonable expectations of compatibility: In the case of 2, the behaviour is the same except for that it will now warn you if you are doing something wrong. In the case of 1, the behaviour almost always moves in a direction which should cause things to behave more like the code was clearly intended to mean.

HOWEVER this does still cause compatibility problems.

  1. Tests may now be testing a lot less than they previously were because max_iterations has dropped. They're testing as much as they were asked to test, but not as much as they previously were.
  2. Tests may now start to fail that previously didn't. If I set max_iterations to 5 currently then my test will still pass because it's ignored and uses max_examples. Under the new system, min_satisfying_examples will be set to max_examples=max_iterations, so any duplications or failed assumptions will cause the test to fail.

2 could be mitigated but not entirely removed by defaulting min_satisfying_examples to a smaller value in the presence of low max_examples. For example min(max_examples, max_iterations // 2, explicit_setting_value) might be a reasonable default for min_satisfying_examples.

All of this is going to be super fiddly and full of edge cases to implement, but that should be a Simple Matter Of Code. The real issue is getting the actual behaviour right. Anyone have any thoughts?

mulkieran commented 9 years ago

Is the quoted line below:

"If min_satisfying_examples < max_examples, set min_satisfying_examples to max_examples"

supposed to be something more like:

"If max_examples < min_satisfying_examples ..."

Otherwise, the suggestion seems fine to me, including the caveats. It requires an understanding of what max_iterations actually means, but if the programmer is setting it explicitly, they might as well understand that.

I think that the default re (2) that you suggest should probably be avoided. Maybe it would make sense to have something like that configurable in Settings, some invariant like: max_iterations_factor = Decimal("1.2") max_examples_factor = Decimal("1.4")

and then enforce the invariant that

max_iterations >= 1.2 * max_examples max_examples >= 1.4 * min_satisfying_examples

but, at present, I am not organized enough or my testing is not complex enough to really have a use for it, either for settings in individual tests or with respect to more general settings.

DRMacIver commented 9 years ago

Yes you're absolutely right that I got the inequality backwards. I've fixed that now, thanks.

You're probably right that complicated defaults are best avoided. I'm not sure what the best behaviour there is. Maybe it's simply to default min_satisfying_examples to 1, which is a fairly unproblematic default.

Zac-HD commented 6 years ago

Following a proposal in #535, #1211 deprecates and disables the max_iterations setting - replacing it with a max_examples * 5 heuristic.

We would also like to remove min_satisfying_example from the user-facing API at some point but do not currently have a good proposal for what to do instead. Maybe max(1, max_examples // 5)?

DRMacIver commented 6 years ago

We would also like to remove min_satisfying_example from the user-facing API at some point but do not currently have a good proposal for what to do instead. Maybe max(1, max_examples // 5)?

I think making it always 1 and letting the health checks deal with the case where you're filtering out a large percentage of the examples is probably easier and produces better error messages.

Zac-HD commented 6 years ago

I think making min_satisfying_example always 1 and letting the health checks deal with the case where you're filtering out a large percentage of the examples is probably easier and produces better error messages.

Sounds good - I'll add this to #1211. The "trust us, we have heuristic healthchecks" release is coming...