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...

[lp:~gz/bzr/cleanup_testcases_by_collection_613247]

5357. By Martin Packman on 2010-08-23

Revert changes to bb.test_log which caused some failures

5358. By Martin Packman on 2010-08-26

Instead break cycle from closure over testcase instance in bb.test_log

5359. By Martin Packman on 2010-10-19

Merge bzr.dev to pick up leak fixes

5360. By Martin Packman on 2010-10-19

Use jam's trick with getDetails rather than hacking around double underscore name mangling

5361. By Martin Packman on 2010-10-20

A few more simple test changes to avoid or break cycles

5362. By Martin Packman on 2010-10-20

Adapt test cleanup accounting to subunit quirks

[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.

[lp:~gz/bzr/cleanup_testcases_by_collection_613247]

5363. By Martin Packman on 2010-11-08

Resort to relying on subunit internals to avoid messing around with stdout in testing

5364. By Martin Packman on 2010-11-08

Add testing for uncollected case warnings under subunit

5365. By Martin Packman on 2010-11-08

Only fiddle with gc for tests if it is enabled, as noted by spiv in review

5366. By Martin Packman on 2010-11-12

Tidy up and comment some of the test changes

5367. By Martin Packman on 2011-05-18

Merge bzr.dev to renew work

5368. By Martin Packman on 2011-05-18

Fix mixup in lazy hook restoration and clear dict

5369. By Martin Packman on 2011-05-18

Put reporting of uncollected tests behind a -Ecollection flag

5370. By Martin Packman on 2011-05-18

Cleanup testcase logging internals and avoid a cycle

5371. By Martin Packman on 2011-05-18

Clear extra lists with test instances in fork_for_tests

5372. By Martin Packman on 2011-05-20

Supersede all of the split out work that jam did

5373. By John A Meinel on 2011-05-25

Bring bzr.dev 5912 back into gz's branch.

5374. By Martin Packman on 2011-05-26

Use 'uncollected_cases' as the flag name which seems a bit clearer

5375. By Martin Packman on 2011-05-26

Fix tests to work regardless of flag state

5376. By Martin Packman on 2011-05-26

Add test subclass for collection of cases under --parallel=fork

5377. By Martin Packman on 2011-05-26

Fix subunit collection tests on Python 2.7 with a little copy code

5378. By Martin Packman on 2011-05-26

Make the fork tests check the right code on single core machines

5379. By Martin Packman on 2011-05-26

Really fix the fork tests by screwing around with subunit internals

5380. By Martin Packman on 2011-05-26

Tweaks and comments

5381. By Martin Packman on 2011-05-26

Merge bzr.dev to resolve conflicts

5382. By Martin Packman on 2011-06-16

Fix fork tests when run using fork, this was a pain to debug

5383. By Martin Packman on 2011-07-27

Use tweaks for subunit protocol client with --parallel=fork as well as --subunit

5384. By Martin Packman on 2011-09-16

Merge bzr.dev to unify _type_equality_funcs workarounds

5385. By Martin Packman on 2011-09-16

Factor out _type_equality_funcs to a new function

5386. By Martin Packman on 2011-09-16

Use pseudo testcase to always fail as suggested by lifeless in review rather than hacking the subunit stream

5387. By Martin Packman on 2011-09-16

Stop making zombie testcases for now as it's an extra risk

5388. By Martin Packman on 2011-10-04

Ensure all inner tests in TestUncollectedWarnings are run

5389. By Martin Packman on 2011-10-04

After adding a pseudo-failure update the stored test count again

5390. By Martin Packman on 2011-10-04

Add release notes

5391. By Martin Packman on 2011-10-05

Revert checking of test count in TestUncollectedCases.test_random_seed

5392. By Martin Packman on 2011-10-18

Dedent release notes to right format

Unmerged revisions

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2010-10-12 09:46:37 +0000
+++ bzrlib/branch.py	2010-10-20 23:41:10 +0000
@@ -3258,7 +3258,10 @@
         try:
             target.unlock()
         finally:
-            raise exc_info[0], exc_info[1], exc_info[2]
+            try:
+                raise exc_info[0], exc_info[1], exc_info[2]
+            finally:
+                del exc_info
     else:
         target.unlock()
         return result

=== modified file 'bzrlib/cleanup.py'
--- bzrlib/cleanup.py	2010-04-27 07:29:11 +0000
+++ bzrlib/cleanup.py	2010-10-20 23:41:10 +0000
@@ -188,7 +188,10 @@
                 # but don't propagate them.
                 _run_cleanup(cleanup, *c_args, **kwargs)
         if exc_info is not None:
-            raise exc_info[0], exc_info[1], exc_info[2]
+            try:
+                raise exc_info[0], exc_info[1], exc_info[2]
+            finally:
+                del exc_info
         # No error, so we can return the result
         return result
 

=== modified file 'bzrlib/decorators.py'
--- bzrlib/decorators.py	2010-02-17 17:11:16 +0000
+++ bzrlib/decorators.py	2010-10-20 23:41:10 +0000
@@ -101,7 +101,10 @@
         try:
             self.unlock()
         finally:
-            raise exc_info[0], exc_info[1], exc_info[2]
+            try:
+                raise exc_info[0], exc_info[1], exc_info[2]
+            finally:
+                del exc_info
     else:
         self.unlock()
         return result
@@ -144,7 +147,10 @@
             try:
                 self.unlock()
             finally:
-                raise exc_info[0], exc_info[1], exc_info[2]
+                try:
+                    raise exc_info[0], exc_info[1], exc_info[2]
+                finally:
+                    del exc_info
         else:
             self.unlock()
             return result
@@ -166,7 +172,10 @@
         try:
             self.unlock()
         finally:
-            raise exc_info[0], exc_info[1], exc_info[2]
+            try:
+                raise exc_info[0], exc_info[1], exc_info[2]
+            finally:
+                del exc_info
     else:
         self.unlock()
         return result
@@ -197,7 +206,10 @@
             try:
                 self.unlock()
             finally:
-                raise exc_info[0], exc_info[1], exc_info[2]
+                try:
+                    raise exc_info[0], exc_info[1], exc_info[2]
+                finally:
+                    del exc_info
         else:
             self.unlock()
             return result

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-10-13 08:01:36 +0000
+++ bzrlib/errors.py	2010-10-20 23:41:10 +0000
@@ -1638,6 +1638,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/osutils.py'
--- bzrlib/osutils.py	2010-09-16 09:52:29 +0000
+++ bzrlib/osutils.py	2010-10-20 23:41:10 +0000
@@ -269,7 +269,10 @@
             else:
                 rename_func(tmp_name, new)
     if failure_exc is not None:
-        raise failure_exc[0], failure_exc[1], failure_exc[2]
+        try:
+            raise failure_exc[0], failure_exc[1], failure_exc[2]
+        finally:
+            del failure_exc
 
 
 # In Python 2.4.2 and older, os.path.abspath and os.path.realpath

=== modified file 'bzrlib/tests/TestUtil.py'
--- bzrlib/tests/TestUtil.py	2010-09-21 03:20:09 +0000
+++ bzrlib/tests/TestUtil.py	2010-10-20 23:41:10 +0000
@@ -19,6 +19,7 @@
 import sys
 import logging
 import unittest
+import weakref
 
 from bzrlib import pyutils
 
@@ -79,14 +80,29 @@
         tests = list(self)
         tests.reverse()
         self._tests = []
+        stored_count = 0
         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 = getattr(result, "_count_stored_tests", int)()
+            if case is not None and isinstance(case, unittest.TestCase):
+                if stored_count == new_stored_count:
+                    # Testcase didn't fail, but somehow is still alive
+                    print "Uncollected test case: %s" % (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	2010-10-18 17:06:37 +0000
+++ bzrlib/tests/__init__.py	2010-10-20 23:41:10 +0000
@@ -306,13 +306,21 @@
         if addCleanup is not None:
             addCleanup(self._check_leaked_threads, test)
 
+    def stopTest(self, test):
+        super(ExtendedTestResult, self).stopTest(test)
+        # Manually break cycles, means touching various private things but hey
+        getDetails = getattr(test, "getDetails", None)
+        if getDetails is not None:
+            getDetails().clear()
+        type_equality_funcs = getattr(test, "_type_equality_funcs", None)
+        if type_equality_funcs is not None:
+            type_equality_funcs.clear()
+        self._traceback_from_test = None
+
     def startTests(self):
         self.report_tests_starting()
         self._active_threads = threading.enumerate()
 
-    def stopTest(self, test):
-        self._traceback_from_test = None
-
     def _check_leaked_threads(self, test):
         """See if any threads have leaked since last call
 
@@ -400,6 +408,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):
@@ -1195,13 +1207,13 @@
         try:
             def capture():
                 orig_log_exception_quietly()
-                captured.append(sys.exc_info())
+                captured.append(sys.exc_info()[1])
             trace.log_exception_quietly = capture
             func(*args, **kwargs)
         finally:
             trace.log_exception_quietly = orig_log_exception_quietly
         self.assertLength(1, captured)
-        err = captured[0][1]
+        err = captured[0]
         self.assertIsInstance(err, exception_class)
         return err
 
@@ -1601,6 +1613,7 @@
     def _restoreHooks(self):
         for klass, (name, hooks) in self._preserved_hooks.items():
             setattr(klass, name, hooks)
+        self._preserved_hooks.clear()
 
     def knownFailure(self, reason):
         """This test has failed for some known reason."""
@@ -2982,6 +2995,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),
@@ -3085,34 +3101,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):
@@ -3129,90 +3128,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):

=== modified file 'bzrlib/tests/blackbox/test_breakin.py'
--- bzrlib/tests/blackbox/test_breakin.py	2009-09-28 03:36:45 +0000
+++ bzrlib/tests/blackbox/test_breakin.py	2010-10-20 23:41:10 +0000
@@ -42,10 +42,6 @@
     def setUp(self):
         super(TestBreakin, self).setUp()
         self.requireFeature(tests.BreakinFeature)
-        if sys.platform == 'win32':
-            self._send_signal = self._send_signal_win32
-        else:
-            self._send_signal = self._send_signal_via_kill
 
     def _send_signal_via_kill(self, pid, sig_type):
         if sig_type == 'break':
@@ -89,6 +85,11 @@
             exit_code = breakin.determine_signal()
             ctypes.windll.kernel32.TerminateProcess(pid, exit_code)
 
+    if sys.platform == 'win32':
+        _send_signal = _send_signal_win32
+    else:
+        _send_signal = _send_signal_via_kill
+
     def _popen(self, *args, **kwargs):
         if sys.platform == 'win32':
             CREATE_NEW_PROCESS_GROUP = 512

=== modified file 'bzrlib/tests/blackbox/test_log.py'
--- bzrlib/tests/blackbox/test_log.py	2010-07-09 16:16:11 +0000
+++ bzrlib/tests/blackbox/test_log.py	2010-10-20 23:41:10 +0000
@@ -77,6 +77,7 @@
                 self.log_catcher = test_log.LogCatcher(*args, **kwargs)
                 # Always return our own log formatter
                 return self.log_catcher
+        self.addCleanup(setattr, MyLogFormatter, "__new__", None)
 
         def getme(branch):
                 # Always return our own log formatter class hijacking the

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py	2010-06-20 11:18:38 +0000
+++ bzrlib/tests/blackbox/test_serve.py	2010-10-20 23:41:10 +0000
@@ -276,7 +276,8 @@
 
 class TestUserdirExpansion(TestCaseWithMemoryTransport):
 
-    def fake_expanduser(self, path):
+    @staticmethod
+    def fake_expanduser(path):
         """A simple, environment-independent, function for the duration of this
         test.
 

=== modified file 'bzrlib/tests/per_repository/test_check.py'
--- bzrlib/tests/per_repository/test_check.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/per_repository/test_check.py	2010-10-20 23:41:10 +0000
@@ -113,8 +113,8 @@
             needed_refs.setdefault(ref, []).append(tree.branch)
         self.tree_check = tree._check
         self.branch_check = tree.branch.check
-        tree._check = self.tree_callback
-        tree.branch.check = self.branch_callback
+        self.overrideAttr(tree, "_check", self.tree_callback)
+        self.overrideAttr(tree.branch, "check", self.branch_callback)
         self.callbacks = []
         tree.branch.repository.check([revid], callback_refs=needed_refs)
         self.assertNotEqual([], self.callbacks)

=== modified file 'bzrlib/tests/per_workingtree/test_smart_add.py'
--- bzrlib/tests/per_workingtree/test_smart_add.py	2010-02-23 07:43:11 +0000
+++ bzrlib/tests/per_workingtree/test_smart_add.py	2010-10-20 23:41:10 +0000
@@ -265,6 +265,7 @@
         super(TestSmartAddTreeUnicode, self).setUp()
         self.build_tree([u'a\u030a'])
         self.wt = self.make_branch_and_tree('.')
+        self.addCleanup(setattr, self, "wt", None)
         self.overrideAttr(osutils, 'normalized_filename')
 
     def test_accessible_explicit(self):

=== modified file 'bzrlib/tests/test_chk_map.py'
--- bzrlib/tests/test_chk_map.py	2010-10-08 02:13:25 +0000
+++ bzrlib/tests/test_chk_map.py	2010-10-20 23:41:10 +0000
@@ -1110,7 +1110,7 @@
             if ('sha1:1adf7c0d1b9140ab5f33bb64c6275fa78b1580b7',) in keys:
                 self.fail("'aaa' pointer was followed %r" % keys)
             return basis_get(keys, order, fulltext)
-        basis._store.get_record_stream = get_record_stream
+        self.overrideAttr(basis._store, "get_record_stream", get_record_stream)
         result = sorted(list(target.iter_changes(basis)))
         for change in result:
             if change[0] == ('aaa',):

=== modified file 'bzrlib/tests/test_clean_tree.py'
--- bzrlib/tests/test_clean_tree.py	2010-08-23 17:54:07 +0000
+++ bzrlib/tests/test_clean_tree.py	2010-10-20 23:41:10 +0000
@@ -99,7 +99,6 @@
         def _dummy_rmtree(path, ignore_errors=False, onerror=None):
             """Call user supplied error handler onerror.
             """
-            self.assertTrue(isinstance(onerror, types.FunctionType))
             # Indicate failure in removing 'path' if path is subdir0
             # We later check to ensure that this is indicated
             # to the user as a warning. We raise OSError to construct

=== modified file 'bzrlib/tests/test_errors.py'
--- bzrlib/tests/test_errors.py	2010-09-13 02:32:44 +0000
+++ bzrlib/tests/test_errors.py	2010-10-20 23:41:10 +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.assertStartsWith(
-            str(err), 'Hook \'hook name\' during hook stage failed:\n')
-        self.assertEndsWith(
-            str(err), 'integer division or modulo by zero')
+            err = errors.HookFailed('hook stage', 'hook name', sys.exc_info(),
+                warn=False)
+        try:
+            self.assertStartsWith(
+                str(err), 'Hook \'hook name\' during hook stage failed:\n')
+            self.assertEndsWith(
+                str(err), 'integer division or modulo by zero')
+        finally:
+            del err
 
     def test_tip_change_rejected(self):
         err = errors.TipChangeRejected(u'Unicode message\N{INTERROBANG}')
@@ -609,11 +612,13 @@
         try:
             raise Exception("example error")
         except Exception:
-            exc_info = sys.exc_info()
-        err = errors.SmartMessageHandlerError(exc_info)
-        self.assertStartsWith(
-            str(err), "The message handler raised an exception:\n")
-        self.assertEndsWith(str(err), "Exception: example error\n")
+            err = errors.SmartMessageHandlerError(sys.exc_info())
+        try:
+            self.assertStartsWith(
+                str(err), "The message handler raised an exception:\n")
+            self.assertEndsWith(str(err), "Exception: example error\n")
+        finally:
+            del err
 
     def test_must_have_working_tree(self):
         err = errors.MustHaveWorkingTree('foo', 'bar')

=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py	2010-10-12 07:24:46 +0000
+++ bzrlib/tests/test_http.py	2010-10-20 23:41:10 +0000
@@ -1977,11 +1977,12 @@
         tests.TestCase.setUp(self)
         self.server = self._activity_server(self._protocol_version)
         self.server.start_server()
-        self.activities = {}
+        _activities = {} # Don't close over self and create a cycle
         def report_activity(t, bytes, direction):
-            count = self.activities.get(direction, 0)
+            count = _activities.get(direction, 0)
             count += bytes
-            self.activities[direction] = count
+            _activities[direction] = count
+        self.activities = _activities
 
         # We override at class level because constructors may propagate the
         # bound method and render instance overriding ineffective (an

=== modified file 'bzrlib/tests/test_knit.py'
--- bzrlib/tests/test_knit.py	2010-06-20 11:18:38 +0000
+++ bzrlib/tests/test_knit.py	2010-10-20 23:41:10 +0000
@@ -415,6 +415,7 @@
         except _TestException, e:
             retry_exc = errors.RetryWithNewPacks(None, reload_occurred=False,
                                                  exc_info=sys.exc_info())
+        # GZ 2010-08-10: Cycle with exc_info affects 3 tests
         return retry_exc
 
     def test_read_from_several_packs(self):

=== modified file 'bzrlib/tests/test_merge.py'
--- bzrlib/tests/test_merge.py	2010-07-29 04:07:27 +0000
+++ bzrlib/tests/test_merge.py	2010-10-20 23:41:10 +0000
@@ -2843,14 +2843,14 @@
 
     def get_merger_factory(self):
         # Allows  the inner methods to access the test attributes
-        test = self
+        calls = self.calls
 
         class FooMerger(_mod_merge.ConfigurableFileMerger):
             name_prefix = "foo"
             default_files = ['bar']
 
             def merge_text(self, params):
-                test.calls.append('merge_text')
+                calls.append('merge_text')
                 return ('not_applicable', None)
 
         def factory(merger):

=== modified file 'bzrlib/tests/test_registry.py'
--- bzrlib/tests/test_registry.py	2010-09-21 03:20:09 +0000
+++ bzrlib/tests/test_registry.py	2010-10-20 23:41:10 +0000
@@ -216,11 +216,12 @@
         # We create a registry with "official" objects and "hidden"
         # objects. The later represent the side effects that led to bug #277048
         # and #430510
-        self.registry =  registry.Registry()
+        self_registry = registry.Registry()
 
         def register_more():
-            self.registry.register('hidden', None)
+            self_registry.register('hidden', None)
 
+        self.registry = self_registry
         self.registry.register('passive', None)
         self.registry.register('active', register_more)
         self.registry.register('passive-too', None)
@@ -230,7 +231,7 @@
             def get_obj(inner_self):
                 # Surprise ! Getting a registered object (think lazy loaded
                 # module) register yet another object !
-                self.registry.register('more hidden', None)
+                self_registry.register('more hidden', None)
                 return inner_self._obj
 
         self.registry.register('hacky', None)

=== modified file 'bzrlib/tests/test_repository.py'
--- bzrlib/tests/test_repository.py	2010-08-05 19:25:52 +0000
+++ bzrlib/tests/test_repository.py	2010-10-20 23:41:10 +0000
@@ -1658,7 +1658,7 @@
         self.addCleanup(target.unlock)
         source = source_tree.branch.repository._get_source(target._format)
         self.orig_pack = target.pack
-        target.pack = self.log_pack
+        self.overrideAttr(target, "pack", self.log_pack)
         search = target.search_missing_revision_ids(
             source_tree.branch.repository, tip)
         stream = source.get_stream(search)
@@ -1682,7 +1682,7 @@
         self.addCleanup(target.unlock)
         source = source_tree.branch.repository
         self.orig_pack = target.pack
-        target.pack = self.log_pack
+        self.overrideAttr(target, "pack", self.log_pack)
         target.fetch(source)
         if expect_pack_called:
             self.assertLength(1, self.calls)

=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2010-10-15 11:20:45 +0000
+++ bzrlib/tests/test_selftest.py	2010-10-20 23:41:10 +0000
@@ -18,6 +18,7 @@
 
 from cStringIO import StringIO
 from doctest import ELLIPSIS
+import gc
 import os
 import signal
 import sys
@@ -36,7 +37,7 @@
     DocTestMatches,
     Equals,
     )
-import testtools.tests.helpers
+import testtools.testresult.doubles
 
 import bzrlib
 from bzrlib import (
@@ -705,7 +706,7 @@
 
     def test_profiles_tests(self):
         self.requireFeature(test_lsprof.LSProfFeature)
-        terminal = testtools.tests.helpers.ExtendedTestResult()
+        terminal = testtools.testresult.doubles.ExtendedTestResult()
         result = tests.ProfileResult(terminal)
         class Sample(tests.TestCase):
             def a(self):
@@ -728,7 +729,7 @@
                 descriptions=0,
                 verbosity=1,
                 )
-        capture = testtools.tests.helpers.ExtendedTestResult()
+        capture = testtools.testresult.doubles.ExtendedTestResult()
         test_case.run(MultiTestResult(result, capture))
         run_case = capture._events[0][1]
         timed_string = result._testTimeString(run_case)
@@ -1069,7 +1070,7 @@
         test = unittest.TestSuite()
         test.addTest(Test("known_failure_test"))
         def failing_test():
-            self.fail('foo')
+            raise AssertionError('foo')
         test.addTest(unittest.FunctionTestCase(failing_test))
         stream = StringIO()
         runner = tests.TextTestRunner(stream=stream)
@@ -1083,7 +1084,7 @@
             '^----------------------------------------------------------------------\n'
             'Traceback \\(most recent call last\\):\n'
             '  .*' # File .*, line .*, in failing_test' - but maybe not from .pyc
-            '    self.fail\\(\'foo\'\\)\n'
+            '    raise AssertionError\\(\'foo\'\\)\n'
             '.*'
             '^----------------------------------------------------------------------\n'
             '.*'
@@ -1095,7 +1096,7 @@
         # the final output.
         class Test(tests.TestCase):
             def known_failure_test(self):
-                self.expectFailure('failed', self.assertTrue, False)
+                self.knownFailure("Never works...")
         test = Test("known_failure_test")
         stream = StringIO()
         runner = tests.TextTestRunner(stream=stream)
@@ -2052,17 +2053,17 @@
 
     def test_lsprof_tests(self):
         self.requireFeature(test_lsprof.LSProfFeature)
-        calls = []
+        results = []
         class Test(object):
             def __call__(test, result):
                 test.run(result)
             def run(test, result):
-                self.assertIsInstance(result, ExtendedToOriginalDecorator)
-                calls.append("called")
+                results.append(result)
             def countTestCases(self):
                 return 1
         self.run_selftest(test_suite_factory=Test, lsprof_tests=True)
-        self.assertLength(1, calls)
+        self.assertLength(1, results)
+        self.assertIsInstance(results.pop(), ExtendedToOriginalDecorator)
 
     def test_random(self):
         # test randomising by listing a number of tests.
@@ -3394,3 +3395,62 @@
                                                 self.verbosity)
         tests.run_suite(suite, runner_class=MyRunner, stream=StringIO())
         self.assertLength(1, calls)
+
+
+class TestUncollectedWarnings(tests.TestCase):
+    """Check reference cycles keeping a test case alive emits in 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 _run_selftest_with_suite(self, **kwargs):
+        sio = StringIO()
+        gc.disable()
+        try:
+            tests.selftest(test_suite_factory=self._get_suite, stream=sio,
+                **kwargs)
+        finally:
+            gc.enable()
+        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])

=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py	2010-05-13 16:17:54 +0000
+++ bzrlib/tests/test_smart.py	2010-10-20 23:41:10 +0000
@@ -103,7 +103,7 @@
         # the default or a parameterized class, but rather use the
         # TestCaseWithTransport infrastructure to set up a smart server and
         # transport.
-        self.transport_server = self.make_transport_server
+        self.overrideAttr(self, "transport_server", self.make_transport_server)
 
     def make_transport_server(self):
         return test_server.SmartTCPServer_for_testing('-' + self.id())

=== modified file 'bzrlib/tests/test_smart_transport.py'
--- bzrlib/tests/test_smart_transport.py	2010-07-01 15:25:41 +0000
+++ bzrlib/tests/test_smart_transport.py	2010-10-20 23:41:10 +0000
@@ -1496,6 +1496,7 @@
         smart_protocol._has_dispatched = True
         smart_protocol.request = _mod_request.SmartServerRequestHandler(
             None, _mod_request.request_handlers, '/')
+        # GZ 2010-08-10: Cycle with closure affects 4 tests
         class FakeCommand(_mod_request.SmartServerRequest):
             def do_body(self_cmd, body_bytes):
                 self.end_received = True

=== modified file 'bzrlib/tests/test_workingtree_4.py'
--- bzrlib/tests/test_workingtree_4.py	2010-07-21 09:58:42 +0000
+++ bzrlib/tests/test_workingtree_4.py	2010-10-20 23:41:10 +0000
@@ -174,9 +174,9 @@
         # it's given; any calls to forbidden methods will raise an
         # AssertionError
         repo = tree.branch.repository
-        repo.get_revision = self.fail
-        repo.get_inventory = self.fail
-        repo._get_inventory_xml = self.fail
+        self.overrideAttr(repo, "get_revision", self.fail)
+        self.overrideAttr(repo, "get_inventory", self.fail)
+        self.overrideAttr(repo, "_get_inventory_xml", self.fail)
         # try to set the parent trees.
         tree.set_parent_trees([(rev1, rev1_tree)])
 
@@ -214,8 +214,8 @@
         # answer 'get_parent_ids' for the revision tree- dirstate does not
         # cache the parents of a parent tree at this point.
         #repo.get_revision = self.fail
-        repo.get_inventory = self.fail
-        repo._get_inventory_xml = self.fail
+        self.overrideAttr(repo, "get_inventory", self.fail)
+        self.overrideAttr(repo, "_get_inventory_xml", self.fail)
         # set the parent trees.
         tree.set_parent_trees([(rev1, rev1_tree), (rev2, rev2_tree)])
         # read the first tree