twisted.protocols.test.test_basic doesn't following coding standard

[itamar]

twisted.protocols.test.test_basic (new in #6025, but just moved existing code mostly) has a variety of issues: bad test names, missing docstrings, uses fail variations of assertion methods, etc.. It should be updated.

[esacteksab]

Only my second ever patch. I'm certain it's not complete nor perfect. Seeking guidance/direction.

Removed failIf, chopped off at 80 chars, added some doc strings.

I have doubts about rcvd for received (not sure if rcvd is OK, or if RX would work or ?)

Thanks!

[tom.prince]

Thanks for your contribution. Here are a few things I saw at a first read through:

1. Test method names should start with test_ but otherwise follow the naming standard. That is, start with a lowercase letter and be camel case, e.g. test_pauseResume not test_PauseResume.
2. Normally, test docstrings should describe the expected behavior they are testing.
3. recvd is actually a (deprecated) attribute for some things in twisted.protocol.basic.
4. There are some logic changes in test_PauseResume which also lacks a docstring.
5. assertTrue(...) is preferable to assertEqual(..., True).

[esacteksab]

1. changed test_PauseResume to test_pauseResume
2. hopefully I got this docstring right.
3. I know there is a depreciation policy for backwards compatibility. Does this test still need to exist?
4. Added docstring to test_pauseResume
5. Replaced assertTrue(...) with assertEqual(..., True).

[esacteksab]

This one passes tests

[jerub]

Sorry, things have changed and this patch no longer applies cleanly. Can you refresh and resubmit the patch please?

[introom]

added some docstring modified the code to be within 80 columns deleted test_deprecatedModuleAttributes per ticket#6321

[thijs]

(In [38136]) Branching to 'coding-standard-6042'

[thijs]

Replying to introom:

added some docstring modified the code to be within 80 columns deleted test_deprecatedModuleAttributes per ticket#6321

Thanks for your patch. I'm not sure why the test is being deleted here, and not in #6321? Your patch is also missing some of the improvements made by esacteksab in my-twisted-patch-6042-4.patch , could you include those as well and supply a new patch?

[introom]

Replying to thijs:

Replying to introom:

added some docstring modified the code to be within 80 columns deleted test_deprecatedModuleAttributes per ticket#6321

Thanks for your patch. I'm not sure why the test is being deleted here, and not in #6321? Your patch is also missing some of the improvements made by esacteksab in my-twisted-patch-6042-4.patch , could you include those as well and supply a new patch?

Thanks for your review. But I am not sure which improvement I am missing. Say in line 1052 (http://twistedmatrix.com/trac/attachment/ticket/6042/my-twisted-patch-6042-4.patch) I think it's not suitable to add p.pauseProducing. because if the code is ok, then p.pauseProducing should already been invoked in p.dataReceived(str) in line 1050.

Would you please tell me which improvement I am missing?

[thijs]

Replying to introom:

Would you please tell me which improvement I am missing?

I meant the changes he did in test_basic relating to getting things on lines of 80, and adding docstrings to test(cases) that don't have them (for example ConsumingProtocol and ProducerTestCase).

[introom]

Replying to thijs:

Replying to introom:

Would you please tell me which improvement I am missing?

I meant the changes he did in test_basic relating to getting things on lines of 80, and adding docstrings to test(cases) that don't have them (for example ConsumingProtocol and ProducerTestCase).

Hi As to 80 cols, in http://twistedmatrix.com/trac/attachment/ticket/6042/my-twisted-patch-6042-4.patch i.e. the first change in line 57, actually it's exactly 79cols < 80cols, so I think's unnecessary to make it span two lines.

As to docstrings, I re-compared my patch and with 6042-4, but find they are basically the same.

[thijs]

Replying to introom:

As to docstrings, I re-compared my patch and with 6042-4, but find they are basically the same.

Ok, could you refresh the patch against trunk now that #6321 landed? And the docstrings should use """ instead of '''.

[introom]

added an empty .misc

[thijs]

(In [38157]) apply 6042_introom_2.patch, refs #6042

[tom.prince]

testPauseResumes name doesn't follow the coding standard, and its docstring doesn't explain what the test is trying to assert.

[tomprince]

(In [38504]) Add some explantion to testPauseResume.

Refs: #6042.

[tom.prince]

I renamed the test method, and add some more detailed explanation of what testPauseResume is trying to test.