Opened 22 months ago

Closed 3 months ago

#6042 defect closed fixed (fixed)

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

Reported by: itamar Owned by: tom.prince
Priority: low Milestone:
Component: core Keywords: easy
Cc: thijs Branch: branches/coding-standard-6042
(diff, github, buildbot, log)
Author: Launchpad Bug:

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 18 months ago.
my-twisted-patch-6042-3.patch (8.2 KB) - added by esacteksab 18 months ago.
my-twisted-patch-6042-4.patch (8.2 KB) - added by esacteksab 18 months ago.
This one passes tests
6042_introom.patch (6.2 KB) - added by introom 16 months ago.
6042_introom_2.patch (5.4 KB) - added by introom 16 months ago.
6042_introom_3.patch (5.1 KB) - added by introom 16 months ago.

Download all attachments as: .zip

Change History (29)

Changed 18 months ago by esacteksab

comment:1 Changed 18 months 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 18 months ago by esacteksab

  • Keywords changed from easy, review to easy review

comment:3 Changed 18 months ago by tom.prince

  • Owner set to tom.prince

comment:4 Changed 18 months 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 18 months 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 18 months ago by esacteksab

comment:6 Changed 18 months ago by esacteksab

  • Owner esacteksab deleted

Changed 18 months ago by esacteksab

This one passes tests

comment:7 Changed 17 months ago by aal95

  • Owner set to aal95
  • Status changed from new to assigned

comment:8 Changed 17 months ago by jerub

  • Keywords review removed

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

Changed 16 months ago by introom

comment:9 follow-up: Changed 16 months ago by introom

  • Keywords review added
  • Owner aal95 deleted
  • Status changed from assigned to new

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

comment:10 Changed 16 months ago by thijs

  • Author set to thijs
  • Branch set to branches/coding-standard-6042

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

comment:11 in reply to: ↑ 9 ; follow-up: Changed 16 months ago by thijs

  • Author thijs deleted
  • Cc thijs added
  • Keywords review removed
  • Owner set to introom

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 ; follow-up: Changed 16 months ago by 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?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 16 months ago by 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).

comment:14 in reply to: ↑ 13 ; follow-up: Changed 16 months ago by 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.

comment:15 in reply to: ↑ 14 Changed 16 months ago by 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 '''.

Changed 16 months ago by introom

Changed 16 months ago by introom

comment:16 Changed 16 months ago by introom

  • Keywords review added
  • Owner introom deleted

added an empty .misc

comment:17 Changed 16 months ago by thijs

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

comment:18 Changed 14 months ago by tom.prince

  • Keywords review removed
  • Owner set to tom.prince
  • Status changed from new to assigned

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

comment:19 Changed 14 months ago by tomprince

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

Refs: #6042.

comment:20 Changed 14 months ago by tom.prince

  • Keywords review added
  • Owner tom.prince deleted
  • Status changed from assigned to new

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

comment:21 Changed 13 months ago by rwall

  • Owner set to rwall
  • Status changed from new to assigned

reviewing...

comment:22 Changed 13 months ago by rwall

  • Keywords review removed
  • Owner changed from rwall to tom.prince
  • Status changed from assigned to new

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 months ago by tomprince

  • Resolution set to fixed
  • Status changed from new to closed

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