Ticket #6042 defect new

Opened 8 months ago

Last modified 5 weeks ago

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

Reported by: itamar Owned by:
Priority: low Milestone:
Component: core Keywords: easy review
Cc: thijs Branch: branches/coding-standard-6042
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

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

Change History

Changed 4 months ago by esacteksab

1

  Changed 4 months ago by esacteksab

  • keywords easy, review added; easy removed

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!

2

  Changed 4 months ago by esacteksab

  • keywords easy added; easy, removed

3

  Changed 3 months ago by tom.prince

  • owner set to tom.prince

4

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

5

  Changed 3 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 3 months ago by esacteksab

6

  Changed 3 months ago by esacteksab

  • owner esacteksab deleted

Changed 3 months ago by esacteksab

This one passes tests

7

  Changed 3 months ago by aal95

  • status changed from new to assigned
  • owner set to aal95

8

  Changed 2 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 6 weeks ago by introom

9

follow-up: ↓ 11   Changed 6 weeks 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

10

  Changed 5 weeks ago by thijs

  • branch set to branches/coding-standard-6042
  • branch_author set to thijs

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

11

in reply to: ↑ 9 ; follow-up: ↓ 12   Changed 5 weeks ago by thijs

  • owner set to introom
  • cc thijs added
  • keywords review removed
  • branch_author thijs deleted

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 Download, could you include those as well and supply a new patch?

12

in reply to: ↑ 11 ; follow-up: ↓ 13   Changed 5 weeks 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 Download, 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?

13

in reply to: ↑ 12 ; follow-up: ↓ 14   Changed 5 weeks 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).

14

in reply to: ↑ 13 ; follow-up: ↓ 15   Changed 5 weeks 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.

15

in reply to: ↑ 14   Changed 5 weeks 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 5 weeks ago by introom

Changed 5 weeks ago by introom

16

  Changed 5 weeks ago by introom

  • keywords review added
  • owner introom deleted

added an empty .misc

17

  Changed 5 weeks ago by thijs

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

Note: See TracTickets for help on using tickets.