Code review comment for lp://staging/~allenap/txlongpoll/oops-amqp

Revision history for this message
Raphaƫl Badin (rvb) wrote :

This branch looks good.

[0]

Maybe the tests would be more clear if they tested one thing each like this: http://paste.ubuntu.com/731893/ (warning: I've not even run that code ;) ). What do you think?

[1]

In test_parse_int_options, why do you test the option values parsed *and* the default options, why not a simple test that the option value is what it should be (like 1234 for the frontend port)? (you already have a test for the default option values)

[2]

Maybe:
s/test_with_all_the_things/test_with_all_the_params
s/test_with_some_of_the_things/test_with_some_of_the_params
;)

review: Approve

« Back to merge proposal