RemotedTestCase.addCleanup exists but fails under Python 2.7

[Andrew Bennetts]

bzr at one point has this code:

        # Only check for thread leaks if the test case supports cleanups
        addCleanup = getattr(test, "addCleanup", None)
        if addCleanup is not None:
            addCleanup(self._check_leaked_threads, test)

When 'test' is a RemotedTestCase and when run under Python 2.7, this fails, because: RemotedTestCase inherits from unittest.TestCase (and thus inherits the addCleanup method), but does not upcall to TestCase's __init__, so self._cleanups isn't initialised. Even if it did upcall, addCleanup wouldn't be respected because RemotedTestCase completely overrides run. The way this manifests itself is that 'bzr selftest --parallel=fork' hangs almost immediately.

I think the solution is probably to stop inheriting from TestCase: it ought to be enough to simply provide the same interface, and the friction with TestCase's __init__ is too great for RemotedTestCase to sensibly reuse it.

[Robert Collins (lifeless)]

On Tue, Sep 14, 2010 at 12:05 PM, Andrew Bennetts
<email address hidden> wrote:
> Public bug reported:
>
> bzr at one point has this code:
>
>        # Only check for thread leaks if the test case supports cleanups
>        addCleanup = getattr(test, "addCleanup", None)
>        if addCleanup is not None:
>            addCleanup(self._check_leaked_threads, test)

Thats buggy code though.

[Andrew Bennetts (spiv)]

Robert Collins wrote:
> > bzr at one point has this code:
> >
> >        # Only check for thread leaks if the test case supports cleanups
> >        addCleanup = getattr(test, "addCleanup", None)
> >        if addCleanup is not None:
> >            addCleanup(self._check_leaked_threads, test)
>
> Thats buggy code though.

I'm not surprised. Care to elaborate though? (I think I have a fair idea
why, but it'd prefer not to guess.)

Regardless, RemotedTestCase inheriting from TestCase without up-calling
seems pretty fragile, and likely to be a source of trouble as the standard
unittest.TestCase implementation keeps evolving.

[Robert Collins (lifeless)]

On Tue, Sep 14, 2010 at 7:27 PM, Andrew Bennetts
<email address hidden> wrote:
> Robert Collins wrote:
>> > bzr at one point has this code:
>> >
>> >        # Only check for thread leaks if the test case supports cleanups
>> >        addCleanup = getattr(test, "addCleanup", None)
>> >        if addCleanup is not None:
>> >            addCleanup(self._check_leaked_threads, test)
>>
>> Thats buggy code though.
>
> I'm not surprised.  Care to elaborate though?  (I think I have a fair idea
> why, but it'd prefer not to guess.)

TestCase offers two contracts:
 - the runner contract (run, debug, id, description, countTests)
 - the testcase contract (only for use by a subclass on self)

The code that bzr has is not in the runner contact, and by definition
not running under the testcase contract.

So its being fundamentally naughty.

> Regardless, RemotedTestCase inheriting from TestCase without up-calling
> seems pretty fragile, and likely to be a source of trouble as the standard
> unittest.TestCase implementation keeps evolving.

This is the first issue seen, subclassing matters to some clients
(also buggy) and its not wrong to not upcall when you don't -call at
all-.

The bzr code path shown will break with other third party
implementations as well - it has nothing to do with upcalling per se.

[Martin Packman (gz)]

Marking as also affects bzr as the opinion is the bzr code is wrong. I asked about this in code review but got no feedback, suggestions welcome.

[Martin Packman (gz)]

With natty people have started caring about 2.7 so better resolve this now.

[Jonathan Lange (jml)]

It's not clear what the status of this bug is in subunit. Robert seems to imply that it's not a bug, but it's hard to gauge.

[Robert Collins (lifeless)]

I'm marking this invalid because the subclassing is deliberate - there are type sniffers out there and merely meeting the contract wasn't enough in the past.