[Twisted-Python] Global reactor unit tests in the Twisted test suite

[Glyph]

On Nov 1, 2011, at 10:38 AM, exarkun at twistedmatrix.com wrote:

> Hello,
> 
> I'd like for us to decide that we will introduce no new unit tests into 
> Twisted's test suite which use the global reactor.

+1.

Let's add this to the coding standard.  It doesn't look to me like anyone's objecting.

I think it might be worth considering a grandfather clause.  If we have an outstanding patch or branch which is otherwise close to being merged, whose only problem is real-reactor tests, I think it might not be worth it to block its landing at this point.  Instead, we could file a new ticket to fix its tests separately.

I don't know of any such outstanding tickets though, so hopefully this is a baseless concern.  And I'm happy to be overruled on this point, as well, if you feel strongly about it :).

> These tests require extra complexity in trial - to support the Deferred, 
> to check for left over event sources (incompletely implemented), and to 
> clean up left over event sources (incompletely implemented).

It might be useful to have a switch in trial that reports tests which return a Deferred, to make it easy to spot violators of this policy within Twisted.

I hope you're not suggesting getting rid of the feature _entirely_ from trial, though.  The first step to doing so would be to provide a totally comprehensive, complete, documented, maintained test fake for every interface supplied by every reactor... and then some.  I know that there are hundreds of tests for applications which _use_ Twisted that I've written which would be mind-bendingly difficult to rewrite using even a fake reactor.

For one simple example, on more than one occasion I've had a high-level abstraction wrapping a proprietary C program with spawnProcess and I wanted to test high-level logic that dealt with its results in specific cases; I could of course try to test everything using fakes, but it was actually harder to write the fakes than to set up the state for the real system, and the tests were significantly higher-value as regression tests with the real components integrated.

(In some cases it's a proprietary C extension module which needs to be run via deferToThread.)

That said, there are some caveats: none of these considerations should ever apply to Twisted itself; if they do seem to apply, then it's probably time to split that code out into a separate project.

Conversely, third-party projects should be able to implicitly rely on the reactor working as expected, and therefore don't have any concerns about testing the reactor itself.

If we could publicly expose the ReactorBuilder testing facility, and make it build only a single reactor by default, then it might be possible to test code like I described by spinning up a new reactor for each thing... but in some cases, there's an expensive subprocess that I really want multiple tests to share.  Would it be possible to move the associated file descriptors between reactors?  (On a related note, I wonder, how am I ever going to get those tests to work in disttrial...)

To summarize my own position about Trial, Deferreds, and real-reactor testing in general:

Whenever it's a reasonable amount of effort, tests should always be written against in-memory data structures and avoid doing real I/O, including any real asynchrony, since tests like that are easier to maintain and debug.
Within Twisted itself, it should always be a reasonable amount of effort to write tests the "right" way.  Any code which seems to require a real-asynchronous testing approach is either out of scope for Twisted or has other architectural problems which need to be addressed before landing it.
Some applications which use Twisted will need to interact with other systems as part of their unit tests, and they should be able to use the reactor to do that.  We should provide as many facilities to help people avoid writing tests like this as possible, but there are circumstances where we couldn't possibly provide enough, so we should continue to support trial's use as an integration testing tool.
We don't currently provide enough facilities to make it easy to avoid the reactor in all the cases where you should avoid it, and we should really improve proto_helpers et. al. so that it's easier to find in-memory implementations of stuff.

> The tests themselves are also complicated by the need to clean up those 
> event sources.

Amen.  Even today, the only document I'm aware of which actually covers this in its entirety is <http://blackjml.livejournal.com/23029.html>.  I think _maybe_ the trial tutorial actually covers all the steps but I'll need to read it very carefully to see if I can spot anything that it doesn't address :).

> The tests only exercise functionality against one reactor at a time 
> which leads to additional challenges for buildbot.

AMEN.  If we made all tests ReactorBuilder tests, would we be able to kill all the alternate-reactor (i.e. -r whatever) buildbots?  It seems like that would speed up build results quite a bit.

> If a test is for reactor functionality with multiple implementations, 
> the ReactorBuilder-style tests are better.  If a test is for 
> implementation details of a particular reactor, I think the necessary 
> parts of that reactor should just be invoked directly - on a new 
> instance.  For anything that's not a test for reactor functionality, no 
> reactor should be involved at all (protocol implementations, etc).

As I said above, we have a few gaps in this area which we need to work on filling.  For example, I recently wrote some tests where I wanted an IReactorCore provider but was dismayed to discover that Twisted doesn't provide an in-memory implementor of that interface anywhere, despite the fact that all I needed to call was addSystemEventTrigger/fireSystemEventTrigger, two APIs which just manipulate lists in memory even in their "real" implementations.  I was bad open-source citizen and did not immediately file a bug!  However, writing this email made me do so <http://twistedmatrix.com/trac/ticket/5353>.

While the code that I was writing wasn't for Twisted itself, I do feel like this may be an area where the prevalence of bad examples comes from the fact that we don't give our users a lot of options for nice, isolated testing.  It's definitely easier to just use the real reactor, despite its myriad issues, than to spend a week implementing comprehensive, testable versions of the ~40-odd interfaces in twisted.internet.interfaces.  But that means that all tx* projects tend to test things in the worse style, which means that most new patch contributors are likely to be cribbing from bad examples when writing a patch for Twisted.

So I filed another ticket for another frequent sticking point for me: <http://twistedmatrix.com/trac/ticket/5354>.  Hopefully if we can make more such things popular it will more widely disseminate the skills necessary to write tests in a more self-contained style.

> I've mentioned these ideas to various people at various times, but they 
> don't seem to be catching on, so I'd like to hear why and come to some 
> conclusion about how we're going to write tests in the future.

The new Trial tutorial provides a great resource that we were lacking for a long time, so we should make reference to its section on using StringTransport and Clock frequently and with enthusiasm.  

We a similar document for ReactorBuilder though.

I think we may want to also write up a companion wiki page that is more the rhetorical rather than pedagogic side of this problem, collecting our explanations of why you really want to use in-memory stuff instead of "real" reactors.  New contributors sometimes say that they don't feel like the code is "really" being tested unless they're testing against the real implementation of something, and it may not be immediately obvious that testing against a not-real implementation is desirable in many cases.  Much of it can just be copy/pasted from this thread :).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20111101/77a541a4/attachment.html>