Merge lp:~spiv/bzr/traceback-accumulation-2.2 into lp:bzr/2.2

[Andrew Bennetts]

More robust fix for TestCase cloning, this time with tests.

Robert pointed out that my simple TestCase.__copy__ would break tests that were created by repeated application of apply_scenarios. I don't think there are any cases of this in bzr's own test suite (we use multiply_scenarios instead, so there's still just one application of a scenario list to a series of tests), but it could break plugins and invites trouble.

So this patch implements __copy__ as:

 * shallow copy the instance __dict__
 * deep copy individual attributes on a hard-coded blacklist
 * create a new instance (skipping __init__) and give it the new __dict__

In addition, this adds two new tests to test_selftest to catch both the original problem and Robert's concern. I feel compelled to point out that if such tests had existed in the first place we wouldn't have this problem now ;)

Finally, this tweaks the relevant NEWS entry.

[Robert Collins (lifeless)]

I think this is really rather in the wrong place - testtools is the source of the bug.

[Andrew Bennetts (spiv)]

As the comment in the patch says this is something I think testtools ought to provide, but it seems to me that as a practical matter it's easiest to include a fix in bzrlib that works even with older, unfixed testtools. We often take the same approach with other dependencies, like Python itself. I realise that testtools takes patches and releases new versions more frequently and readily than Python, but it's considerably more effort to get all of PQM, babune, and developers' systems updated with a new testtools than it is to just put a fix in bzr now, and allow those to be updated at leisure.

[Martin Pool (mbp)]

I think it's ok to have a workaround of this size in bzr, as long as
there's an explanation why.

I think needing to override __copy__ is kind of a bad smell in Python.

[lp:~spiv/bzr/traceback-accumulation-2.2]

5089. By Andrew Bennetts on 2010-09-15

Restore accidentally deleted test docstring.

5090. By Andrew Bennetts on 2010-09-15

Add detailed comment explaining the __copy__ method.

[Andrew Bennetts (spiv)]

I've added a comment to __copy__ to explain why it's there (and how it works).

[Andrew Bennetts (spiv)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-09-14 15:09:47 +0000
+++ NEWS	2010-09-15 06:47:43 +0000
@@ -75,10 +75,10 @@
 *******
 
 * Tracebacks from a parameterized test are no longer reported against every
-  parameterization of that test.  This was done by adding a simple
+  parameterization of that test.  This was done by adding a custom
   ``__copy__`` method to TestCase, so that ``multiply_tests`` no longer
-  causes testtools.TestCase instances to share a details dict.
-  (Andrew Bennetts)
+  causes testtools.TestCase instances to share a details dict (or other
+  mutable attributes).  (Andrew Bennetts, #625574)
 
 
 bzr 2.2

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-09-14 02:23:34 +0000
+++ bzrlib/tests/__init__.py	2010-09-15 06:47:43 +0000
@@ -799,9 +799,17 @@
     _log_file_name = None
     # record lsprof data when performing benchmark calls.
     _gather_lsprof_in_benchmarks = False
+    # Mutable attributes defined in bzrlib.tests.TestCase and
+    # testtools.TestCase that should not be shallow copied by clone_test.
+    # This is ugly, but probably the best we can do until testtools provide a
+    # nicer way to deal with this, see
+    # <https://bugs.launchpad.net/testtools/+bug/637725>.
+    _attrs_to_deepcopy = (
+        '_cleanups', 'exception_handlers', '_unique_id_gen',
+        '_traceback_id_gen', '_TestCase__details',
+        '_TestCase__exception_handlers')
 
     def __init__(self, methodName='testMethod'):
-        self.__methodName = methodName
         super(TestCase, self).__init__(methodName)
         self._cleanups = []
         self._directory_isolation = True
@@ -811,7 +819,23 @@
             (TestNotApplicable, self._do_not_applicable))
 
     def __copy__(self):
-        return self.__class__(self.__methodName)
+        # XXX: This method works around the lack of a way to correctly clone a
+        # test with current testtools.  See
+        # <https://bugs.launchpad.net/testtools/+bug/637725>.
+        # The work around is to:
+        #  - shallow copy self.__dict__
+        #  - deep copy individual attributes in the _attrs_to_deepcopy black
+        #    list.
+        #  - create a new instance (skipping __init__) with the new __dict__.
+        attrs = self.__dict__.copy()
+        for attr_name in self._attrs_to_deepcopy:
+            if attr_name in attrs:
+                attrs[attr_name] = copy.deepcopy(attrs[attr_name])
+        # Some Python voodoo to create an instance without invoking its
+        # __init__, because we've already constructed the __dict__ to use.
+        new_instance = self.__class__.__new__(self.__class__)
+        new_instance.__dict__ = attrs
+        return new_instance
 
     def setUp(self):
         super(TestCase, self).setUp()

=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2010-09-14 07:12:47 +0000
+++ bzrlib/tests/test_selftest.py	2010-09-15 06:47:43 +0000
@@ -26,6 +26,7 @@
 import warnings
 
 from testtools import MultiTestResult
+from testtools.content import Content
 from testtools.content_type import ContentType
 from testtools.matchers import (
     DocTestMatches,
@@ -42,7 +43,6 @@
     lockdir,
     memorytree,
     osutils,
-    progress,
     remote,
     repository,
     symbol_versioning,
@@ -1655,6 +1655,40 @@
         self.assertEqual('original', obj.test_attr)
 
 
+class TestTestCloning(tests.TestCase):
+    """Tests that test cloning of TestCases (as used by multiply_tests)."""
+
+    def test_cloned_testcase_does_not_share_details(self):
+        """A TestCase cloned with clone_test does not share mutable attributes
+        such as details or cleanups.
+        """
+        class Test(tests.TestCase):
+            def test_foo(self):
+                self.addDetail('foo', Content('text/plain', lambda: 'foo'))
+        orig_test = Test('test_foo')
+        cloned_test = tests.clone_test(orig_test, orig_test.id() + '(cloned)')
+        orig_test.run(unittest.TestResult())
+        self.assertEqual('foo', orig_test.getDetails()['foo'].iter_bytes())
+        self.assertEqual(None, cloned_test.getDetails().get('foo'))
+
+    def test_double_apply_scenario_preserves_first_scenario(self):
+        """Applying two levels of scenarios to a test preserves the attributes
+        added by both scenarios.
+        """
+        class Test(tests.TestCase):
+            def test_foo(self):
+                pass
+        test = Test('test_foo')
+        scenarios_x = [('x=1', {'x': 1}), ('x=2', {'x': 2})]
+        scenarios_y = [('y=1', {'y': 1}), ('y=2', {'y': 2})]
+        suite = tests.multiply_tests(test, scenarios_x, unittest.TestSuite())
+        suite = tests.multiply_tests(suite, scenarios_y, unittest.TestSuite())
+        all_tests = list(tests.iter_suite_tests(suite))
+        self.assertLength(4, all_tests)
+        all_xys = sorted((t.x, t.y) for t in all_tests)
+        self.assertEqual([(1, 1), (1, 2), (2, 1), (2, 2)], all_xys)
+
+
 # NB: Don't delete this; it's not actually from 0.11!
 @deprecated_function(deprecated_in((0, 11, 0)))
 def sample_deprecated_function():