Opened 5 years ago

Closed 3 years ago

#6042 defect closed fixed (fixed)

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

Reported by: Itamar Turner-Trauring Owned by: Tom Prince
Priority: low Milestone:
Component: core Keywords: easy
Cc: Thijs Triemstra Branch: branches/coding-standard-6042
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description

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.

Attachments (6)

my-twisted-patch-6042.patch (8.1 KB) - added by esacteksab 5 years ago.
my-twisted-patch-6042-3.patch (8.2 KB) - added by esacteksab 5 years ago.
my-twisted-patch-6042-4.patch (8.2 KB) - added by esacteksab 5 years ago.
This one passes tests
6042_introom.patch (6.2 KB) - added by eddie 4 years ago.
6042_introom_2.patch (5.4 KB) - added by eddie 4 years ago.
6042_introom_3.patch (5.1 KB) - added by eddie 4 years ago.

Download all attachments as: .zip

Change History (29)

Changed 5 years ago by esacteksab

Attachment: my-twisted-patch-6042.patch added

comment:1 Changed 5 years ago by esacteksab

Keywords: review added

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!

comment:2 Changed 5 years ago by esacteksab

Keywords: easy, revieweasy review

comment:3 Changed 5 years ago by Tom Prince

Owner: set to Tom Prince

comment:4 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to esacteksab

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

comment:5 Changed 5 years ago by esacteksab

Keywords: review added
  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).

Changed 5 years ago by esacteksab

comment:6 Changed 5 years ago by esacteksab

Owner: esacteksab deleted

Changed 5 years ago by esacteksab

This one passes tests

comment:7 Changed 5 years ago by Aaron

Owner: set to Aaron
Status: newassigned

comment:8 Changed 5 years ago by Stephen Thorne

Keywords: review removed

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

Changed 4 years ago by eddie

Attachment: 6042_introom.patch added

comment:9 Changed 4 years ago by eddie

Keywords: review added
Owner: Aaron deleted
Status: assignednew

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

comment:10 Changed 4 years ago by Thijs Triemstra

Author: thijs
Branch: branches/coding-standard-6042

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

comment:11 in reply to:  9 ; Changed 4 years ago by Thijs Triemstra

Author: thijs
Cc: Thijs Triemstra added
Keywords: review removed
Owner: set to eddie

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?

comment:12 in reply to:  11 ; Changed 4 years ago by eddie

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?

comment:13 in reply to:  12 ; Changed 4 years ago by Thijs Triemstra

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

comment:14 in reply to:  13 ; Changed 4 years ago by eddie

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.

comment:15 in reply to:  14 Changed 4 years ago by Thijs Triemstra

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

Changed 4 years ago by eddie

Attachment: 6042_introom_2.patch added

Changed 4 years ago by eddie

Attachment: 6042_introom_3.patch added

comment:16 Changed 4 years ago by eddie

Keywords: review added
Owner: eddie deleted

added an empty .misc

comment:17 Changed 4 years ago by Thijs Triemstra

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

comment:18 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Tom Prince
Status: newassigned

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

comment:19 Changed 4 years ago by Tom Prince

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

Refs: #6042.

comment:20 Changed 4 years ago by Tom Prince

Keywords: review added
Owner: Tom Prince deleted
Status: assignednew

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

comment:21 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

reviewing...

comment:22 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Tom Prince
Status: assignednew

Code Review

Notes:

  • Merges cleanly
  • Build results look ok:
  • Formatting and whitespace changes make things more readable.
  • New test comments make the intent much clearer.
  • Coverage is slightly improved
    [richard@zorin Twisted]$ diff -u trunk/.coverage.report branches/coding-standard-6042/.coverage.report
    --- trunk/.coverage.report	2013-06-19 08:45:50.079155125 +0100
    +++ branches/coding-standard-6042/.coverage.report	2013-06-19 08:46:37.706934850 +0100
    @@ -1,3 +1,3 @@
     Name                      Stmts   Miss  Cover
     ---------------------------------------------
    -twisted/protocols/basic     301     16    95%
    +twisted/protocols/basic     308     16    95%
    

Some things to fix or answer:

  1. twisted/protocols/test/test_basic.py
    1. Typo: "writes that that line"
    2. There are still a few docstrings that say "Test x" and "x should y"
    3. Various Twistedchecker errors, some of which look valid:
      ************* Module twisted.protocols.test.test_basic
      W9208:1050,4:OnlyProducerTransport.pauseProducing: Missing docstring
      W9208:1054,4:OnlyProducerTransport.resumeProducing: Missing docstring
      W9208:1058,4:OnlyProducerTransport.write: Missing docstring
      W9402:1187,0: The first letter of comment should be capitalized
      W9013:152,0: Expected 3 blank lines, found 2
      C0103:184,12:LineReceiverTestCase.test_buffer: Invalid name "packet_size" (should match ([a-z_][a-zA-Z0-9]*)$)
      W9015:194,0: Too many blank lines, expected 2
      C0103:205,12:LineReceiverTestCase.test_pausing: Invalid name "packet_size" (should match ([a-z_][a-zA-Z0-9]*)$)
      C0103:228,12:LineReceiverTestCase.test_rawPausing: Invalid name "packet_size" (should match ([a-z_][a-zA-Z0-9]*)$)
      C0103:249,12:LineReceiverTestCase.test_stopProducing: Invalid name "packet_size" (should match ([a-z_][a-zA-Z0-9]*)$)
      W9208:405,0:TestMixin: Missing docstring
      W9208:407,4:TestMixin.connectionMade: Missing docstring
      W9208:411,4:TestMixin.stringReceived: Missing docstring
      W9208:418,4:TestMixin.connectionLost: Missing docstring
      W9208:423,0:TestNetstring: Missing docstring
      W9208:431,0:LPTestCaseMixin: Missing docstring
      W9204:438,4:LPTestCaseMixin.getProtocol: Missing epytext markup @return for return value
      C0103:483,12:NetstringReceiverTestCase.test_buffer: Invalid name "packet_size" (should match ([a-z_][a-zA-Z0-9]*)$)
      C0103:486,12:NetstringReceiverTestCase.test_buffer: Invalid name "MAX_LENGTH" (should match ((([a-z_])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$)
      C0301:705,0: Line too long (80/79)
      W9202:809,4:RecvdAttributeMixin.makeMessage: Missing epytext markup @param for argument "data"
      W9202:809,4:RecvdAttributeMixin.makeMessage: Missing epytext markup @param for argument "protocol"
      W9204:809,4:RecvdAttributeMixin.makeMessage: Missing epytext markup @return for return value
      C0103:863,8:RecvdAttributeMixin.test_switching: Invalid name "SWITCH" (should match ([a-z_][a-zA-Z0-9]*)$)
      C0103:890,8:RecvdAttributeMixin.test_recvdInLengthLimitExceeded: Invalid name "DATA" (should match ([a-z_][a-zA-Z0-9]*)$)
      
    4. I guess introom's empty misc newsfile didn't get applied from the patch. Remember to create it before merging.
    5. You mentioned in a previous comment that recvd is deprecated. I found ticket #5770 is still open but almost ready to be merged. Might be worth adding a Refs: #5770 in the commit message.

I'd be happy for this to be merged if you fix the typo and the line length and whitespace issues above.

The other twistedchecker errors would be nice to fix (invalid variable names, missing mixin docstrings) but it may not be a good use of time.

Up to you.

-RichardW.

comment:23 Changed 3 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [42224]) Merge coding-standard-6042: Improve coding standard in twisted.protocols.test.test_basic.

Author: esacteksab, introom, tom.prince Reviewer: tom.prince, thijs, rwall Fixes: #6042

Note: See TracTickets for help on using tickets.