[Twisted-Python] Ticket #1330 - Socks V5 functionality

[exarkun at twistedmatrix.com]

On 06:14 pm, dstainton415 at gmail.com wrote:
>Hi, I'd like to help out and write unit tests for the Socks v5 code in
>this ticket:
>https://twistedmatrix.com/trac/ticket/1330
>
>Should I write something very similar to this?? ::
>http://twistedmatrix.com/trac/browser/trunk/twisted/test/test_socks.py
>
>My goal is getting socksv5 client and server code merged to mainline
>Twisted with unit tests.


twisted/test/test_socks.py is a bad example of a test suite.  Here are 
the things about it you should not emulate:

  * It has documentation that is far from complete.  Documentation is 
just as important in unit tests as elsewhere.  In particular, 
documenting the intent of every test method is critical otherwise the 
test suite is very difficult to maintain.

  * It exercises too much code in each test method.  Well written test 
methods do a single very simple thing.  A good rule of thumb is that 
there should only be one `TestCase.assert...` method call in each test 
method.

  * It uses some `TestCase.assert...` methods which are deprecated or 
soon to be deprecated.  `assert_` is the main offender here.

  * It doesn't completely cover the implementation (probably because the 
implementation wasn't developed test-driven).  You can achieve full test 
coverage without doing test-driven development but it takes more 
discipline.  I suggest doing a test-driven implementation of the SOCKSv5 
functionality you want (the easy approach to this is to start writing 
tests, then copy the *smallest* possible piece of the existing, untested 
implementation into your new implementation to make those tests pass; 
repeat until you have all of the desired functionality).

  * `StringTCPTransport` seems redundant.  `StringTransport` offers all 
of this functionality already.

  * Many names used in the module don't follow the Twisted name 
convention (most obviously, "under_scores" are used throughout rather 
than "camelCase").

  * Native strings are used to represent byte strings throughout.

  * The protocol interface is uniformly misused (it should call 
`makeConnection` not `connectionMade`)

Hope this helps,
Jean-Paul