Split up and improve a couple of printing tests for twisted.python.failure

[itamar]

test_printingSmokeTest and test_printingCapturedVarsSmokeTest should each be split into separate tests for each method they are calling, and also need some more serious assertions added.

[thijs]

(In [37021]) Branching to 'failure-tests-5953'

[thijs]

(In [37022]) split tests, add news file. refs #5953

[thijs]

Forced a  build.

1. refactored and split the tests
2. added a test for format_frames's ValueError

Please review.

[tom.prince]

Thanks for working on this. Here are a few comments, from a first pass over the code. twisted.python.failure has some rather subtle code, so there may end up being more comments, on further consideration.

1. In docstrings, when refering to parameters, use C{}' instead of L{}. And when using L{}, you should use either something that would be a valid reference, or a full path, e.g L{failure.Failure.trap} or L{twisted.python.failure.Failure.trap}`. See the  epytext manual for details. Unfortunately, we don't currently run tests through api-docs on the buildbot.
2. assertCorrectTraceback seems incorrectly named or poorly documented.
   • After staring at the code for a while, it occured to me that we want to do is assert that the traceback has a bunch of lines, some of which we don't know. I wonder if the tests would be clearer if we had a function that checked if the list of lines matched our expected lists of lines, with a placeholder for matching lines we don't want to assert about.
3. Tests docstrings should describe the expected behavior (see #6301 or jml's  post on Google+). (re: assertDetailedTraceback and assertBriefTraceback for example)
   • I realize that this information is partly included in the test_* methods that call them, but the docstings for the assert* methods are unhelpful.
4. The printing tests shouldn't duplicate the logic of failure, to generate the strings it is checking. For example, we know that the error is always going to be 'ZeroDivisionError' and not pickled, so those can be hard-coded in the tests.
5. assertDetailedTraceback
   1. We should be able to say exactly what the line containing our local variable is, and it will be in linesWithVars.
   2. We should be able to check that frames are printed correctly.
6. assertBriefTraceback
   1. We should check that the exception type and message is properly included.

Please re-submit after addressing the above issues.

Some further thoughts (this would probably be a good follow-up ticket to file):

6. It would be nice if there were some test that Failure is actually capturing the right traceback.
7. It would be nice if we could make some explicit assertions about what the frame lines printed look like.
8. It would be nice if we could test printing independently of the creation logic of Failure.__init__.

(8) could partially be handled by making assertions about frames that we know will be in the traceback (i.e. getDivisionFailure and assert*Traceback). And, if we passed explicit traceback into Failure, or a fake one like t.p.failure._Traceback, that might be enough separation to handle 7+8.

9. There are no tests for serializing Failures. Please file a ticket for that.

[thijs]

(In [37186]) address review comments. refs #5953

[thijs]

Replying to tom.prince:

1. In docstrings, when refering to parameters, use C{}' instead of L{}. And when using L{}, you should use either something that would be a valid reference, or a full path, e.g L{failure.Failure.trap} or L{twisted.python.failure.Failure.trap}`. See the  epytext manual for details. Unfortunately, we don't currently run tests through api-docs on the buildbot.

Fixed.

1. assertCorrectTraceback seems incorrectly named or poorly documented.

Renamed it..

* After staring at the code for a while, it occured to me that we want to do is assert that the traceback has a bunch of lines, some of which we don't know. I wonder if the tests would be clearer if we had a function that checked if the list of lines matched our expected lists of lines, with a placeholder for matching lines we don't want to assert about.

Not sure how i'd go about this..

1. Tests docstrings should describe the expected behavior (see #6301 or jml's  post on Google+). (re: assertDetailedTraceback and assertBriefTraceback for example) * I realize that this information is partly included in the test_* methods that call them, but the docstings for the assert* methods are unhelpful.

Added descriptions of the format of the desired traceback to the assert* methods.

1. The printing tests shouldn't duplicate the logic of failure, to generate the strings it is checking. For example, we know that the error is always going to be 'ZeroDivisionError' and not pickled, so those can be hard-coded in the tests.

The pickled version is generated by the test_printDetailedTracebackCapturedVarsCleaned test, that's why the check is there.

1. assertDetailedTraceback 1. We should be able to say exactly what the line containing our local variable is, and it will be in linesWithVars.

Fixed. Differs when cleanFailure is called.

1. We should be able to check that frames are printed correctly.

This is going to be tricky across various platforms, with/without pdb, pyc vs py, etc. Any suggestions?

1. assertBriefTraceback 1. We should check that the exception type and message is properly included.

On python2.6 it's:

<type 'exceptions.ZeroDivisionError'>: float division

on 2.7 it's:

<type 'exceptions.ZeroDivisionError'>: division by zero

So i've included the first part in the check.

[tom.prince]

Thanks for your work on this.

* After staring at the code for a while, it occured to me that we want to do is assert that the traceback has a bunch of lines, some of which we don't know. I wonder if the tests would be clearer if we had a function that checked if the list of lines matched our expected lists of lines, with a placeholder for matching lines we don't want to assert about.

Not sure how i'd go about this..

psuedo-python:

if expected[0] is SKIP:
  skip = True
  expected.pop(0)
else:
  skip = False
for line in actualLines:
  if expected[0].match(line):
    expected.pop(0)
  elif not skip:
    self.fail("doesn't match")
}}
But that probably deserves its own ticket.


>>  We should be able to check that frames are printed correctly.
> This is going to be tricky across various platforms, with/without pdb, pyc vs py, etc. Any suggestions? 
I originally wrote:
We know at least two frames (and we could arrange to know three) that will show up every time:
{{{
/home/tomprince/src/twisted/twisted/test/test_failure.py:299: test_printDetailedTraceback(...)
/home/tomprince/src/twisted/twisted/test/test_failure.py:176: assertDetailedTraceback(...)
 [Capture of Locals and Globals disabled (use captureVars=True)]
--- <exception caught here> ---
/home/tomprince/src/twisted/twisted/test/test_failure.py:39: getDivisionFailure(...)
 [Capture of Locals and Globals disabled (use captureVars=True)]
exceptions.ZeroDivisionError: division by zero
*--- End of Failure #29 ---
}}}
We clearly don't want to match all of that (i.e. ignore the path prefix and content (but not format) of the line number), but we can check the file and method name.

Please commit after addressing the following points, and file tickets for 6+7+8 from my first review.
  1. I realize I wasn't clear, and there isn't documentation on when to use `L{}` vs `C{}` (#6318). Parameter names should always be `C{}` (there is nothing to link to for them in any case).
  1. There is some trailing whitespace in `assertDetailedTraceback`'s docstring.

[tom.prince]

(reposting the tail, for formatting)

We should be able to check that frames are printed correctly.

This is going to be tricky across various platforms, with/without pdb, pyc vs py, etc. Any suggestions?

I originally wrote: We know at least two frames (and we could arrange to know three) that will show up every time:

/home/tomprince/src/twisted/twisted/test/test_failure.py:299: test_printDetailedTraceback(...)
/home/tomprince/src/twisted/twisted/test/test_failure.py:176: assertDetailedTraceback(...)
 [Capture of Locals and Globals disabled (use captureVars=True)]
--- <exception caught here> ---
/home/tomprince/src/twisted/twisted/test/test_failure.py:39: getDivisionFailure(...)
 [Capture of Locals and Globals disabled (use captureVars=True)]
exceptions.ZeroDivisionError: division by zero
*--- End of Failure #29 ---

We clearly don't want to match all of that (i.e. ignore the path prefix and content (but not format) of the line number), but we can check the file and method name.

Please commit after addressing the following points, and file tickets for 6+7+8 from my first review.

1. I realize I wasn't clear, and there isn't documentation on when to use L{} vs C{} (#6318). Parameter names should always be C{} (there is nothing to link to for them in any case).
2. There is some trailing whitespace in assertDetailedTraceback's docstring.

[thijs]

(In [37281]) address review comments, refs #5953

[thijs]

(In [37282]) fix for py3.3, refs #5953

[thijs]

(In [37305]) Merge failure-tests-5953: Split up and improve the printings tests for t.p.failure.

Author: thijs Reviewer: tom.prince Fixes: #5953