Opened 2 years ago

Closed 21 months ago

#5952 enhancement closed fixed (fixed)

Improve docstring for test_notTrapped in twisted.test.test_failure

Reported by: itamar Owned by: rwall
Priority: normal Milestone:
Component: core Keywords: easy documentation
Cc: thijs, sreepriya1111@… Branch: branches/test-not-trapped-5952
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

The docstring for test_notTrapped doesn't actually describe what the test does very well; it should be rewritten to explain that.

Attachments (3)

my-first-patch.patch (1.9 KB) - added by sreepriya 22 months ago.
In the patch, the method test_notTrapped() is broken to two methods for PY3 and PY2. Docstrings are added for both the methods.
patch1.patch (1.7 KB) - added by sreepriya 22 months ago.
This patch breaks the method names test_notTrapped into two methods and docstring added for both. The Comment11 review changes are implemented.
patch3.patch (2.6 KB) - added by sreepriya 21 months ago.
The patch includes the comments given by rwall. Coding standards are followed, old test_trapped is removed and broken to two functions, if_PY3 is made closer.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 2 years ago by thijs

  • Keywords documentation added

comment:2 Changed 23 months ago by thijs

  • Cc thijs added

comment:3 Changed 22 months ago by sreepriya

  • Owner set to sreepriya

comment:4 Changed 22 months ago by sreepriya

Instead of the docstring "Making sure trap doesn't trap what it shouldn't", does the following docstring make sense ?
"Avoids trapping failures that are defined"

comment:5 Changed 22 months ago by exarkun

  • Keywords changed from easy, documentation to easy documentation

comment:6 Changed 22 months ago by tom.prince

I'm not sure if that is much better. Unfortunately, there isn't much documentation on what makes a good test docstring (see #6301, which links to this google+ post).

Generally a test docstring wants to be something like When <some thing is done>, <something else occurs>. Often <something is done> is a method being called. And <something else occurs> is generally what is captured by the assertion(s).

In this particular case, I think the method may actually want to be split up into two methods. One for the python 2 only assertion, and one for the python 2+3 assertion.

comment:7 Changed 22 months ago by sreepriya

If a new method is added, I must also make changes in the function names in the places where these tests are conducted right?

comment:8 Changed 22 months ago by sreepriya

Why do we need a separate assertion for python 2 ? Why is it that python 2+3 doesn't work for even python 2 ?

Changed 22 months ago by sreepriya

In the patch, the method test_notTrapped() is broken to two methods for PY3 and PY2. Docstrings are added for both the methods.

comment:9 Changed 22 months ago by sreepriya

  • Keywords review added
  • Owner changed from sreepriya to tom.prince

comment:10 Changed 22 months ago by sreepriya

  • Owner tom.prince deleted

comment:11 Changed 22 months ago by tom.prince

  • Keywords review removed
  • Owner set to sreepriya

Thanks for your contribution.

  1. There are a number of unrelated changes in this patch, and it doesn't apply cleanly to trunk. Also test_notTrapped_PY3 is weirdly indented.
  2. The method being tested here is Failure.trap, so the docstring should say something about the expected behaviour of that function. You should probably read the api documentation for that method, as well as the relevant howto. Also, the comments in the method are probably helpful.
  3. The method names don't follow the coding standard. Also, they test something slight different, and should be named accordingly.
  4. Rather than doing if _PY3 in the test, it should be done outside, setting .skip on the test, with a message describing why it is being skipped.
  5. (minor) There should be 2 blank lines between methods in a class.

Please resolve the above issues and resubmit for review.

Changed 22 months ago by sreepriya

This patch breaks the method names test_notTrapped into two methods and docstring added for both. The Comment11 review changes are implemented.

comment:12 Changed 22 months ago by sreepriya

  • Keywords review added
  • Owner sreepriya deleted

comment:13 Changed 22 months ago by sreepriya

  • Cc sreepriya1111@… added

comment:14 Changed 21 months ago by rwall

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

Reviewing...

comment:15 Changed 21 months ago by rwall

  • Author set to rwall
  • Branch set to branches/test-not-trapped-5952

(In [37643]) Branching to 'test-not-trapped-5952'

comment:16 Changed 21 months ago by rwall

  • Keywords review removed
  • Owner rwall deleted
  • Status changed from assigned to new

Code Review:

Thanks sreepriya for your latest patch.

I created a new branch for you and applied your patch. See

Here are some comments:

  1. Build Results seem ok
  2. There is trailing whitespace in your patch - please remove it.
  3. The lines in the new docstrings are too long. Please rewrap to < 80 columns.
  4. The news file was missing. I added one for you in the branch.
  5. The new test method names don't follow the coding standard. The first letter after "test_" should be lower case eg test_trappedAndReRaiseFailure. See existing test methods for examples.
  6. I don't think you addressed Tom Prince comments about the test docstrings. They are still not clear to me. Test docstrings should explain the expected behaviour of failure.trap. They currently seem to describe how the test works.
  7. The patch replaces the old test_notTrapped with two new tests but you haven't removed the old test.
  8. I think you should move the if not PY3 skip closer to the skipped test.
  9. Also add a more specific reason for skipping the test with PY3

Please resubmit your patch against the new branch.

Thanks again.

comment:17 Changed 21 months ago by rwall

Here's exarkun's explanation for the PY3 test skip:

17:46 < exarkun> Failure.trap is stu0000000000000pid
17:47 < exarkun> Yes, you cannot raise Failure on Python 3, so it doesn't.  And you should never have raised Failure on Python 2 anyway, so it's too 
                 bad that Failure does.
>>> class Foo:
...     def __init__(self, *args):
...             pass
... 
>>> raise Foo('test')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: exceptions must derive from BaseException

So instead of "This test works only with Python 2" say something like "In Python3, failure.trap raises the original Exception instead of a failure instance because Python3 can only raise BaseException subclasses."

Changed 21 months ago by sreepriya

The patch includes the comments given by rwall. Coding standards are followed, old test_trapped is removed and broken to two functions, if_PY3 is made closer.

comment:18 Changed 21 months ago by sreepriya

  • Keywords review added

comment:19 Changed 21 months ago by rwall

(In [37806]) apply patch3.patch from sreepriya. refs #5952

comment:20 Changed 21 months ago by rwall

(In [37808]) rewrap the skip message. refs #5952

comment:21 Changed 21 months ago by rwall

(In [37811]) the python3 skip test was referencing the wrong function and was in the wrong place. refs #5952

comment:22 Changed 21 months ago by rwall

(In [37812]) a better test method name and a clearer docstring. refs #5952

comment:23 Changed 21 months ago by rwall

(In [37813]) a better test method name and a clearer docstring. refs #5952

comment:24 Changed 21 months ago by rwall

Thanks sreepriya

I reviewed your latest attachment:patch3.patch and fixed some things
at the same time.

  • There was trailing whitespace and an unrelated deletion of a blank line - Fixed.
  • The python3 skip test was referencing the old test method name and using the class name within the class definition - Fixed.
  • I think you misunderstood my previous review comment about the test method docstrings. I've rewritten the docstrings and given the test methods clearer names (IMHO).
  • I simplified the test for trap raising the wrapped Exception and made it a Python3 only test.

Ready for review in log:branches/test-not-trapped-5952

comment:25 follow-up: Changed 21 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall
  1. You can probably just say "subclass of one of the expected", rather than having a parenthetical.
  2. The test incidentally checked that failure.Failure(), when catching a Failure actually creates a Failure with the right .value (i.e. the exception not the caught Failure). It would be good to add a test to do that. Although, perhaps it would be better to simply construct a failure with another. Or perhaps 2, a) construct a failure with a failure, b) .trap -> Failure().

comment:26 in reply to: ↑ 25 Changed 21 months ago by rwall

  • Keywords review added
  • Owner rwall deleted

Thanks for the review Tom.

I've responded to your comments below...

Replying to tom.prince:

  1. You can probably just say "subclass of one of the expected", rather than having a parenthetical.

Done.

  1. The test incidentally checked that failure.Failure(), when catching a Failure actually creates a Failure with the right .value (i.e. the exception not the caught Failure). It would be good to add a test to do that. Although, perhaps it would be better to simply construct a failure with another. Or perhaps 2, a) construct a failure with a failure, b) .trap -> Failure().

I added two new tests as suggested. But actually I think that the
second case is already handled in FindFailureTests.

Ready for another review in log:branches/test-not-trapped-5952

comment:27 Changed 21 months ago by tom.prince

  • Keywords review removed
  • Owner set to rwall

Looks good, please merge.

comment:28 Changed 21 months ago by rwall

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

(In [38052]) Merge test-not-trapped-5952: Improved tests and test documentation for Failure.trap.

Author: sreepriya, rwall
Reviewer: tom.prince, rwall
Fixes: #5952

Improved tests and test documentation for Failure.trap.

Note: See TracTickets for help on using tickets.