Ticket #6049 defect new

Opened 19 months ago

Last modified 11 months ago

twisted.trial.test.test_asyncassertions doesn't follow the coding standard

Reported by: itamar Owned by: kkpattern
Priority: normal Milestone:
Component: trial Keywords: easy
Cc: jml, mtraceur@…, kylerzhang11@…, thijs Branch: branches/test-asyncassertions-cleanup-6049
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

twisted.trial.test.test_asyncassertions should be improved so that e.g. all tests have docstrings, and the existing docstrings are more explicit about what they are testing, etc..

Attachments

my-twisted-patch.patch Download (3.0 KB) - added by marktraceur 18 months ago.
A patch to elaborate existing docstrings and add new ones.
docstring-elaboration.patch Download (3.0 KB) - added by marktraceur 18 months ago.
A patch to elaborate existing docstrings and add new ones.
docstring-elaboration.2.patch Download (3.3 KB) - added by marktraceur 18 months ago.
A patch to elaborate existing docstrings and add new ones.
docstring-elaboration.3.patch Download (2.2 KB) - added by kkpattern 12 months ago.
A patch to elaborate existing docstrings and add new ones.
ticket6049.4.patch Download (3.0 KB) - added by kkpattern 12 months ago.
A patch to elaborate existing docstrings and add new ones, also elaborate the test names.

Change History

1

Changed 19 months ago by DefaultCC Plugin

  • cc jml added

Changed 18 months ago by marktraceur

A patch to elaborate existing docstrings and add new ones.

2

Changed 18 months ago by marktraceur

  • cc mtraceur@… added
  • owner set to marktraceur

I figured I'd jump in and contribute a little patch! My Saturday nights are rather uneventful anyway :)

I'm also recording the process of fixing this bug as part of an attempt to drive contributions to free software! No need to act differently, but I figured I'd say something in the interest of full disclosure :)

Changed 18 months ago by marktraceur

A patch to elaborate existing docstrings and add new ones.

Changed 18 months ago by marktraceur

A patch to elaborate existing docstrings and add new ones.

3

Changed 18 months ago by marktraceur

  • keywords easy, review added; easy removed

4

Changed 18 months ago by exarkun

  • keywords easy added; easy, removed
  • owner marktraceur deleted

Thanks for the patch. Just a couple issue tracker comments. Make sure to unassign the ticket when you submit it for review (empty owner is at the top of the owners drop-down). Also, keywords are separated by spaces, not commas.

5

Changed 18 months ago by marktraceur

Sounds good! The interface is a bit arcane to new folks, but I'll see if I can't clarify some of this in the wiki pages for new contributors. Thanks a lot for the help!

6

Changed 18 months ago by marktraceur

Oh, hm, I should note that the latest attachment (docstring-elaboration.2.patch) is the patch I'd like to be reviewed.

7

Changed 18 months ago by therve

  • owner set to marktraceur
  • keywords review removed

Thanks a lot for the patch. I'm going to be a bit pedantic, but as the patch is mostly about test docstring I feel like it's relevant here: test docstring shouldn't use "Test/check that" and "will/should". For example:

Test that assertFailure, when called on a function that raises a 
L{ZeroDivisionError}, will fail the deferred function if it instead 
raises an L{OverflowError}. 

could be written:

C{assertFailure} returns a C{Failure} if the passed L{Deferred} doesn't fail with expected exception.

Note that I also changed the docstring to make it a bit more generic: the test is not about the precise type of exception raised, but about the behavior of assertFailure in general. Also, there is no such thing as a deferred function, but rather a function that returns a Deferred.

Thanks!

Changed 12 months ago by kkpattern

A patch to elaborate existing docstrings and add new ones.

8

Changed 12 months ago by kkpattern

  • keywords review added
  • cc kylerzhang11@… added
  • owner marktraceur deleted

A patch to elaborate existing docstrings and add new ones, based on therve's comments.

9

Changed 12 months ago by thijs

  • keywords review removed
  • cc thijs added
  • owner set to kkpattern

Thanks for your contribution.

  1. _checkInfo still needs a docstring with @param and @type for each parameter
  2. Most of the test names are incorrect, eg. test_assertFailure_wrongException should be test_assertFailureWrongException
  3. C{Failure} should be L{Failure}

Changed 12 months ago by kkpattern

A patch to elaborate existing docstrings and add new ones, also elaborate the test names.

10

Changed 12 months ago by kkpattern

  • owner kkpattern deleted
  • keywords review added

Updated patch, incorporating review comments.

11

Changed 11 months ago by therve

  • owner set to therve

12

Changed 11 months ago by therve

  • branch set to branches/test-asyncassertions-cleanup-6049
  • branch_author set to therve

(In [38585]) Branching to 'test-asyncassertions-cleanup-6049'

13

Changed 11 months ago by therve

  • keywords review removed
  • owner changed from therve to kkpattern

Thanks a lot for the patch. I've committed the patch in a branch, and there are a bunch of twistedchecker failures remaining:  http://buildbot.twistedmatrix.com/builders/twistedchecker/builds/730/steps/run-twistedchecker/logs/new%20twistedchecker%20errors. Please provide against the branch to fix them.

Note: See TracTickets for help on using tickets.