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

[Bob Ippolito]

On May 6, 2005, at 8:19 PM, David Bolen wrote:

> 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.

This is an implementation detail.  runUntilCurrent, doIteration, and  
iterate should not be public API.

> 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 reactor itself needs startup/shutdown events.  As you've noticed,  
the thread pool depends on them.

>> 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.

We all want things that aren't really possible to do :)

> 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).

However if some implementation detail of twisted changes to take  
advantage of reactor startup/shutdown events internally then all of  
your tests would break even though the application would work.   
Having tests that fail in theory but work in practice is uh, bad.

>> 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.

Staying away from trial isn't really a bad idea.  It's obviously  
broken.  However, bringing the broken functionality from trial back  
into unittest doesn't make it any better :)

>>>     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).

Trial could and should (but doesn't currently) do that automatically  
if your test function returns an iterator instead of None.

> 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.

Using Twisted isn't as simple as using urllib to suck down a web  
page, and using ascii strings is easier than using unicode...

>> 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 assuming you actually know about all of the services necessary  
to make something work, and that these services can startup/shutdown  
properly in this manner.  The more special crap you write in a test  
the less useful the test is because it's testing something in an  
entirely different way than it actually works.

>> 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.

Move to 2.0 ASAP.  Don't backport to 1.3.

> 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.

Trial doesn't use reactor.run at all, that's why it's broken.  Using  
the generator would be iterating the reactor but it would be not  
broken.  A good implementation of test would start and stop the  
reactor at the beginning and end of every test so that you (probably)  
don't end up with side-effects due to the order that the tests run.

> 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.

You're right, some things don't break in horrible ways when you use  
deferred{Result,Error}.  Sometimes you can concatenate a str and a  
unicode and it Just Works too, but it only works if the str contains  
characters that can be decoded by the default encoding, whatever that  
happens to be at the time :)

-bob