Testing infrastructure for ReactorMixin tests: hooking up two protocols to each other

[itamar]

I've implemented the following code a few times: run one protocol on server, another protocol connecting to it as client, run the reactor until they both disconnect and then gathering the resulting disconnect reasons. There should be utility methods for easily doing this, and at least a couple of the existing implementations should be ported to use this as a demonstration that the API is generic enough.

[itamarst]

(In [33177]) Branching to 'connect-protocols-5392'

[itamar]

Ugh. ConnectionTestMixins does this in a half-assed, badly factored unhelpful sort of way. My code is better.

Do we need backwards compatibility for ConnectionTestMixins or can I refactor it?

[itamar]

My end goal, BTW, is to refactor both the abort tests and pretty much everything in test_tls to use the new utility method. But if ConnectionTestMixins should stay as is, I might stop after the abort tests.

[exarkun]

Test packages aren't public.

[itamar]

Ready for review. I've refactored some, but not all, tests in twisted.internet.test.test_tcp, test_unix and test_tls to use this new infrastructure, and it will also be useful for future work on reactor tests (e.g. a thorough half-close test suite as part of #5341, or an update to #5347 if this gets merged first).

I am mystified by the PortableGtkReactor failures ( http://buildbot.twistedmatrix.com/builders/winxp32-py2.7/builds/622). Any ideas?

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392

[itamar]

Ah. Because PortableGtkReactor is a broken piece of crap with minimum latency of 100ms between any network event loop iteration, and so (a) it's slow (b) it will fail tests with short timeouts. The easy fix is to make it have less crappy latency (which will also speed up the test suite). But perhaps that should be in a different ticket?

[glyph]

1. I found it a bit odd that you called the arguments serverProtocolFactory and clientProtocolFactory, despite the fact that they're not, well, protocol factories. I'd suggest renaming them to something like
2. the PortableGtkReactor failures do need to be addressed, since that's a supported platform. File another ticket, and maybe I'll have a look at that so I can get credit for this review? ;-)

Otherwise it looks pretty good and " 6 files changed, 270 insertions(+), 295 deletions(-)" speaks for itself. If PortableGtkReactor can be made to pass its tests, then this can land.

[glyph]

Actually I guess I should move it out of review, since other people should look at other tickets first.

[itamarst]

(In [33445]) Branching to 'connect-protocols-5392-2'

[itamar]

I renamed the parameters, and tests now pass on portable gtk reactor:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392-2

Putting up for review again (which can be quick) because I additionally also refactored test_newtls.py to use the new code.

[exarkun]

1. It doesn't look like the parameters were renamed.
2. The proliferation of new timeouts with new values is bad. Any test that involves real I/O is going to take an unpredictable amount of time. If we are ever going to deal with it, we really need to avoid having twenty different timeouts spread out all over the place. Beyond that, tests with 1 second timeouts are going to fail on the buildbots, a lot (The abortConnection tests with their custom 10 second timeout are already failing a lot).
3. Why is this a ReactorBuilder method? Making it so provides an extremely short-term convenience (lots of existing test methods can start using it with minimum typing) but makes it inaccessible to any other code and collides with any potential future kind of connection testing any of these tests might want to do (not catastrophically, but I can imagine a future with 5 different connectiong testing helpers, confusing people about which one they want to use). The implementation doesn't really need to be part of ReactorBuilder - it could even accept a reactor as an argument and not need to be mixed in to a ReactorBuilder/EndpointCreator either. The extra assertions about removeAll() and getDelayedCalls() seem orthogonal, too.

[exarkun]

ugh, accidental early submit.

[itamarst]

(In [33590]) Apparently I forgot to commit the renamed parameters change. Refs #5392

[itamar]

I moved connectProtocols out of the class and made it a function, and removed the timeout parameter.

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392-2

(I think the API documentation buildslave failure is unrelated to this branch, since it talks about Agent.)

[itamar]

Getting this in would be helpful for testing #3204.

[washort]

(In [33717]) Branching to connect-protocols-5392-3

[washort]

(In [33723]) Apparently I forgot to commit the renamed parameters change. Refs #5392

[washort]

Okay my use of bzr rebase was a little clumsy but I resolved a conflict with trunk and put it into connect-protocols-5392-3.

[exarkun]

Thanks! I'm really happy to see this refatoring. Overall I think it's a great change. I see a few not-so-major things with the code, though:

1. in twisted/internet/test/connectionmixins.py
   1. There's some random whitespace change in the serverFactoryFor docstring
2. in twisted/internet/test/reactormixins.py
   1. Potential error-handling shortcomings in the implementation of connectProtocols:
      1. serverEndpoint.listen Deferred could fail, the result is a hang I think
      2. Same goes for clientEndpoint.connect
   2. A 5-tuple is not a radical data structure. Perhaps an epsilon.structlike.record? Oh wait that's not part of Twisted yet. Well, something with attributes instead of indexes would be nice.
3. in twisted/internet/test/test_tls.py
   1. WrapperFactory.buildProtocol's self_ argument could probably be better named (I imagine someone coming along and deleting an apparently accidental stray _ and then being confused). "wrapperSelf" or something like that would probably do nicely.
   2. Also, WrapperFactory is gross :( I liked StartTLSClientEndpoint better before. :/ Maybe we can figure out a way to clean it up more later, though.
   3. StartTLSCreator only uses startTLS on the client side, so the name is a little misleading.
4. Since this is private infrastructure, do we really want to include it in the news file?
5. Also, what do you think of adjusting the name of this new helper function? connectProtocols doesn't suggest anything about running a reactor or waiting for a full connection/session to complete. unfortunately all of the other names that come to mind have "and" in them at least once, which isn't any better.

I also see some possible future improvements:

1. Using twisted.protocols.policies to remove the need for subclassing to use connectProtocols.
2. Allowing protocol instances to be passed in, instead of callable factories (simplifies more user code than it complexifies, I think)

[itamar]

I addressed all the review comments except WrapperFactory being gross. I also changed the API so protocol instances are passed in, which simplified things quite a bit. Started buildbot here:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392-3

[itamarst]

(In [34147]) Branching to 'connect-protocols-5392-4'

[itamar]

OK, merged forward.

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392-4

[exarkun]

Thanks!

1. twisted/internet/test/reactormixins.py
   1. The docs for ConnectableProtocol should probably point out that two of its attributes get set by some outside code. Perhaps it's worth changing this, even, and adding a method to ConnectableProtocol for setting these.
   2. The docstring also says L{ReactorBuilder.runProtocolsWithReactor}, no longer accurate.
   3. It looks like it would be easy to pull Factory out of runProtocolsWithReactor.
   4. runProtocolsWithReactor will fail if the endpoint listen call returns a synchronously failing Deferred (eg because a port couldn't be bound). finished will run and call reactor.stop() before reactor.run() has happened. twisted.internet.test.connectionmixins.needsRunningReactor was recently added to help handle this case in some other tests.
   5. The isinstance in finished can be avoided if log.err were to be added as a separate errback prior to the reactor.stop()-calling callback.
   6. Also, we should really, really stop adding log.err calls without a 2nd argument.
2. twisted/internet/test/test_tcp.py
   1. twisted.internet.test.test_tcp.AbortConnectionTestCase_Win32Reactor.test_resumeProducingThrows seems to fail a lot on the Windows Python 2.6 builder.
   2. The TCPCreator class docstring has C{ReactorBuilder.runProtocolsWithReactor}. This is the wrong name now, and it should probably be L{} instead.
   3. TCP4ClientTestsBuilder needs a class docstring
   4. TCPClientTestsBuilder and TCP6ClientTestsBuilder need better class docstrings.
   5. I think TCPClientTestsBuilder is (still) misnamed. It's not a builder in the same way as the other things in this file. It is intentionally not used to generate test cases with makeTestCaseClasses.
   6. Some (technically pre-existing) issues with runAbortTest:
      1. The comment above getDelayedCalls() has a mistake - it says buildReactor, but I'm pretty certain it means runReactor. A way to avoid this special check would be to cancel the timeout at the end of runReactor, I think. Maybe file a ticket for that?
      2. I think that the return value of flushLoggedErrors should basically always be checked, at least for its length.
3. twisted/internet/test/test_tls.py
   1. StartTLSClientEndpoint would be improved by the switch protocol ticket, right? Can you add a link to that ticket above WrapperFactory?
4. I think there are a few more tests that can be converted to use this new API. Can you identify one or two and file tickets for converting them?

[itamar]

• 2.1 will be addressed by #5393.
• 2.6.1 will be addressed by #5634.
• 4 is addressed by #5635. I also want to use this infrastructure in #3204, and I also got the impression that this would supersede #5622.

All other comments were addressed, I hope. I started another buildbot run:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392-4

[exarkun]

Thanks! Looks great, please merge.

[itamarst]

(In [34184]) Merge connect-protocols-5392-4: Utility function for connecting two protocols.

Author: itamar, washort Review: exarkun Fixes: #5392

Add new utility method to reactormixins test infrastructure, runProtocolsWithReactor. This connects two protocols with two endpoints, allowing the simplification of many of our more convoluted reactor tests.