Improve docstring for test_notTrapped in twisted.test.test_failure

[itamar]

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

[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"

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

[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?

[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 ?

[sreepriya]

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

[tom.prince]

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.

[sreepriya]

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

[rwall]

Reviewing...

[rwall]

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

[rwall]

Code Review:

Thanks sreepriya for your latest patch.

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

• log:branches/test-not-trapped-5952

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.

[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."

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

[rwall]

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

[rwall]

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

[rwall]

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

[rwall]

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

[rwall]

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

[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

•  Build results

[tom.prince]

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

[rwall]

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.

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

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

•  Build Results

[tom.prince]

Looks good, please merge.

[rwall]

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