On Fri, Aug 26, 2022 at 4:15 PM Glyph <gl...@twistedmatrix.com> wrote:

>
>
> On Aug 26, 2022, at 7:52 AM, Jean-Paul Calderone <
> exar...@twistedmatrix.com> wrote:
>
> I think property testing is a valuable tool in the testing toolbox and
> Twisted should consider adopting Hypothesis to employ it for some tests.
>
> Thoughts?
>
>
> I am somewhere in the neighborhood of +0 on this.  In principle, I love
> hypothesis and I feel like it can potentially reveal a lot of correctness
> issues, particularly in things like protocol parsing (hey, that's a thing
> we do!).  In practice, my one experience using it in depth is with
> Hyperlink and Klein, where the main effect of its deployment seems to have
> been to have uninteresting test strategy bugs provoking spurious flaky test
> failures downstream.
>

I think this is a great point.  Thanks for bringing it up.  The Klein case
is an interesting one but I think it also represents a class of failures
that we can easily set aside for Twisted.  Let me check my understanding of
the situation first: hyperlink exposes some Hypothesis strategies as part
of its public API for other libraries to use to help them write tests more
easily.  Klein uses one of these strategies and sometimes that strategy
itself fails while building a value.

If that's correct, the simplest solution for Twisted is to say that we
won't expose any Hypothesis strategies as part of the public API to be
consumed by other projects this way.  Of course this decision could be
revisited later after we convince ourselves we have a plan that would
prevent this failure mode - but for Hypothesis use that's purely internal
to Twisted, does it seem correct that this wouldn't be a problem?  Only
Twisted's own test suite would be subject to such failures (more on this
below).

I spent a lot of words exploring the other examples you mentioned but I've
also summarized at the bottom, in case anyone wants to save themselves 10
or so minutes.


> Here's the Klein issue: https://github.com/twisted/klein/issues/561
>
> One interesting detail of this particular bug is that Hypothesis *has* spotted
> a real issue here, but not in the right way: in its public "generate some
> valid URL representations for testing" strategy, it has instead
> accidentally produced invalid garbage that Hyperlink should arguably deal
> with somehow but which it should never produce as output because it's
> garbage.  It also flagged the issue in the wrong project, because we saw
> the failure in Klein and not in Hyperlink itself.
>

This is a bit off the point, but it looks like this particular instance is
only so troublesome because it has gone unfixed in hyperlink for so long?
The hyperlink issue doesn't seem to have gotten any attention from the
maintainers ... maybe along with all of the rest of hyperlink :/  Now I'm a
bit sad to see it isn't more actively maintained.


> I didn't do this integration, so I may have missed its value.  Perhaps on
> our way to this point, it revealed and fixed several important parsing bugs
> in Hyperlink or data-handling issues in Klein, but I'm not aware of any.
>
> So the devil's in the details here.  I don't know what configuration is
> required to achieve this, but Hypothesis should be able to generate new bug
> reports, but *not* novel blocking failures on other (presumably
> unrelated) PRs.  It seems like everyone agrees this is how it's *supposed* to
> be used, but in practice nobody bothers to figure out all the relevant
> configuration details and it becomes a source of never-ending suite
> flakiness that interrupts other work.  I haven't fully read through all of
> these, but a quick search on Github reveals thousands and thousands of
> issues like these, where others projects' test suites take a reliability
> hit due to Hypothesis's stochastic nature:
>
>
>    - https://github.com/anachronauts/jeff65/issues/41
>    - https://github.com/Scille/parsec-cloud/issues/2864
>    - https://github.com/Scille/parsec-cloud/issues/2867
>    - https://github.com/astropy/astropy/issues/10724
>    - https://github.com/pytorch/pytorch/issues/9832
>
>
> And I don't even know *how* to properly set up Hypothesis so this doesn't
> happen.
>

It's great to have these examples to consider.  I see a few different kinds
of failures here.

   - jeff65#41 and astropy#10724 are running into something that I
   basically consider to be a misfeature of Hypothesis.  By default Hypothesis
   imposes a 200ms wallclock deadlock for strategies .  *Especially* on CI
   this is absolutely a recipe for irrelevant intermittent failures.  The
   deadline is configurable
   
<https://hypothesis.readthedocs.io/en/latest/settings.html#hypothesis.settings.deadline>
   and it can also just be turned off, which is what I do at least for CI for
   all projects where I use Hypothesis (and sometimes in my regular dev
   environment too).  I see the motivation for this feature - it's not that
   hard to write strategies that absolutely explode in terms of complexity of
   the values being built and this comes with a corresponding explosion in
   their runtime.  I think this is the kind of thing you want to track with
   something like speed.twistedmatrix.com (no longer operational, I guess)
   - like any other performance concern.

   - pytorch#9832 *appears* to just be a bug in pytorch - but I admit I
   didn't dig very deep into this one.  The Hypothesis exception is saying
   that the code under test is itself non-deterministic.  There's not much
   Hypothesis can do if you use it to say you want "f(x) == y" and then half
   the time f(x) == z (for y != z) instead.  By *not* using Hypothesis, you
   might get a test suite that doesn't have this failure because you never
   notice the value of x that triggers the problem or you might get a test
   suite that has the failure but around 200x less often (because in the
   default settings Hypothesis is going to try to run your test function 200
   times on each test suite run) and so it's just more noticeable once you're
   using Hypothesis.  Hypothesis helpfully reports the specific case that's a
   problem (in the case of this pytorch failure, it says  it called the test
   function with batch_size=3, stride=1, pad=2, kernel=5, dilation=1,
   size=9, channels=7, order=u'NCHW', gc=, dc=[] and the first time it
   raised AssertionError: Gradient check failed for input X but on a
   subsequent call it did not.  Since the test appears to involve floating
   point math ... I'm not surprised, probably?  pytorch could probably
use the example
   feature to turn this from a non-deterministic failure into a
   deterministic failure, if they wanted.

   There is another configurable feature related to this.  You can "
   derandomize
   
<https://hypothesis.readthedocs.io/en/latest/settings.html#hypothesis.settings.derandomize>"
   runs so that, while Hypothesis still calls your test with semi-random
   values from your strategies a bunch of times, it gives it the same sequence
   of values on every test run - so if the test passes once in your dev env
   you can be pretty sure it will pass on CI.  Of course, this is because
   you're reducing the amount of testing you're letting Hypothesis do for
   you.  But still, flaky CI is annoying so maybe this is a reasonable way to
   configure CI.  The Hypothesis docs themselves suggest you could have most
   CI run derandomized but a separate dedicated job that runs randomized, not
   for each PR/commit, to discover new bugs - but keep them out of the usual
   development workflow.


   - The parsec-cloud failures ... are tricky.  parsec-cloud is using a
   very cool, advanced feature of Hypothesis for testing state machines where
   Hypothesis essentially builds a path through the state space of a state
   machine as the "given".  I've written a few of these and they're extremely
   powerful in terms of size of test code versus amount of coverage you get.
   But parsec-cloud is also using the "Bundle" feature from this part of
   Hypothesis which I've never really understand, *and *some kind of
   Hypothesis/trio integration that I'm not familiar with, *and* it seems
   to be exercising an *asynchronous* coroutine based state machine, *and*
   all of the output is filtered through pytest so it's not in the shape I'm
   used to ... Overall this really just means I don't know what's going on
   here.  Hypothesis *says* that one of the *strategies* is behaving
   non-deterministically but honestly I'm not even 100% I see what strategies
   this test code is using.  So, maybe this is an application bug, maybe it's
   a test bug, maybe it's a Hypothesis bug ... I dunno.

   I'd be happy to say that Twisted should steer clear of any asynchronous
   state machine testing with Hypothesis for the near future too.  Maybe even
   any synchronous state machine testing (but automat should *totally* be
   leveraging that, by the way).  Offhand I can think of a few areas of
   Twisted where the state machine testing might be handy but the regular
   stateless version is going to be useful much, much more often.

There is one other area worth mentioning - and that's trial integration.
Hypothesis basically works with trial with no extra work but it's not quite
as polished as it could be.  In particular, you can't use the given
decorator with a test method that returns a Deferred and sometimes trial's
stdout handling frustrates Hypothesis' attempts to report details about
failures.  I don't think either of these are very deep (and the former
doesn't really bother me personally much since I prefer not to write
Deferred-returning tests anymore) but they probably represent a bit more
development work that *should* happen if we do start using Hypothesis
widely in Twisted's test suite.  Of course, doing this work will also
benefit all downstream trial users who want to use Hypothesis.


> So, in summary: I'd love to see Hypothesis help us discover and fix bugs
> but if it is a choice between occasionally helping us find those bugs but
> constantly blocking important work with difficult-to-debug failures in
> unrelated parsing code that is now covered by hypothesis, or "nothing", I'd
> go with "nothing".
>

So, I think the following steps/policies would address all of the concerns
described here:

   - Restrict Hypothesis usage to Twisted's own test suite (i.e., it is an
   implementation detail of a private part of Twisted, not the basis of any
   public interface)
   - Turn off Hypothesis' deadline feature, probably everywhere
   - At least for normal CI, and maybe for development environments, turn
   off Hypothesis' randomization feature.
   Perhaps set up a scheduled CI job that runs with randomization enabled.
   - At least initially - and maybe indefinitely - avoid the stateful
   testing functionality.

 Jean-Paul
_______________________________________________
Twisted mailing list -- twisted@python.org
To unsubscribe send an email to twisted-le...@python.org
https://mail.python.org/mailman3/lists/twisted.python.org/
Message archived at 
https://mail.python.org/archives/list/twisted@python.org/message/HIEAR5OXSAJNGKQMQQMIMJXQT2KDKSIS/
Code of Conduct: https://twisted.org/conduct

Reply via email to