Opened 2 years ago

Last modified 15 months ago

#6049 defect new

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 (5)

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

Download all attachments as: .zip

Change History (18)

comment:1 Changed 2 years ago by DefaultCC Plugin

  • Cc jml added

Changed 22 months ago by marktraceur

A patch to elaborate existing docstrings and add new ones.

comment:2 Changed 22 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 22 months ago by marktraceur

A patch to elaborate existing docstrings and add new ones.

Changed 22 months ago by marktraceur

A patch to elaborate existing docstrings and add new ones.

comment:3 Changed 22 months ago by marktraceur

  • Keywords review added

comment:4 Changed 22 months ago by exarkun

  • Keywords changed from easy, review to easy review
  • 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.

comment:5 Changed 22 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!

comment:6 Changed 22 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.

comment:7 Changed 22 months ago by therve

  • Keywords review removed
  • Owner set to marktraceur

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 17 months ago by kkpattern

A patch to elaborate existing docstrings and add new ones.

comment:8 Changed 17 months ago by kkpattern

  • Cc kylerzhang11@… added
  • Keywords review added
  • Owner marktraceur deleted

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

comment:9 Changed 17 months ago by thijs

  • Cc thijs added
  • Keywords review removed
  • 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 17 months ago by kkpattern

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

comment:10 Changed 17 months ago by kkpattern

  • Keywords review added
  • Owner kkpattern deleted

Updated patch, incorporating review comments.

comment:11 Changed 15 months ago by therve

  • Owner set to therve

comment:12 Changed 15 months ago by therve

  • Author set to therve
  • Branch set to branches/test-asyncassertions-cleanup-6049

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

comment:13 Changed 15 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.