Opened 5 years ago

Closed 5 years ago

#6282 release blocker: regression closed fixed (fixed)

Trial tracebacks are not properly trimmed since 12.3.0

Reported by: ralphm Owned by: therve
Priority: normal Milestone: Python-3.x
Component: trial Keywords:
Cc: Jonathan Lange Branch: branches/async-trim-frames-6282
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

Due to the porting effort of trial to Python 3, tracebacks in the trial reporter are no longer properly trimming tracebacks:

from twisted.trial import unittest

class Test(unittest.TestCase):

    def test_it(self):
        raise Exception()

yields:

[..]
[ERROR]
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/twisted/internet/defer.py", line 138, in maybeDeferred
    result = f(*args, **kw)
  File "/usr/lib/python2.7/dist-packages/twisted/internet/_utilspy3.py", line 41, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/usr/lib/python2.7/dist-packages/twisted/internet/_utilspy3.py", line 37, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/ralphm/work/wokkel/trunk/q.py", line 6, in test_it
    raise Exception()
exceptions.Exception:
[..]

This is due to importing runWithWarningsSuppressed from _utilspy3 instead of util and an additional frame there. This makes the frames not being trimmed by Reporter._trimFrames.

Related tickets: #6033 and #6235.

Change History (15)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

comment:2 Changed 5 years ago by therve

Owner: set to therve

comment:3 Changed 5 years ago by therve

Author: therve
Branch: branches/async-trim-frames-6282

(In [36988]) Branching to 'async-trim-frames-6282'

comment:4 Changed 5 years ago by therve

Keywords: review added
Owner: therve deleted

Nice catch, fix ready to review.

comment:5 Changed 5 years ago by Jean-Paul Calderone

Milestone: Python-3.x

comment:6 Changed 5 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:7 Changed 5 years ago by Jean-Paul Calderone

Can you comment on why the branch changes the assertions made in test_hiddenException? It seems like that's a general improvement (?) in behavior, rather than a fix for a regression.

comment:8 Changed 5 years ago by Jean-Paul Calderone

Owner: Jean-Paul Calderone deleted
Status: assignednew

comment:9 Changed 5 years ago by therve

Sure: in r35861 which introduced the regression by creating utilspy3, the test was changed in a wrong way to comply to the new behavior, instead of fixing it.

comment:10 Changed 5 years ago by Jean-Paul Calderone

Sure: in r35861 which introduced the regression by creating utilspy3, the test was changed in a wrong way to comply to the new behavior, instead of fixing it.

Crap. :( Thanks for catching and fixing that.

comment:11 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to therve
  1. There are a number of missing docstrings. TestAsynchronousFail. (It would be nice to add them to the other methods of TracebackHandling that got touched as well.
  2. The docstring for test_exception isn't clear. The test it is running is synchronous (just raises an exception). I think you mean a test using twisted.trial.unittest.TestCase which *can* handle asynchronous tests.
  3. The comment in _trimFrames needs to be updated and turned into a proper docstring.
  4. What is going to happen in _trimFrames is the traceback doesn't look as expected? We should detect this and do something, as well as test this case. (Doing this may have caught the regression.

Please address the above issues and re-submit.

Note for archaeologists:

  • SynchronousTestCase uses twisted.python.util.runWithWarningsSuppressed (which doesn't support deferreds), and TestCase uses twisted.internet.utils.runWithWarningsSuppresed which does).

comment:12 Changed 5 years ago by therve

Keywords: review added
Owner: therve deleted

I've fixed 1-3. I don't think we can do anything for 4, as there are too many different cases that can happen. We already had a test which ought to have caught the regression.

comment:13 Changed 5 years ago by Tom Prince

Owner: set to therve

A couple of last things:

  1. As discussed on IRC, we should document that _trimFrames gets called with tracebacks other than the ones we expect (primarly when using testcase not from trial).
  2. _trimFrames should use C{} and L{} for code and references, and have @param and @return
  3. getErrorFrames should have @param and @return.

Please address the above issues and merge.

comment:14 Changed 5 years ago by Tom Prince

Keywords: review removed

comment:15 Changed 5 years ago by therve

Resolution: fixed
Status: newclosed

(In [37172]) Merge async-trim-frames-6282

Author: therve Reviewer: tom.prince Fixes: #6282

Fix a regression in Reporter._trimFrames where utils.runWithWarningsSuppressed unexpectedly appeared.

Note: See TracTickets for help on using tickets.