Bazaar

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

  1. traceback-accumulation-2.2
  2. Merge into 2.2
Proposed by Andrew Bennetts
Status: Merged
Approved by: Andrew Bennetts
Approved revision: no longer in the source branch.
Merged at revision: 5090
Proposed branch: lp:~spiv/bzr/traceback-accumulation-2.2
Merge into: lp:bzr/2.2
Diff against target: 126 lines (+64/-6)
3 files modified
NEWS (+3/-3)
bzrlib/tests/__init__.py (+26/-2)
bzrlib/tests/test_selftest.py (+35/-1)
To merge this branch: bzr merge lp:~spiv/bzr/traceback-accumulation-2.2
Reviewer Review Type Date Requested Status
bzr-core Pending
Review via email: mp+35494@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Andrew Bennetts (spiv) wrote :

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.

Revision history for this message
Martin Pool (mbp) wrote :

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.

Revision history for this message
Andrew Bennetts (spiv) wrote :

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

Revision history for this message
Andrew Bennetts (spiv) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-09-14 15:09:47 +0000
+++ NEWS 2010-09-15 06:47:43 +0000
@@ -75,10 +75,10 @@
75*******75*******
7676
77* Tracebacks from a parameterized test are no longer reported against every77* Tracebacks from a parameterized test are no longer reported against every
78 parameterization of that test. This was done by adding a simple78 parameterization of that test. This was done by adding a custom
79 ``__copy__`` method to TestCase, so that ``multiply_tests`` no longer79 ``__copy__`` method to TestCase, so that ``multiply_tests`` no longer
80 causes testtools.TestCase instances to share a details dict.80 causes testtools.TestCase instances to share a details dict (or other
81 (Andrew Bennetts)81 mutable attributes). (Andrew Bennetts, #625574)
8282
8383
84bzr 2.284bzr 2.2
8585
=== 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 @@
799 _log_file_name = None799 _log_file_name = None
800 # record lsprof data when performing benchmark calls.800 # record lsprof data when performing benchmark calls.
801 _gather_lsprof_in_benchmarks = False801 _gather_lsprof_in_benchmarks = False
802 # Mutable attributes defined in bzrlib.tests.TestCase and
803 # testtools.TestCase that should not be shallow copied by clone_test.
804 # This is ugly, but probably the best we can do until testtools provide a
805 # nicer way to deal with this, see
806 # <https://bugs.launchpad.net/testtools/+bug/637725>.
807 _attrs_to_deepcopy = (
808 '_cleanups', 'exception_handlers', '_unique_id_gen',
809 '_traceback_id_gen', '_TestCase__details',
810 '_TestCase__exception_handlers')
802811
803 def __init__(self, methodName='testMethod'):812 def __init__(self, methodName='testMethod'):
804 self.__methodName = methodName
805 super(TestCase, self).__init__(methodName)813 super(TestCase, self).__init__(methodName)
806 self._cleanups = []814 self._cleanups = []
807 self._directory_isolation = True815 self._directory_isolation = True
@@ -811,7 +819,23 @@
811 (TestNotApplicable, self._do_not_applicable))819 (TestNotApplicable, self._do_not_applicable))
812820
813 def __copy__(self):821 def __copy__(self):
814 return self.__class__(self.__methodName)822 # XXX: This method works around the lack of a way to correctly clone a
823 # test with current testtools. See
824 # <https://bugs.launchpad.net/testtools/+bug/637725>.
825 # The work around is to:
826 # - shallow copy self.__dict__
827 # - deep copy individual attributes in the _attrs_to_deepcopy black
828 # list.
829 # - create a new instance (skipping __init__) with the new __dict__.
830 attrs = self.__dict__.copy()
831 for attr_name in self._attrs_to_deepcopy:
832 if attr_name in attrs:
833 attrs[attr_name] = copy.deepcopy(attrs[attr_name])
834 # Some Python voodoo to create an instance without invoking its
835 # __init__, because we've already constructed the __dict__ to use.
836 new_instance = self.__class__.__new__(self.__class__)
837 new_instance.__dict__ = attrs
838 return new_instance
815839
816 def setUp(self):840 def setUp(self):
817 super(TestCase, self).setUp()841 super(TestCase, self).setUp()
818842
=== 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 @@
26import warnings26import warnings
2727
28from testtools import MultiTestResult28from testtools import MultiTestResult
29from testtools.content import Content
29from testtools.content_type import ContentType30from testtools.content_type import ContentType
30from testtools.matchers import (31from testtools.matchers import (
31 DocTestMatches,32 DocTestMatches,
@@ -42,7 +43,6 @@
42 lockdir,43 lockdir,
43 memorytree,44 memorytree,
44 osutils,45 osutils,
45 progress,
46 remote,46 remote,
47 repository,47 repository,
48 symbol_versioning,48 symbol_versioning,
@@ -1655,6 +1655,40 @@
1655 self.assertEqual('original', obj.test_attr)1655 self.assertEqual('original', obj.test_attr)
16561656
16571657
1658class TestTestCloning(tests.TestCase):
1659 """Tests that test cloning of TestCases (as used by multiply_tests)."""
1660
1661 def test_cloned_testcase_does_not_share_details(self):
1662 """A TestCase cloned with clone_test does not share mutable attributes
1663 such as details or cleanups.
1664 """
1665 class Test(tests.TestCase):
1666 def test_foo(self):
1667 self.addDetail('foo', Content('text/plain', lambda: 'foo'))
1668 orig_test = Test('test_foo')
1669 cloned_test = tests.clone_test(orig_test, orig_test.id() + '(cloned)')
1670 orig_test.run(unittest.TestResult())
1671 self.assertEqual('foo', orig_test.getDetails()['foo'].iter_bytes())
1672 self.assertEqual(None, cloned_test.getDetails().get('foo'))
1673
1674 def test_double_apply_scenario_preserves_first_scenario(self):
1675 """Applying two levels of scenarios to a test preserves the attributes
1676 added by both scenarios.
1677 """
1678 class Test(tests.TestCase):
1679 def test_foo(self):
1680 pass
1681 test = Test('test_foo')
1682 scenarios_x = [('x=1', {'x': 1}), ('x=2', {'x': 2})]
1683 scenarios_y = [('y=1', {'y': 1}), ('y=2', {'y': 2})]
1684 suite = tests.multiply_tests(test, scenarios_x, unittest.TestSuite())
1685 suite = tests.multiply_tests(suite, scenarios_y, unittest.TestSuite())
1686 all_tests = list(tests.iter_suite_tests(suite))
1687 self.assertLength(4, all_tests)
1688 all_xys = sorted((t.x, t.y) for t in all_tests)
1689 self.assertEqual([(1, 1), (1, 2), (2, 1), (2, 2)], all_xys)
1690
1691
1658# NB: Don't delete this; it's not actually from 0.11!1692# NB: Don't delete this; it's not actually from 0.11!
1659@deprecated_function(deprecated_in((0, 11, 0)))1693@deprecated_function(deprecated_in((0, 11, 0)))
1660def sample_deprecated_function():1694def sample_deprecated_function():

Subscribers

People subscribed via source and target branches