[Twisted-Python] Re: [Twisted-commits] r15665 - Rewritten endpoints (TCP and UNIX) with unittests

David Reid dreid at dreid.org
Sun Jan 22 16:21:19 EST 2006


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jean-Paul Calderone wrote:
> This branch seems to have no ticket number, so I'm commenting here.

Thanks for the comments, I'll create a ticket for it.

> twisted/test/test_endpoints.py:
> 
> 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?

The client connection attempts to 127.0.0.2 are there to make sure that
the connection fails.  It seemed logical at the time, but the time was
very late.

> None of the tests assert that the protocol/port is hooked up to the
> right address, either on the local or remote side.

I'll add these.

> None of the tests exercise client-side binding.

I'm not sure what you mean by this, but there is a test for each of the
endpoint classes that tests that the client can connect to a "normally
established server" Perhaps I didn't do this very well.

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

I wanted to get something that actually worked before I tried to factor
out duplicate, code.  But thanks for the note anyway.

> twisted/internet/endpoint.py:
> 
> 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).

Noted.

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

I made comments about a deferred never callbacking at the end of the
day, because I didn't realize that obviously IServerEndpoint.listen will
call back with the IListeningPort, I forgot to adjust the docstrings.

> Typo in IServerEndpoint.listen's docstring - "incomfing".
> 
> Also, these interfaces belong in twisted.internet.interfaces.

I'll move them there.

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

I'll look into ProtocolWrapper.

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

Oops

> TCPEndpoint.listen - try/bareword except?  Why?  This looks like a
> perfect place for twisted.internet.defer.execute().

I didn't know about defer.execute(), but thanks, I'll use it.

> AddressToEndpoint looks like it should just be a function that returns
> either a TCPEndpoint or a UNIXEndpoint.

I figured that out as I woke up this morning.

> Trailing whitespace *and tabs* throughout.

*grumble* *grumble* Yet-Another-Carbon-Emacs *grumble*

Thanks for the input JP.

- -David
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)

iD8DBQFD0/dLrsrO6aeULcgRAl8WAJ9YSYusZfZ7d+1T4WvyGWYPnIMTBACfXr/V
NTCZJGY+Q/1zsbWEqNWrkhg=
=EORJ
-----END PGP SIGNATURE-----




More information about the Twisted-Python mailing list