Opened 2 years ago

Closed 21 months ago

#5953 enhancement closed fixed (fixed)

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

Reported by: itamar Owned by: thijs
Priority: normal Milestone:
Component: core Keywords:
Cc: thijs Branch: branches/failure-tests-5953
(diff, github, buildbot, log)
Author: thijs Launchpad Bug:

Description

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.

Change History (12)

comment:1 Changed 22 months ago by thijs

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

comment:2 Changed 22 months ago by thijs

  • Author set to thijs
  • Branch set to branches/failure-tests-5953

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

comment:3 Changed 22 months ago by thijs

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

comment:4 Changed 22 months ago by thijs

  • Cc thijs added
  • Keywords review added
  • Owner thijs deleted
  • Status changed from assigned to new

Forced a build.

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

Please review.

comment:5 follow-up: Changed 22 months ago by tom.prince

  • Keywords review removed
  • Owner set to thijs

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):

  1. It would be nice if there were some test that Failure is actually capturing the right traceback.
  2. It would be nice if we could make some explicit assertions about what the frame lines printed look like.
  3. 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.

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

comment:6 Changed 22 months ago by thijs

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

comment:7 in reply to: ↑ 5 Changed 22 months ago by thijs

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

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.

comment:8 Changed 22 months ago by tom.prince

  • Keywords review removed
  • Owner changed from tom.prince to thijs

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.

comment:9 Changed 21 months ago by 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.

comment:10 Changed 21 months ago by thijs

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

comment:11 Changed 21 months ago by thijs

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

comment:12 Changed 21 months ago by thijs

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

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

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

Note: See TracTickets for help on using tickets.