testtools change broke BZR_TEST_PDB

[Martin Pool]

After the change to testtools, BZR_TEST_PDB=1 does go into the debugger, but looking at the wrong stack, so it is no longer useful.

Perhaps this should in fact be fixed by moving that feature into testtools.

Example:

mbp@grace% BZR_TEST_PDB=1 ./bzr --no-plugins selftest Remote -1
bzr selftest: /home/mbp/bzr/trivial/bzr
   /home/mbp/bzr/trivial/bzrlib
   bzr-2.1.0dev5 python-2.6.4 Linux-2.6.31-17-generic-x86_64-with-Ubuntu-9.10-karmic

> /home/mbp/lib/python/testtools/testresult/real.py(377)addError()ocal_remote
-> return self.decorated.addError(test, details=details)
(Pdb) bt
> /home/mbp/lib/python/testtools/testresult/real.py(377)addError()
-> return self.decorated.addError(test, details=details)
(Pdb) c

======================================================================
ERROR: test_branch_local_remote (bzrlib.tests.blackbox.test_branch.TestRemoteBranch)
----------------------------------------------------------------------
_StringException: Text attachment: log
------------
1.876 creating repository in file:///tmp/testbzr-I7x_n5.tmp/.bzr/.
1.879 creating branch <bzrlib.branch.BzrBranchFormat7 object at 0x7f1e66583210> in file:///tmp/testbzr-I7x_n5.tmp/.bzr/
1.885 trying to create missing lock '/tmp/testbzr-I7x_n5.tmp/.bzr/checkout/dirstate'
1.885 opening working tree '/tmp/testbzr-I7x_n5.tmp'
------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/home/mbp/lib/python/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/home/mbp/lib/python/testtools/testcase.py", line 341, in _run_setup
    self.setUp()
  File "/home/mbp/bzr/trivial/bzrlib/tests/blackbox/test_branch.py", line 395, in setUp
    tree = self.make_branch_and_tree('branch')
  File "/home/mbp/bzr/trivial/bzrlib/tests/__init__.py", line 2600, in make_branch_and_tree
    b = self.make_branch(relpath, format=format)
  File "/home/mbp/bzr/trivial/bzrlib/tests/__init__.py", line 2320, in make_branch
    repo = self.make_repository(relpath, format=format)
  File "/home/mbp/bzr/trivial/bzrlib/tests/__init__.py", line 2348, in make_repository
    made_control = self.make_bzrdir(relpath, format=format)
  File "/home/mbp/bzr/trivial/bzrlib/tests/__init__.py", line 2326, in make_bzrdir
    maybe_a_url = self.get_url(relpath)
  File "/home/mbp/bzr/trivial/bzrlib/tests/__init__.py", line 2241, in get_url
    base = self.get_server().get_url()
  File "/home/mbp/bzr/trivial/bzrlib/tests/__init__.py", line 2209, in get_server
    self.start_server(self.__server, self.get_vfs_only_server())
  File "/home/mbp/bzr/trivial/bzrlib/tests/__init__.py", line 1022, in start_server
    transport_server.start_server(backing_server)
TypeError: start_server() takes exactly 1 argument (2 given)
------------

[Robert Collins (lifeless)]

On Thu, 2010-01-07 at 01:15 +0000, Martin Pool wrote:
> Public bug reported:
>
> After the change to testtools, BZR_TEST_PDB=1 does go into the debugger,
> but looking at the wrong stack, so it is no longer useful.
>
> Perhaps this should in fact be fixed by moving that feature into
> testtools.

Its probably one of the unaltered bits of code - a reporter that takes
(self, test, err) rather than self, test, err=None, details=None) -
changing to that style will avoid testtools encountering an error and
should fix the feature.

-Rob

[Robert Collins (lifeless)]

As far as testtools is concerned, this is a wishlist request.

[Martin Packman (gz)]

Fixing this isn't as simple as adapting to the testtools api changes unfortunately. The way testtools runs cases differs significantly from how unittest does it, and addError/addFailure are called outside the exception context, after all the cleanups have run, so pdb has no traceback to be looking at.

I'll fix the bzr side of this, making testtools not break the expectation we can get the right stack from sys.exc_info is harder.

[Robert Collins (lifeless)]

@mgz, I'm not sure what changes are needed in testtools to fix this bug. Could you expand on that?

[Martin Packman (gz)]

Hypothetically, saving the actual exception on the test case, rather than just a stringified traceback, then reraising over addError so calling sys.exc_info() from there will give the original exception context. In practice, testtools is designed around separating your result handling code from the test case run so you can transparently have subunit style things work with only serialisable information passed across.

[Robert Collins (lifeless)]

Ok. So my suggestion then is that FOO_TEST_PDB is a test runner responsibility, and we can ask two questions:
 - should testtools proffer this itself [yes! - thats what 'debug' vs 'run' on TestSuite/TestCase is for].
 - should it be possible, and how, for extension code to implement this feature?

The latter question is interesting, but the former is something we can and should do directly.