On Nov 1, 2011, at 10:38 AM, exar...@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 :).
_______________________________________________
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
