[Twisted-Python] Re: Hanging test cases (Was: Evangelism notes...)

[David Bolen]

Bob Ippolito <bob at redivi.com> writes:

> (1) Reactors can only be (meaningfully/predictably/etc) iterated if
> Twisted rules the universe AND the implementation of that reactor is
> amenable to that feature.  This is not a tautology.
> (2) Reactors need to fire various startup/shutdown events.  Reactors
> shouldn't be doing ANYTHING unless they are in a running state.
> 
> The current deferredResult/deferredError breaks both of these
> conditions.
> (1) It iterates the reactor (which is a historically public, but
> conceptually broken interface)
> (2) It iterates the reactor in a STOPPED state.  The reactor is never
> "running" during these tests.  Startup/Shutdown does not happen!

This seems like an internal implementation issue to me - when a
reactor is "running" (I've called run()), it's basically stuck in a
loop doing runUntilCurrent and then doIteration.  That's precisely
what iterate does.

Now, I agree that if you have tests that never initiate the run, that
you skip over some startup events.  But except for the uppermost
tests, what is being tested is a specific unit test for a piece of the
system that should be testable in isolation.  If the item under test
has a dependency on startup events, the test should arrange for them.

> The fact that the doc strings talk about re-entrancy of certain
> reactor functions and whatnot scares the shit out of me.  They should
> not have to be re-entrant.  What their current implementation is
> doing is really really broken.

Not sure which doc strings those are, but I don't see a re-entrancy
problem in the sort of scenario I'm looking at - rather than:

        reactor.run which calls
           reactor.runUntilCurrent/doIteration which causes
               my deferrable stuff to operate

I have

        deferredResult which calls
           reactor.iterate which uses
               reactor.runUntilCurrent/doIteration which causes
                   my deferrable stuff to operate
       
I don't think there's any more room for re-entrancy issues than in a normal
running reactor.

Maybe this is a key difference with trial?  If trial has a top level
reactor.run that is always above any test, then I do see how you could
get re-entrancy problems, since you'd be re-entering reactor.iterate
from within an existing reactor.iterate call.

I guess that's true of any nested use of deferredResult too, but we
don't nest our deferredResult calls - no real need since any
deferrable is directly wrapped with the deferredResult call, and
deferredResult is only used in the tests, so what they are calling is
always production code that is written properly with callbacks and
what not.

> The problem, for tests, is that using a properly written
> deferredResult / deferredError the reactor would startup/shutdown
> violently throughout the course of a single test, and will break the
> hell out of it if it has anything to do with services/etc.  So, while
> for SOME deferreds it would work fine, but for others, it wouldn't.
> Basically, unless you can encapsulate the entire test in a single
> deferred, then it shouldn't be using deferredResult/deferredError.

I still don't necessarily see that (the last sentence).  We use
multiple deferrable operations in single tests, but never more than
one at a time (e.g., no recursive or nested uses).  But certainly more
than a single deferrable operation within a single test.

I would, however, agree that I'd prefer even more the ability to
completely start/stop a reactor during the course of a test, but would
still like to be able to iterate it manually during the test to
provide a natural blocking flow to the test.

But in my current scenario, the majority of my components under test
are having interfaces tested that are not impacted by startup/shutdown
(and we don't use any services, in the Twisted sense, for example).

> Therefore, what SHOULD happen is that trial should let you write
> tests that return deferreds, and it should let you write it in the
> deferredGenerator style (so it doesn't suck so much).

I don't think I'd disagree with that - we pretty much stayed away from
trial since I wasn't comfortable with it initially, so we're a pure
Unittest approach, which certainly doesn't provide any framework for
something like that.

> Trial *could*, in theory, put the reactor into a "started" state at
> the beginning of every tests and a "stopped" state at the end, but
> then you're testing in a strange environment that doesn't really
> mimic how Twisted actually works, and it still breaks (1) which makes
> it unsuitable for testing the reactors where Twisted does not rule
> the universe.  Testing this theory would require changes to the
> reactor interface to make some parts of if re-entrant (bleargh).

Yeah, that's a tough problem, although one that would also simplify
the fact that we often run the tests under the unittest GUI, and
occasionally have to fight cross-test pollution from the reactor
persisting across test runs, which is a wart from the current
restrictions.

> Well, it causes you to write two lines of code instead of one.  That
> sucks, but so what?  You don't have to write big callback chains..
> It's conceptually identical, it's just that you have to throw in some
> extra boiler plate that says THIS IS A DEFERRED.  If the way we say
> that wasn't so ugly, it might actually be a good thing from a
> readability standpoint :)

Well, seeing "deferredResult" sort of already says "THIS IS A
DEFERRED" in the test.  And it's less the number of lines than the
inversion of the logic.  In my example, it's tougher to read through
that test and see the underlying core test of the user() and
saveUser() calls.  This is true really of any deferred code in
general, so I find the ability to simplify things in the test level an
improvement.  As you say, the way we "say" this currently is ugly.

> >     self.user = deferredResult(self.umgr.user())
> >     self.user.email = 'test at dom.ain'
> >     deferredResult(self.umgr.saveUser(self.user))
> 
> # let's assume I've aliased waitForDeferred to "wait"
> 
> d = waitForDeferred(self.umgr.user())
> yield d
> self.user = d.getResult()
> self.user.email = 'test at dom.ain'
> d = waitForDeferred(self.umgr.saveUser(self.user))
> yield d
> d.getResult()
> 
> So, the test is 6 lines instead of 3.  Which sucks, but it's  correct.
> For tests that do more stuff, it probably should be even  less of a
> problem.

Except that you didn't include the additional code I'd now have to
write to actually turn that into a generator which is iterated over by
the test in order to actually process the deferrable yields.  (Unless
this is something trial does automatically somehow).

That's what I meant by saying that the integration into generators is
slick (and goes a good way to linearizing what is normally a callback
chain), but still isn't quite as simple as the interface provided by
the deferred{Result,Error} functions.

> Well, let's say your database thing is a service, that maintains some
> kind of ephemeral state that's required in some way.  If
> deferredResult were properly written, this ephemeral state would hit
> the bit bucket on each deferredResult, probably breaking your code
> even though the test are "correct".

I might be getting lost on the "properly written" part, but if I were
testing a component that did have state triggered during reactor
startup/shutdown (which is what I think you're referring to), that
test would likely be using direct calls to the component to trigger
the startup/shutdown actions as part of the test setup/teardown, but
without using the reactor.

That's because the test would be focused on the component and not on
Twisted itself (which in the context of such a test would be a
"system" component that I'm trusting would do the right thing).  This
is clearly different than when testing Twisted itself.

Now that would cover the lion's share of the component tests but
somewhere there's the question of who plugs the components interface
into the twisted events and is that done right.  And I agree that's
messy right now, but it's such a small bit of initialization code that
even if it doesn't get full coverage in the tests it's not hard to
validate.  But I'd certainly welcome ways to test that more fully.

So my tests should be fine for what they are testing (the component).

> If you add such a component to the system, you might have to rewrite
> all of your tests, because it would not be possible to fix the way
> deferredResult works.

I'm not following this.

> So, use deferredGenerator / waitForDeferred.

Once we move to 2.0 we might consider that (I'm not quite up for
back-porting into 1.3 that we're using now), but I still think that
would complicate the tests (in terms of the overhead to actually run
each test like a generator) - but different people can have different
views on what is more maintainable.

Since we're not running our entire test bed under control of a single
"reactor.run" call (I presume that's more of what trial does?), even
using the generator would effectively be iterating the reactor
somewhere along the line.

I don't know - maybe my use case is just limited enough (non trial, no
nesting, etc...) that I don't see any true exposures through
deferred{Result,Error} while I'm getting benefits.

-- David