Merge lp:~gz/bzr/cleanup_testcases_by_collection_613247 into lp:bzr

[Martin Packman]

This branch ensures that after each test case is run, they are cleaned up rather than lingering until the entire suite completes. On my machine this reduces the memory usage of a full test run from ~575MB to ~140MB, which prevents OOM related errors towards the end. In fact, the first revision is a sufficient fix, however Robert's intention in removing an earlier hack was to allow each test case instance to be deallocated after being run. To make that happen, a lot of code needs to be touched to make sure decrementing the refcount after the test completes will actually result in its deallocation.

The most visible change is there will now be a whine printed to the output stream if a testcase stays alive. As this style has proved rather too easy to ignore, I may try an alternative approach once the remaining ones are fixed. The test result has already been recorded by this point, but adding a new stub test to run next that fails may be an option.

As written, the code relies on refcounting working, so cycles need to be avoided. The alternative is to run a full gc collection after every test, which slow enough to be worth avoiding.

Some changes to testtools are needed to avoid some needless exc_info cycles, but even with that fixed we're hamstrung on a few fronts. Firstly, cases with errors or failures are kept in lists on the result object. In testtools, there is also a list for expected failures but bazaar doesn't use this one. However, the new unittest _ExpectedFailure gets the old exc_info which creates cycles.

The last change on the branch guts the TestDecorator classes. This is needed because otherwise they were keeping extra references to test cases alive, but I found the way they worked previously overly complicated anyway. I'd like to simplify them further if there's not a good backward compatibility reason for keeping the old style.

The branch needs a bit more work still, about a hundred tests still need poking to avoid cycles, and needs a NEWS entry and so on. I'd like some feedback on if people think this is a worthwhile change over just stripping the __dict__ and if there are any other issues I've not realised.

[Robert Collins (lifeless)]

So, I think this is more correct; fixing cycles is pretty important
for us in other ways : I like that we're finding latent issues here.

I'm not worried about the references to test cases in errors/failures
lists : they are a benefit (and the __dict__ stripping was a _REAL_
pain there in the past).

-Rob

[Martin Packman (gz)]

> I'm not worried about the references to test cases in errors/failures
> lists : they are a benefit (and the __dict__ stripping was a _REAL_
> pain there in the past).

Indeed that mostly won't be an issue (and can sometimes be useful), my one worry is the occasional breaks-everything rev will then go back to swapping my machine to death. That's why the current implementation still wipes all uncollected tests, but I'm okay with the idea of removing that once everything else is sorted.

A few things I forgot to mention in the initial review comment.

The lock decorators now use this horribly obnoxious pattern:

    self.lock_read()
    try:
        result = unbound(self, *args, **kwargs)
    except:
        import sys
        exc_info = sys.exc_info()
        try:
            self.unlock()
        finally:
            try:
                raise exc_info[0], exc_info[1], exc_info[2]
            finally:
                del exc_info
    else:
        self.unlock()
        return result

If anyone can think of a sane way of spelling that, please say.

Cycles needing to be avoided will have to be documented if we go with this, and can be annoying to understand without decent knowledge of python internals. I've been using a rough reference tracking function to fix things, but it's not worth landing. Is there any package that can help contributors diagnose things like this?

Would there be any benefit in running all tests with gc disabled by default, and only enabling it for those that explicitly require it?

Is there a better way of telling if the test you just ran failed other than comparing failure counts on the result object?

[Robert Collins (lifeless)]

> Is there a better way of telling if the test you just ran failed other than comparing failure counts on the result object?

Where do you need to know this?

[Martin Packman (gz)]

> > Is there a better way of telling if the test you just ran failed other than
> comparing failure counts on the result object?
>
> Where do you need to know this?

It has to be outside all the normal methods unfortunately. Even stopTest is too early, as that's passed a reference to the test case, so clearly the caller is going to be keeping it alive. Instead I added the collection accounting in TestSuite.run which was already popping testcases from its internal list, now it uses weakref to check that they really expire after being run.

I've put up <lp:~gz/testtools/avoid_exc_info_cycles> with the testtools changes I needed.

[Vincent Ladeuil (vila)]

Hmm, I appreciate the effort to reduce the memory footprint, but unfortunately this seems far from landable :-/

BZR_PLUGIN_PATH=-site ./bzr selftest --parallel=fork
bzr selftest: /home/vila/src/bzr/reviews/cleanup_testcases_by_collection_613247/bzr
   /home/vila/src/bzr/reviews/cleanup_testcases_by_collection_613247/bzrlib
   bzr-2.2b3 python-2.6.5 Linux-2.6.32-24-generic-x86_64-with-Ubuntu-10.04-lucid

----------------------------------------------------------------------
Ran 8 tests in 0.489s

OK
2 tests skipped

Huh ? Where are my tests gone ? (Strong suspicion around your modification of __iter__)

Ran 24856 tests in 1050.614s

FAILED (failures=2, errors=2, known_failure_count=48)

Including failures for bb.test_log so your fix is wrong there.

On the overall, with a superficial reading, the way you avoid ref cycles seem hard to understand
so very likely to be re-introduced. How about breaking the ref-cycles with addCleanup calls instead
(by deleting the offending attributes) ?

Now, a way to track the uncollectable test case sounds like a good way to progress but given the amount of work, could you instead devise a patch where the check depends on some option ?
Once we reach 0 we can toggle the default value for this option.

For the run above, I also get ~4000 Uncollected test cases, far too much to land this patch :-/

review: Needs Fixing

[Martin Packman (gz)]

Thanks for looking at this Vincent.

> Huh ? Where are my tests gone ? (Strong suspicion around your modification of
> __iter__)

I suspect this is related to --parallel which I thought might be affected but didn't have any easy way of testing on my machine, I'll take another look at that code.

> Including failures for bb.test_log so your fix is wrong there.

Gah, how did I miss those...

> On the overall, with a superficial reading, the way you avoid ref cycles seem
> hard to understand
> so very likely to be re-introduced. How about breaking the ref-cycles with
> addCleanup calls instead
> (by deleting the offending attributes) ?

Yes, that it's hard to understand is a big concern. At least this patch, with retroactively changing a bunch of tests in subtle ways, is the worst part of it. It's much easier to adjust the test you're writing right now because you get told there's a cycle. So, moving in the direction of actually causing a failure once all existing issues are fixed would I think prevent regressions without being too taxing on contributors.

Note, that sometimes cycles do have to be avoided, because it's easier than trying to break them later, particularly with exc_info and closures where it's internal objects pointing at each other rather than public TestCase attributes.

> Now, a way to track the uncollectable test case sounds like a good way to
> progress but given the amount of work, could you instead devise a patch where
> the check depends on some option ?
> Once we reach 0 we can toggle the default value for this option.

Well, the "print junk to stdout" is my less intrusive option, but the aim is to get it to 0 before landing. However...

> For the run above, I also get ~4000 Uncollected test cases, far too much to
> land this patch :-/

This could well be just needing the testtools exc_info cycle cleanup as well, branch for that linked in previous comment.

...getting 4000 complaints from not having the testtools change probably does mean this needs to land quieter, making everyone patch testtools or be spammed isn't polite.

[Vincent Ladeuil (vila)]

> Thanks for looking at this Vincent.
>
> > Huh ? Where are my tests gone ? (Strong suspicion around your modification
> of
> > __iter__)
>
> I suspect this is related to --parallel which I thought might be affected but
> didn't have any easy way of testing on my machine, I'll take another look at
> that code.

Clearly --parallel specific and at least one __iter__ you've modified should be involved.

>
> > Including failures for bb.test_log so your fix is wrong there.
>
> Gah, how did I miss those...

I'm pretty sure the way the log formatter are registered requires that a single instance is used
and you've changed that. In this case I think breaking the cycle with an addCleanup should be ok.

>
> > On the overall, with a superficial reading, the way you avoid ref cycles
> seem
> > hard to understand
> > so very likely to be re-introduced. How about breaking the ref-cycles with
> > addCleanup calls instead
> > (by deleting the offending attributes) ?
>
> Yes, that it's hard to understand is a big concern. At least this patch, with
> retroactively changing a bunch of tests in subtle ways, is the worst part of
> it. It's much easier to adjust the test you're writing right now because you
> get told there's a cycle.

I understand that, but avoiding the cycle may make the code less clear than breaking it during tearDown,
especially if there is an accompanying comment ;)

> So, moving in the direction of actually causing a
> failure once all existing issues are fixed would I think prevent regressions
> without being too taxing on contributors.

Yup, +1 on that.

> Note, that sometimes cycles do have to be avoided, because it's easier than
> trying to break them later, particularly with exc_info and closures where it's
> internal objects pointing at each other rather than public TestCase
> attributes.

No problem with that, but again adding a comment will help document such cases (so people
can learn from them) and avoid regressions.

> > Now, a way to track the uncollectable test case sounds like a good way to
> > progress but given the amount of work, could you instead devise a patch
> where
> > the check depends on some option ?
> > Once we reach 0 we can toggle the default value for this option.
>
> Well, the "print junk to stdout" is my less intrusive option, but the aim is
> to get it to 0 before landing. However...

I'm fine with that as long as an option control it, there are precedents, look for the lock checks.

> > For the run above, I also get ~4000 Uncollected test cases, far too much to
> > land this patch :-/
>
> This could well be just needing the testtools exc_info cycle cleanup as well,
> branch for that linked in previous comment.

Right, testing with your testtools patch, I still get ~2000 uncollected tests.

>
> ...getting 4000 complaints from not having the testtools change probably does
> mean this needs to land quieter, making everyone patch testtools or be spammed
> isn't polite.

Yup, that's why I suggest an option to control the output that could be stay off by default
until you reach 0 uncollected tests which you can then target in several steps.

In fact, ideally, the first step could be a patch that just imp...

[Martin Packman (gz)]

Quick followup on some points from review last time round.

> > > Huh ? Where are my tests gone ? (Strong suspicion around your modification
> > of
> > > __iter__)
> >
> > I suspect this is related to --parallel which I thought might be affected
> but
> > didn't have any easy way of testing on my machine, I'll take another look at
> > that code.
>
> Clearly --parallel specific and at least one __iter__ you've modified should
> be involved.

This was a combination of subunit playing very differently to the normal selftest result object, and bug 625551 meaning the errors from the children were silently eaten so I didn't notice.

> I'm pretty sure the way the log formatter are registered requires that a
> single instance is used
> and you've changed that. In this case I think breaking the cycle with an
> addCleanup should be ok.

Made this change.

> I'm fine with that as long as an option control it, there are precedents, look
> for the lock checks.

I'm still avoiding this for the moment, so people with 8GB ram at least get some console spam before they completely break my box.

> Right, testing with your testtools patch, I still get ~2000 uncollected tests.

The testtools change landed for 0.9.7 and I'd be interested in a new count. Should now not break with subunit which should help.

[Andrew Bennetts (spiv)]

Martin [gz] wrote:
[...]
> As written, the code relies on refcounting working, so cycles need to be
> avoided. The alternative is to run a full gc collection after every test,
> which slow enough to be worth avoiding.

Another alternative: why not a single full gc collection at the end of the
entire run, and then report any test cases that are unexpectedly still
alive?

I don't think you should assume that “uncollected at this instant” implies
“can't be collected yet”. And the real issue is uncollectable objects, not
objects that have fractionally outlived their usefulness, AFAICS.

The point is that if a VM chooses to allow say 20 or even 200 used test case
instances to be alive simultaneously before collecting them, then that's
fine. It'll still keep the memory ceiling reasonably low rather than
growing in proportion to the number of tests. So requiring that a test case
lifetime is strictly minimal is too strong a requirement, and I'm not sure
it's worth paying the price for it. The price is either assuming a
refcounting VM (and the fairly extreme senstivity to refcounting differences
due to different versions of dependencies like testtools or even the Python
stdlib), or forcing a full GC after every test. I agree that the latter is
too much, but I also think the former isn't worth paying either.

To be clear, I don't mind (small) contortions to make most or even all tests
be collected promptly via refcounting, under common conditions. I do mind
emitting warnings when that fails, because I think the noise will outweigh
the benefits.

I'd be very happy to have an optional flag to selftest to force a full GC
after every test (or more creatively, after every N tests, then once a leak
is found re-run the last N individually to pinpoint it...). And maybe even
an optional flag to warn if the refcount-based attempt to release the test
case doesn't release it. It's good to have tools to help developers
diagnose memory issues. I just don't believe these tools should be on all
the time unless they are very cheap and very accurate.

> The branch needs a bit more work still, about a hundred tests still need
> poking to avoid cycles, and needs a NEWS entry and so on. I'd like some
> feedback on if people think this is a worthwhile change over just
> stripping the __dict__ and if there are any other issues I've not
> realised.

Why not strip __dict__ as well? Defense in depth...

Basically, I'm not inclined to go to extreme lengths to get utter perfection
here: if the memory consumption is down, and is likely stay down, and is
fairly easy to diagnose and correct if it doesn't stay down, then I think
that's clearly Good Enough, even if there are a handful of tests that don't
relinquish all their memory, or don't relinquish it as promptly as they
theoretically could.

Aside from the comments above, most of the diff looks ok to me except these
bits:

> +++ bzrlib/tests/test_errors.py 2010-10-20 23:41:12 +0000
> @@ -575,12 +575,15 @@
> try:
> 1/0
> except ZeroDivisionError:
> - exc_info = sys.exc_info()
> - err = errors.HookFailed('hook stage', 'hook name', exc_info, warn=False)
> - self.asser...

[Andrew Bennetts (spiv)]

I said:

> Why not strip __dict__ as well? Defense in depth...

I just realised you do! So that's nice :)

[Andrew Bennetts (spiv)]

FWIW, under 4-way --parallel=fork on my laptop:

* I see significant memory savings too (no hard figures as I wasn't watching closely enough, but at least half the size, possibly better).

* The time drops by about 30s (from 560s down to 530s), which is the sort of modest improvement I was hoping to see :)

So, nice work! I just don't want it to be a burden on future development.

[Martin Packman (gz)]

Thanks Andrew, this is great feedback.

> Another alternative: why not a single full gc collection at the end of the
> entire run, and then report any test cases that are unexpectedly still
> alive?

I didn't take this approach because I'm already in swapping death by that point. Now the main issues are fixed in this branch that sounds like a reasonable option.

> The point is that if a VM chooses to allow say 20 or even 200 used test case
> instances to be alive simultaneously before collecting them, then that's
> fine. It'll still keep the memory ceiling reasonably low rather than
> growing in proportion to the number of tests. So requiring that a test case
> lifetime is strictly minimal is too strong a requirement, and I'm not sure
> it's worth paying the price for it. The price is either assuming a
> refcounting VM (and the fairly extreme senstivity to refcounting differences
> due to different versions of dependencies like testtools or even the Python
> stdlib), or forcing a full GC after every test. I agree that the latter is
> too much, but I also think the former isn't worth paying either.

It's true cycles are harmless in and off themselves nowadays, but they do make execution non-deterministic. By and large, they are also avoidable and I think something of a code smell in tests at least. Many of the changes I've made to tests actually improve the code, and where it makes things worse tends to be in tests using slightly odd patterns.

I think there are benefits in not creating cycles in bzrlib proper. There are cases where building up a lot of garbage really hurts†, and while performance regressions should be noticed, I like selftest coverage.

There's only one Python implementation bzr can use, and it refcounts, so I don't think relying on that is much of a stretch. Neither Jython nor IronPython‡ are funded any more, and PyPy...

> Why not strip __dict__ as well? Defense in depth...

As you noted this branch does do that, but Robert was trying to avoid this. Having failed tests in a examinable state at the end of the run is potentially useful.

> Basically, I'm not inclined to go to extreme lengths to get utter perfection
> here: if the memory consumption is down, and is likely stay down, and is
> fairly easy to diagnose and correct if it doesn't stay down, then I think
> that's clearly Good Enough, even if there are a handful of tests that don't
> relinquish all their memory, or don't relinquish it as promptly as they
> theoretically could.

I feel this is where we are and have been with thread leaks, and it's been pain felt disproportionately by the wrong people. It's much easier for the test writer to get things correct, than those who end up suffering from the random problems. This is why I'm adding noise, and even considering making tests fail, for such a minor thing. It might be an overreaction, but I'd prefer to have an always on, testable solution than one big switch that will be generally ignored.

> That hunk looks odd. Surely falling off the end of the function
> already deletes all the locals? And if the test fails, I don't really care
> about the extra memory cost. Certainly not enough to clutter the test.

What...

[Andrew Bennetts (spiv)]

Martin [gz] wrote:
> Thanks Andrew, this is great feedback.
>
> > Another alternative: why not a single full gc collection at the end of the
> > entire run, and then report any test cases that are unexpectedly still
> > alive?
>
> I didn't take this approach because I'm already in swapping death by that
> point. Now the main issues are fixed in this branch that sounds like a
> reasonable option.

Ah, right. For unexpected swap death it would be good to have an optional mode
along the lines of this patch. Although hopefully it'll never return :)

> > The point is that if a VM chooses to allow say 20 or even 200 used test case
> > instances to be alive simultaneously before collecting them, then that's
> > fine. It'll still keep the memory ceiling reasonably low rather than
> > growing in proportion to the number of tests. So requiring that a test case
> > lifetime is strictly minimal is too strong a requirement, and I'm not sure
> > it's worth paying the price for it. The price is either assuming a
> > refcounting VM (and the fairly extreme senstivity to refcounting differences
> > due to different versions of dependencies like testtools or even the Python
> > stdlib), or forcing a full GC after every test. I agree that the latter is
> > too much, but I also think the former isn't worth paying either.
>
> It's true cycles are harmless in and off themselves nowadays, but they do make
> execution non-deterministic. By and large, they are also avoidable and I think
> something of a code smell in tests at least. Many of the changes I've made to
> tests actually improve the code, and where it makes things worse tends to be
> in tests using slightly odd patterns.

I'm not really bothered by a little non-determinism in tests, especially as we
do so little with __del__ methods or weakref callbacks. If we want to remove
non-determinism in tests we have a much bigger problem: threads.

I'd say the changes you've made to test code are 50-50: about half a touch
nicer, about half a bit worse. I'm talking in terms of clarity, which is
probably the most important concern for test code. Adding try/finally blocks
that happen to break cycles a casual reader or writer wouldn't even think of,
when cycles aren't significant to the behaviour being tested, detract from the
test.

> I think there are benefits in not creating cycles in bzrlib proper. There are
> cases where building up a lot of garbage really hurts†, and while performance
> regressions should be noticed, I like selftest coverage.

Yes, John's noticed the same thing. Manually breaking cycles is a terribly
fragile way to get a 5% benefit though (5% being what I saw in terms of selftest
performance, and roughly what John's noticed in the past for some other issues).
The best fix is to simply create less objects, or at least create less
gc-tracked objects: hence StaticTuple, and other performance work John's done.
Manually breaking cycles here and there throughout the code isn't really
tackling root causes.

I don't mind it in generic higher-order function infrastructure like
bzrlib.decorators and bzrlib.cleanups, where many places benefit from a fairly
simple change (although even there it m...

[Vincent Ladeuil (vila)]

This is definitely cleaner ! Excellent.

Yet:

./bzr selftest -s bzrlib.tests.test_selftest.TestUncollectedWarnings => 7 failures

and doing a full run output an 'Uncollected test case:' line for every test.

Do I miss some special version of testtools ? (I'm running with lp:testtools revno 128).

review: Needs Fixing

[Martin Packman (gz)]

Sorry, had left tip on launchpad in a slightly broken state, have synced it up with my local branch again.

[Andrew Bennetts (spiv)]

I still see a fair few "Uncollected test case" messages when doing "./bzr selftest". Too many to be useful, I fear.

And if I use --parallel=fork (which I do almost all the time) then every test is reported as uncollected! So this still needs a bit more work I guess. I'm going to put this back to Work in Progress to declutter the review queue.

review: Needs Fixing

[Andrew Bennetts (spiv)]

On the plus side it does still pass regular selftest locally, even after merging latest lp:bzr (no conflicts currently).

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2011-06-21 11:34:52 +0000
+++ bzrlib/errors.py	2011-07-01 00:12:55 +0000
@@ -1659,6 +1659,7 @@
 
     def __init__(self, exc_info):
         import traceback
+        # GZ 2010-08-10: Cycle with exc_tb/exc_info affects at least one test
         self.exc_type, self.exc_value, self.exc_tb = exc_info
         self.exc_info = exc_info
         traceback_strings = traceback.format_exception(

=== modified file 'bzrlib/tests/TestUtil.py'
--- bzrlib/tests/TestUtil.py	2011-05-26 08:05:45 +0000
+++ bzrlib/tests/TestUtil.py	2011-07-01 00:12:55 +0000
@@ -19,6 +19,7 @@
 import sys
 import logging
 import unittest
+import weakref
 
 from bzrlib import pyutils
 
@@ -82,14 +83,37 @@
         tests = list(self)
         tests.reverse()
         self._tests = []
+        stream = getattr(result, "stream", None)
+        # With subunit, not only is stream underscored, but the actual result
+        # object is hidden inside a wrapper decorator, get out the real stream
+        if stream is None:
+            stream = result.decorated._stream
+        stored_count = 0
+        count_stored_tests = getattr(result, "_count_stored_tests", int)
+        from bzrlib.tests import selftest_debug_flags
+        notify = "uncollected_cases" in selftest_debug_flags
         while tests:
             if result.shouldStop:
                 self._tests = reversed(tests)
                 break
-            tests.pop().run(result)
+            case = _run_and_collect_case(tests.pop(), result)()
+            new_stored_count = count_stored_tests()
+            if case is not None and isinstance(case, unittest.TestCase):
+                if stored_count == new_stored_count and notify:
+                    # Testcase didn't fail, but somehow is still alive
+                    stream.write("Uncollected test case: %s\n" % (case.id(),))
+                # Zombie the testcase but leave a working stub id method
+                case.__dict__ = {"id": lambda _id=case.id(): _id}
+            stored_count = new_stored_count
         return result
 
 
+def _run_and_collect_case(case, res):
+    """Run test case against result and use weakref to drop the refcount"""
+    case.run(res)
+    return weakref.ref(case)
+
+
 class TestLoader(unittest.TestLoader):
     """Custom TestLoader to extend the stock python one."""
 

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-06-27 15:42:09 +0000
+++ bzrlib/tests/__init__.py	2011-07-01 00:12:55 +0000
@@ -495,6 +495,10 @@
         self.not_applicable_count += 1
         self.report_not_applicable(test, reason)
 
+    def _count_stored_tests(self):
+        """Count of tests instances kept alive due to not succeeding"""
+        return self.error_count + self.failure_count + self.known_failure_count
+
     def _post_mortem(self, tb=None):
         """Start a PDB post mortem session."""
         if os.environ.get('BZR_TEST_PDB', None):
@@ -3159,6 +3163,9 @@
                             result_decorators=result_decorators,
                             )
     runner.stop_on_failure=stop_on_failure
+    if isinstance(suite, unittest.TestSuite):
+        # Empty out _tests list of passed suite and populate new TestSuite
+        suite._tests[:], suite = [], TestSuite(suite)
     # built in decorator factories:
     decorators = [
         random_order(random_seed, runner),
@@ -3262,34 +3269,17 @@
 
 class TestDecorator(TestUtil.TestSuite):
     """A decorator for TestCase/TestSuite objects.
-    
-    Usually, subclasses should override __iter__(used when flattening test
-    suites), which we do to filter, reorder, parallelise and so on, run() and
-    debug().
+
+    Contains rather than flattening suite passed on construction
     """
 
-    def __init__(self, suite):
-        TestUtil.TestSuite.__init__(self)
-        self.addTest(suite)
-
-    def countTestCases(self):
-        cases = 0
-        for test in self:
-            cases += test.countTestCases()
-        return cases
-
-    def debug(self):
-        for test in self:
-            test.debug()
-
-    def run(self, result):
-        # Use iteration on self, not self._tests, to allow subclasses to hook
-        # into __iter__.
-        for test in self:
-            if result.shouldStop:
-                break
-            test.run(result)
-        return result
+    def __init__(self, suite=None):
+        super(TestDecorator, self).__init__()
+        if suite is not None:
+            self.addTest(suite)
+
+    # Don't need subclass run method with suite emptying
+    run = unittest.TestSuite.run
 
 
 class CountingDecorator(TestDecorator):
@@ -3306,90 +3296,50 @@
     """A decorator which excludes test matching an exclude pattern."""
 
     def __init__(self, suite, exclude_pattern):
-        TestDecorator.__init__(self, suite)
-        self.exclude_pattern = exclude_pattern
-        self.excluded = False
-
-    def __iter__(self):
-        if self.excluded:
-            return iter(self._tests)
-        self.excluded = True
-        suite = exclude_tests_by_re(self, self.exclude_pattern)
-        del self._tests[:]
-        self.addTests(suite)
-        return iter(self._tests)
+        super(ExcludeDecorator, self).__init__(
+            exclude_tests_by_re(suite, exclude_pattern))
 
 
 class FilterTestsDecorator(TestDecorator):
     """A decorator which filters tests to those matching a pattern."""
 
     def __init__(self, suite, pattern):
-        TestDecorator.__init__(self, suite)
-        self.pattern = pattern
-        self.filtered = False
-
-    def __iter__(self):
-        if self.filtered:
-            return iter(self._tests)
-        self.filtered = True
-        suite = filter_suite_by_re(self, self.pattern)
-        del self._tests[:]
-        self.addTests(suite)
-        return iter(self._tests)
+        super(FilterTestsDecorator, self).__init__(
+            filter_suite_by_re(suite, pattern))
 
 
 class RandomDecorator(TestDecorator):
     """A decorator which randomises the order of its tests."""
 
     def __init__(self, suite, random_seed, stream):
-        TestDecorator.__init__(self, suite)
-        self.random_seed = random_seed
-        self.randomised = False
-        self.stream = stream
-
-    def __iter__(self):
-        if self.randomised:
-            return iter(self._tests)
-        self.randomised = True
-        self.stream.write("Randomizing test order using seed %s\n\n" %
-            (self.actual_seed()))
+        random_seed = self.actual_seed(random_seed)
+        stream.write("Randomizing test order using seed %s\n\n" %
+            (random_seed,))
         # Initialise the random number generator.
-        random.seed(self.actual_seed())
-        suite = randomize_suite(self)
-        del self._tests[:]
-        self.addTests(suite)
-        return iter(self._tests)
+        random.seed(random_seed)
+        super(RandomDecorator, self).__init__(randomize_suite(suite))
 
-    def actual_seed(self):
-        if self.random_seed == "now":
+    @staticmethod
+    def actual_seed(seed):
+        if seed == "now":
             # We convert the seed to a long to make it reuseable across
             # invocations (because the user can reenter it).
-            self.random_seed = long(time.time())
+            return long(time.time())
         else:
             # Convert the seed to a long if we can
             try:
-                self.random_seed = long(self.random_seed)
-            except:
+                return long(seed)
+            except (TypeError, ValueError):
                 pass
-        return self.random_seed
+        return seed
 
 
 class TestFirstDecorator(TestDecorator):
     """A decorator which moves named tests to the front."""
 
     def __init__(self, suite, pattern):
-        TestDecorator.__init__(self, suite)
-        self.pattern = pattern
-        self.filtered = False
-
-    def __iter__(self):
-        if self.filtered:
-            return iter(self._tests)
-        self.filtered = True
-        suites = split_suite_by_re(self, self.pattern)
-        del self._tests[:]
-        self.addTests(suites)
-        return iter(self._tests)
+        super(TestFirstDecorator, self).__init__()
+        self.addTests(split_suite_by_re(suite, pattern))
 
 
 def partition_tests(suite, count):
@@ -3442,9 +3392,12 @@
                 os.waitpid(self.pid, 0)
 
     test_blocks = partition_tests(suite, concurrency)
+    # Clear the tests from the original suite so it doesn't keep them alive
+    suite._tests[:] = []
     for process_tests in test_blocks:
-        process_suite = TestUtil.TestSuite()
-        process_suite.addTests(process_tests)
+        process_suite = TestUtil.TestSuite(process_tests)
+        # Also clear each split list so new suite has only reference
+        process_tests[:] = []
         c2pread, c2pwrite = os.pipe()
         pid = os.fork()
         if pid == 0:
@@ -3456,12 +3409,15 @@
                 # read from stdin (otherwise its a roulette to see what
                 # child actually gets keystrokes for pdb etc).
                 sys.stdin.close()
-                sys.stdin = None
+                # GZ 2011-06-16: Why set stdin to None? Breaks multi fork.
+                #sys.stdin = None
                 stream = os.fdopen(c2pwrite, 'wb', 1)
                 subunit_result = AutoTimingTestResultDecorator(
                     TestProtocolClient(stream))
                 process_suite.run(subunit_result)
             finally:
+                # GZ 2011-06-16: Is always exiting with silent success
+                #                really the right thing? Hurts debugging.
                 os._exit(0)
         else:
             os.close(c2pwrite)
@@ -3574,8 +3530,13 @@
 #                           with proper exclusion rules.
 #   -Ethreads               Will display thread ident at creation/join time to
 #                           help track thread leaks
+<<<<<<< TREE
 
 #   -Econfig_stats          Will collect statistics using addDetail
+=======
+#   -Euncollected_cases     Display the identity of any test cases that weren't
+#                           deallocated after being completed.
+>>>>>>> MERGE-SOURCE
 selftest_debug_flags = set()
 
 
@@ -4683,6 +4644,14 @@
     from subunit.test_results import AutoTimingTestResultDecorator
     class SubUnitBzrProtocolClient(TestProtocolClient):
 
+        # GZ 2011-05-26: This duplicates logic in ExtendedTestResult.stopTest
+        def stopTest(self, test):
+            super(SubUnitBzrProtocolClient, self).stopTest(test)
+            # Break bound method cycles added in Python 2.7 unittest rewrite
+            type_equality_funcs = getattr(test, "_type_equality_funcs", None)
+            if type_equality_funcs is not None:
+                type_equality_funcs.clear()
+
         def addSuccess(self, test, details=None):
             # The subunit client always includes the details in the subunit
             # stream, but we don't want to include it in ours.

=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2011-06-17 13:59:23 +0000
+++ bzrlib/tests/test_selftest.py	2011-07-01 00:12:55 +0000
@@ -17,6 +17,7 @@
 """Tests for the test framework."""
 
 from cStringIO import StringIO
+import gc
 import doctest
 import os
 import signal
@@ -3385,6 +3386,113 @@
         self.assertLength(1, calls)
 
 
+class TestUncollectedWarnings(tests.TestCase):
+    """Check a test case still alive after being run emits a warning"""
+
+    class Test(tests.TestCase):
+        def test_pass(self):
+            pass
+        def test_self_ref(self):
+            self.also_self = self.test_self_ref
+        def test_skip(self):
+            self.skip("Don't need")
+
+    def _get_suite(self):
+        return TestUtil.TestSuite([
+            self.Test("test_pass"),
+            self.Test("test_self_ref"),
+            self.Test("test_skip"),
+            ])
+
+    def _inject_stream_into_subunit(self, stream):
+        """To be overridden by subclasses that run tests out of process"""
+
+    def _run_selftest_with_suite(self, **kwargs):
+        sio = StringIO()
+        self._inject_stream_into_subunit(sio)
+        old_flags = tests.selftest_debug_flags
+        tests.selftest_debug_flags = old_flags.union(["uncollected_cases"])
+        gc_on = gc.isenabled()
+        if gc_on:
+            gc.disable()
+        try:
+            tests.selftest(test_suite_factory=self._get_suite, stream=sio,
+                **kwargs)
+        finally:
+            if gc_on:
+                gc.enable()
+            tests.selftest_debug_flags = old_flags
+        output = sio.getvalue()
+        self.assertNotContainsRe(output, "Uncollected test case.*test_pass")
+        self.assertContainsRe(output, "Uncollected test case.*test_self_ref")
+        return output
+
+    def test_testsuite(self):
+        self._run_selftest_with_suite()
+
+    def test_pattern(self):
+        out = self._run_selftest_with_suite(pattern="test_(?:pass|self_ref)$")
+        self.assertNotContainsRe(out, "test_skip")
+
+    def test_exclude_pattern(self):
+        out = self._run_selftest_with_suite(exclude_pattern="test_skip$")
+        self.assertNotContainsRe(out, "test_skip")
+
+    def test_random_seed(self):
+        self._run_selftest_with_suite(random_seed="now")
+
+    def test_matching_tests_first(self):
+        self._run_selftest_with_suite(matching_tests_first=True,
+            pattern="test_self_ref$")
+
+    def test_starting_with_and_exclude(self):
+        out = self._run_selftest_with_suite(starting_with=["bt."],
+            exclude_pattern="test_skip$")
+        self.assertNotContainsRe(out, "test_skip")
+
+    def test_additonal_decorator(self):
+        out = self._run_selftest_with_suite(
+            suite_decorators=[tests.TestDecorator])
+
+
+class TestUncollectedWarningsSubunit(TestUncollectedWarnings):
+    """Check warnings from tests staying alive are emitted with subunit"""
+
+    _test_needs_features = [features.subunit]
+
+    def _run_selftest_with_suite(self, **kwargs):
+        return TestUncollectedWarnings._run_selftest_with_suite(self,
+            runner_class=tests.SubUnitBzrRunner, **kwargs)
+
+
+class TestUncollectedWarningsForking(TestUncollectedWarnings):
+    """Check warnings from tests staying alive are emitted when forking"""
+
+    _test_needs_features = [features.subunit]
+
+    def _inject_stream_into_subunit(self, stream):
+        """Monkey-patch subunit so the extra output goes to stream not stdout
+
+        Some APIs need rewriting so this kind of bogus hackery can be replaced
+        by passing the stream param from run_tests down into ProtocolTestCase.
+        """
+        from subunit import ProtocolTestCase
+        _original_init = ProtocolTestCase.__init__
+        def _init_with_passthrough(self, *args, **kwargs):
+            _original_init(self, *args, **kwargs)
+            self._passthrough = stream
+        self.overrideAttr(ProtocolTestCase, "__init__", _init_with_passthrough)
+
+    def _run_selftest_with_suite(self, **kwargs):
+        # GZ 2011-05-26: Add a PosixSystem feature so this check can go away
+        if getattr(os, "fork", None) is None:
+            raise tests.TestNotApplicable("Platform doesn't support forking")
+        # Make sure the fork code is actually invoked by claiming two cores
+        self.overrideAttr(osutils, "local_concurrency", lambda: 2)
+        kwargs.setdefault("suite_decorators", []).append(tests.fork_decorator)
+        return TestUncollectedWarnings._run_selftest_with_suite(self, **kwargs)
+
+
 class TestEnvironHandling(tests.TestCase):
 
     def test_overrideEnv_None_called_twice_doesnt_leak(self):