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
1=== modified file 'NEWS'
2--- NEWS 2010-09-14 15:09:47 +0000
3+++ NEWS 2010-09-15 06:47:43 +0000
4@@ -75,10 +75,10 @@
5 *******
6
7 * Tracebacks from a parameterized test are no longer reported against every
8- parameterization of that test. This was done by adding a simple
9+ parameterization of that test. This was done by adding a custom
10 ``__copy__`` method to TestCase, so that ``multiply_tests`` no longer
11- causes testtools.TestCase instances to share a details dict.
12- (Andrew Bennetts)
13+ causes testtools.TestCase instances to share a details dict (or other
14+ mutable attributes). (Andrew Bennetts, #625574)
15
16
17 bzr 2.2
18
19=== modified file 'bzrlib/tests/__init__.py'
20--- bzrlib/tests/__init__.py 2010-09-14 02:23:34 +0000
21+++ bzrlib/tests/__init__.py 2010-09-15 06:47:43 +0000
22@@ -799,9 +799,17 @@
23 _log_file_name = None
24 # record lsprof data when performing benchmark calls.
25 _gather_lsprof_in_benchmarks = False
26+ # Mutable attributes defined in bzrlib.tests.TestCase and
27+ # testtools.TestCase that should not be shallow copied by clone_test.
28+ # This is ugly, but probably the best we can do until testtools provide a
29+ # nicer way to deal with this, see
30+ # <https://bugs.launchpad.net/testtools/+bug/637725>.
31+ _attrs_to_deepcopy = (
32+ '_cleanups', 'exception_handlers', '_unique_id_gen',
33+ '_traceback_id_gen', '_TestCase__details',
34+ '_TestCase__exception_handlers')
35
36 def __init__(self, methodName='testMethod'):
37- self.__methodName = methodName
38 super(TestCase, self).__init__(methodName)
39 self._cleanups = []
40 self._directory_isolation = True
41@@ -811,7 +819,23 @@
42 (TestNotApplicable, self._do_not_applicable))
43
44 def __copy__(self):
45- return self.__class__(self.__methodName)
46+ # XXX: This method works around the lack of a way to correctly clone a
47+ # test with current testtools. See
48+ # <https://bugs.launchpad.net/testtools/+bug/637725>.
49+ # The work around is to:
50+ # - shallow copy self.__dict__
51+ # - deep copy individual attributes in the _attrs_to_deepcopy black
52+ # list.
53+ # - create a new instance (skipping __init__) with the new __dict__.
54+ attrs = self.__dict__.copy()
55+ for attr_name in self._attrs_to_deepcopy:
56+ if attr_name in attrs:
57+ attrs[attr_name] = copy.deepcopy(attrs[attr_name])
58+ # Some Python voodoo to create an instance without invoking its
59+ # __init__, because we've already constructed the __dict__ to use.
60+ new_instance = self.__class__.__new__(self.__class__)
61+ new_instance.__dict__ = attrs
62+ return new_instance
63
64 def setUp(self):
65 super(TestCase, self).setUp()
66
67=== modified file 'bzrlib/tests/test_selftest.py'
68--- bzrlib/tests/test_selftest.py 2010-09-14 07:12:47 +0000
69+++ bzrlib/tests/test_selftest.py 2010-09-15 06:47:43 +0000
70@@ -26,6 +26,7 @@
71 import warnings
72
73 from testtools import MultiTestResult
74+from testtools.content import Content
75 from testtools.content_type import ContentType
76 from testtools.matchers import (
77 DocTestMatches,
78@@ -42,7 +43,6 @@
79 lockdir,
80 memorytree,
81 osutils,
82- progress,
83 remote,
84 repository,
85 symbol_versioning,
86@@ -1655,6 +1655,40 @@
87 self.assertEqual('original', obj.test_attr)
88
89
90+class TestTestCloning(tests.TestCase):
91+ """Tests that test cloning of TestCases (as used by multiply_tests)."""
92+
93+ def test_cloned_testcase_does_not_share_details(self):
94+ """A TestCase cloned with clone_test does not share mutable attributes
95+ such as details or cleanups.
96+ """
97+ class Test(tests.TestCase):
98+ def test_foo(self):
99+ self.addDetail('foo', Content('text/plain', lambda: 'foo'))
100+ orig_test = Test('test_foo')
101+ cloned_test = tests.clone_test(orig_test, orig_test.id() + '(cloned)')
102+ orig_test.run(unittest.TestResult())
103+ self.assertEqual('foo', orig_test.getDetails()['foo'].iter_bytes())
104+ self.assertEqual(None, cloned_test.getDetails().get('foo'))
105+
106+ def test_double_apply_scenario_preserves_first_scenario(self):
107+ """Applying two levels of scenarios to a test preserves the attributes
108+ added by both scenarios.
109+ """
110+ class Test(tests.TestCase):
111+ def test_foo(self):
112+ pass
113+ test = Test('test_foo')
114+ scenarios_x = [('x=1', {'x': 1}), ('x=2', {'x': 2})]
115+ scenarios_y = [('y=1', {'y': 1}), ('y=2', {'y': 2})]
116+ suite = tests.multiply_tests(test, scenarios_x, unittest.TestSuite())
117+ suite = tests.multiply_tests(suite, scenarios_y, unittest.TestSuite())
118+ all_tests = list(tests.iter_suite_tests(suite))
119+ self.assertLength(4, all_tests)
120+ all_xys = sorted((t.x, t.y) for t in all_tests)
121+ self.assertEqual([(1, 1), (1, 2), (2, 1), (2, 2)], all_xys)
122+
123+
124 # NB: Don't delete this; it's not actually from 0.11!
125 @deprecated_function(deprecated_in((0, 11, 0)))
126 def sample_deprecated_function():

Subscribers

People subscribed via source and target branches