Opened 8 years ago

Closed 8 years ago

#1901 defect closed fixed (fixed)

misdefined test method kills trial with an unhandled exception

Reported by: exarkun Owned by:
Priority: highest Milestone:
Component: trial Keywords:
Cc: therve, jml Branch:
Author: Launchpad Bug:

Description

Like so:

exarkun@charm:~$ cat test_foo.py
from twisted.trial.unittest import TestCase

class Foo(TestCase):
    def test_foo():
        pass
exarkun@charm:~$ trial test_foo.py 
Running 1 tests.
test_foo
  Foo
    test_foo ...                                                        [ERROR]

===============================================================================
[ERROR]: test_foo.Foo.test_foo

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/bin/trial", line 24, in ?
    run()
  File "/home/exarkun/Projects/Twisted/trunk/twisted/scripts/trial.py", line 377, in run
    test_result = trialRunner.run(suite)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 503, in run
    result.printErrors()
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/reporter.py", line 277, in printErrors
    self._formatFailureTraceback)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/reporter.py", line 255, in _printResults
    self.write(formatter(*(content[1:])))
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/reporter.py", line 245, in _formatFailureTraceback
    fail.frames, frames = self._trimFrames(fail.frames), fail.frames
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/reporter.py", line 235, in _trimFrames
    last = newFrames[-1]
IndexError: list index out of range
exarkun@charm:~$ 

Change History (17)

comment:1 Changed 8 years ago by exarkun

Hit this again while writing tests for loopbackAsync. Here's the code that did it:

from twisted.trial.unittest import TestCase
from twisted.internet.protocol import Protocol
from twisted.internet.defer import Deferred
from twisted.internet.interfaces import IAddress
from twisted.protocols.loopback import loopbackAsync

class LoopbackAsyncTestCase(TestCase):
    def _hostpeertest(self, get, testServer):
        """
        Test one of the permutations of client/server host/peer.
        """
        class TestProtocol(Protocol):
            def makeConnection(self, transport):
                self.connected.callback(transport)

        if testServer:
            server = TestProtocol()
            d = server.connected = Deferred()
            client = Protocol()
        else:
            server = Protocol()
            client = TestProtocol()
            d = client.connected = Deferred()

        loopbackAsync(server, client)

        def connected(transport):
            host = getattr(transport, get)()
            self.failUnless(IAddress.providedBy(host))

        return d.addCallback(connected)


    def test_serverHost(self, get):
        """
        Test that the server gets a transport with a properly functioning
        implementation of L{ITransport.getHost}.
        """
        return self._hostpeertest("getHost", True)

Not sure if this is caused by the same bug or not.

comment:2 Changed 8 years ago by exarkun

Ah, nevermind. Of course it is the same case. I accidentally left the extra argument on test_serverHost.

comment:3 Changed 8 years ago by therve

  • Owner changed from jml to therve

comment:4 Changed 8 years ago by therve

  • Cc therve jml added
  • Keywords review added
  • Priority changed from normal to highest

Ready to review in test-arguments-1901.

I modified _trimFrames and add tests to the problem. The best as exarkun said would be to remove _trimFrames, but let's live with this for now.

comment:5 Changed 8 years ago by therve

  • Owner therve deleted

comment:6 follow-up: Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

The twisted.internet.defer import within the definition of the ContainMalformed class seems unnecessary. Also please add a docstring for that class describing how it is useful for the tests which use it.

Grammatically, the TestMalformedMethod docstring should read:

Test that trial manages when test methods don't have good signatures.

"trial" and "manages" need to agree; since trial is signular, manages gets the "s". If trial were replaced with a plural noun (eg, "test runners") then "manage" would be correct.

Likewise, "methods" and "do" (as it appears in the contraction "don't") need to agree for the same reason.

(I hope that kind of feedback is useful, if not, just let me know and I'll skip it next time :)

As a non-grammatical comment, I would suggest "correct" in the place of "good".

test_extraArg has a typo of "has" spelled "as".

In reporter.py, the added comment:

There is also another case, when the test method is bad defined and contains extra argument

Should read

There is also another case, when the test method is badly defined and contains extra arguments.

"badly" is the adverb form of the adjective "bad", which is required since it is modifying "defined", which is a verb, not a noun.

Otherwise the change seems reasonable.

comment:7 in reply to: ↑ 6 Changed 8 years ago by therve

  • Keywords review added
  • Owner therve deleted

Replying to exarkun:

The twisted.internet.defer import within the definition of the ContainMalformed class seems unnecessary.

It's used for deferredGenerator, because it changes the signature of the method.

Also please add a docstring for that class describing how it is useful for the tests which use it.

OK, done.

(I hope that kind of feedback is useful, if not, just let me know and I'll skip it next time :)

It is, I'm not as careful as I'd want to be :). I've update the docstrings according to your comments, thanks.

comment:8 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Oops, sorry. With respect to the defer import, I meant that it was unnecessary for it to be inside the class definition, instead of at the top of the module. :) Sorry for being unclear.

comment:9 Changed 8 years ago by therve

  • Keywords review added
  • Owner therve deleted

Sorry for being dumb :)

comment:10 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Looks good to merge now. :)

comment:11 Changed 8 years ago by therve

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

(In [19657]) Merge test-arguments-1901

Author: therve
Reviewer: exarkun
Fixes #1901

Fix a problem with misdefined test methods within trial: report the good error
message and don't kill trial.

comment:12 Changed 8 years ago by exarkun

  • Resolution fixed deleted
  • Status changed from closed to reopened

For some reason, this introduced 8 failures on Windows. :(

See r19662 or buildbot for details.

comment:13 Changed 8 years ago by therve

  • Keywords review added
  • Owner therve deleted
  • Status changed from reopened to new

That should be good now, for some reasons it needed a specific directory to operator.

For record, TestUntilFailure is dangerous because if you introduce a second test in the method it begin to fail the same way.

I try to launch a build on win32 slave.

comment:14 Changed 8 years ago by therve

The 2 builds on win32-py-2.4 and win32-py-2.5 has succeeded.

comment:15 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Some stuff I guess I missed last time:

  • os import at the top of test_runner.py should come before z.i import. in general, stdlib imports should come first, then third party imports, then twisted imports.
  • I think it's good to give each test its own directory, but I think the fact that this is /required/ for the tests to pass indicates a bug in trial. In particular, it seems as though _tearDownLogging isn't being called at all the right times. Perhaps this merits another ticket?

Aside from those issues (and the 2nd one isn't even a blocker for this), looks good to merge, so please do after the import order is fixed.

comment:16 Changed 8 years ago by therve

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

(In [19711]) Merge test-arguments-1901

Author: therve
Reviewer: exarkun
Fixes #1901

Fix a problem with misdefined test methods within trial: report the good error
message and don't kill trial.

comment:17 Changed 4 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.