← Back to team overview

testtools-dev team mailing list archive

[Merge] lp:~spiv/testtools/cloned-details-637725 into lp:testtools

 

Andrew Bennetts has proposed merging lp:~spiv/testtools/cloned-details-637725 into lp:testtools.

Requested reviews:
  testtools developers (testtools-dev)
Related bugs:
  #637725 clone_test_with_new_id shares details dict between test case instances
  https://bugs.launchpad.net/bugs/637725


This is a minimal fix for the accidental sharing of a single details dict among cloned TestCases.  It works by making self.__details get initialised in a lazy fashion by addDetails/getDetails.

I'm a bit uncomfortable that it ignores the larger question of how cloning should behave with other mutable state a freshly-initialised TestCase could have.  e.g. self._cleanups is shared... but because _runCleanups should always empty it completely that doesn't have any effect — unless a subclass does add something to it during __init__, or cloned tests are run concurrently in threads.  And there are other minor issues I can see, such as self._unique_id_gen will be effectively set to itertools.count(0) in a clone (because copy.copy seems to always produce a count(0) regardless of the state).

So I don't think this is the end of the story here, but it's a simple patch that solves the most obvious problem... and it has a test :)
-- 
https://code.launchpad.net/~spiv/testtools/cloned-details-637725/+merge/35780
Your team testtools developers is requested to review the proposed merge of lp:~spiv/testtools/cloned-details-637725 into lp:testtools.
=== modified file 'testtools/testcase.py'
--- testtools/testcase.py	2010-09-12 00:36:10 +0000
+++ testtools/testcase.py	2010-09-17 06:36:52 +0000
@@ -84,7 +84,9 @@
         self._traceback_id_gen = itertools.count(0)
         self.__setup_called = False
         self.__teardown_called = False
-        self.__details = {}
+        # __details is lazy-initialized so that a constructed-but-not-run
+        # TestCase is safe to use with clone_test_with_new_id.
+        self.__details = None
         self.__RunTest = kwargs.get('runTest', RunTest)
         self.__exception_handlers = []
         self.exception_handlers = [
@@ -117,6 +119,8 @@
         :param content_object: The content object for this detail. See
             testtools.content for more detail.
         """
+        if self.__details is None:
+            self.__details = {}
         self.__details[name] = content_object
 
     def getDetails(self):
@@ -124,6 +128,8 @@
 
         For more details see pydoc testtools.TestResult.
         """
+        if self.__details is None:
+            self.__details = {}
         return self.__details
 
     def patch(self, obj, attribute, value):

=== modified file 'testtools/tests/test_testtools.py'
--- testtools/tests/test_testtools.py	2010-09-12 00:36:10 +0000
+++ testtools/tests/test_testtools.py	2010-09-17 06:36:52 +0000
@@ -770,6 +770,18 @@
         self.assertEqual(oldName, test.id(),
             "the original test instance should be unchanged.")
 
+    def test_cloned_testcase_does_not_share_details(self):
+        """A cloned TestCase does not share the details dict."""
+        class Test(TestCase):
+            def test_foo(self):
+                self.addDetail(
+                    'foo', content.Content('text/plain', lambda: 'foo'))
+        orig_test = Test('test_foo')
+        cloned_test = clone_test_with_new_id(orig_test, self.getUniqueString())
+        orig_test.run(unittest.TestResult())
+        self.assertEqual('foo', orig_test.getDetails()['foo'].iter_bytes())
+        self.assertEqual(None, cloned_test.getDetails().get('foo'))
+
 
 class TestDetailsProvided(TestWithDetails):
 


Follow ups