[Twisted-Python] Re: [Twisted-commits] r15665 - Rewritten endpoints (TCP and UNIX) with unittests
exarkun at divmod.com
Sun Jan 22 14:13:06 EST 2006
On Sun, 22 Jan 2006 02:47:24 -0700, David Reid <dreid at wolfwood.twistedmatrix.com> wrote:
>Date: Sun Jan 22 02:47:23 2006
>New Revision: 15665
>Rewritten endpoints (TCP and UNIX) with unittests
This branch seems to have no ticket number, so I'm commenting here.
Several test methods of TestEndpoints define nested functions which discard the Deferred returned by a Port's stopListening method. These need to be waiting on that Deferred.
Why do some tests bind to 127.0.0.1 and some to 127.0.0.2?
None of the tests assert that the protocol/port is hooked up to the right address, either on the local or remote side.
None of the tests exercise client-side binding.
The TCP and UNIX tests are basically identical, except for the endpoint class being used. This is a lot of unnecessary code duplication. Implement the tests with the endpoint class parameterized and then call them all twice, once with TCPEndpoint, once with UNIXEndpoint.
Lots of trailing whitespace throughout the file.
Interface definitions have docstrings - they don't need "pass" (pass is only required to satisfy grammar requirements for /some/ statement, and the docstrings do just fine for that).
The docstrings of neither IClientEndpoint.connect nor IServerEndpoint.listen define the error type which the Deferred may fail with. Is it really true that IServerEndpoint.listen will never callback its Deferred? The tests seem to think otherwise.
Typo in IServerEndpoint.listen's docstring - "incomfing".
Also, these interfaces belong in twisted.internet.interfaces.
_EndpointClientFactory.buildProtocol should not be using callLater(0). There is no guarantee that will be the correct time to deliver notification, and it is unnecessarily inefficient. Instead, you probably want something like a ProtocolWrapper from twisted.protocols.policies.
self.callable(addr) in the same buildProtocol is user-code. It would probably be useful for exceptions it raises to cause the Deferred associated with the factory to errback. Currently, errors it causes will be logged and the Deferred passed back to the application will never fire or errback.
Similar comment for _EndpointServerFactory.buildProtocol's invocation of self.callable().
TCPEndpoint.listen - try/bareword except? Why? This looks like a perfect place for twisted.internet.defer.execute().
AddressToEndpoint looks like it should just be a function that returns either a TCPEndpoint or a UNIXEndpoint.
Trailing whitespace *and tabs* throughout.
More information about the Twisted-Python