Merge lp:~lifeless/testresources/bug-271619 into lp:~testresources-developers/testresources/trunk

[Robert Collins]

[Jonathan Lange (jml)]

I think this patch is fundamentally correct and adds a useful feature.

However, I think the docstring for `reset` should be changed so that the first line says something like, "Return a clean version of 'old_resource'". That is, it should describe what the method does.

The docstring should also provide some hints as to *why* one might want to override the method. One way to do this would be to follow-up the "By default" sentence with "If there is a faster way of resetting the resource than cleaning it and remaking it, you probably want to override this method to do that." Or something along those lines.

Thanks for the patch & for dealing with this long-dormant bug.

jml

review: Approve

[Robert Collins (lifeless)]

Some views on code only show the first line, and as an intentionally overridable method I think its important to keep that in there. However - "Overridable method to return a clean version of old_resource." should meet your points too.

=== modified file 'NEWS'
--- NEWS	2009-06-17 10:23:42 +0000
+++ NEWS	2009-06-18 10:36:00 +0000
@@ -19,6 +19,11 @@
 
     * Expanded TODO. (Jonathan Lange)
 
+    * Resources can now be reset by overriding TestResource.reset, which for
+      some resources is significantly cheaper. If checking for dirtiness is
+      expensive, isDirty can also be overridden.
+      (James Henstridge, Robert Collins)
+
     * Started keeping a NEWS file! (Jonathan Lange)
 
   BUG FIXES:
 
=== modified file 'lib/testresources/__init__.py'
--- lib/testresources/__init__.py	2009-06-17 10:23:42 +0000
+++ lib/testresources/__init__.py	2009-06-18 10:36:00 +0000
@@ -203,7 +203,7 @@
         self._currentResource = None
         self.resources = list(getattr(self.__class__, "resources", []))
 
-    def clean_all(self, resource):
+    def _clean_all(self, resource):
         """Clean the dependencies from resource, and then resource itself."""
         self.clean(resource)
         for name, manager in self.resources:
@@ -233,7 +233,7 @@
         """
         self._uses -= 1
         if self._uses == 0:
-            self.clean_all(resource)
+            self._clean_all(resource)
             self._setResource(None)
 
     def getResource(self):
@@ -245,9 +245,9 @@
         that it is no longer needed.
         """
         if self._uses == 0:
-            self._setResource(self.make_all())
+            self._setResource(self._make_all())
         elif self.isDirty():
-            self._resetResource(self._currentResource)
+            self._setResource(self.reset(self._currentResource))
         self._uses += 1
         return self._currentResource
 
@@ -269,7 +269,7 @@
             finally:
                 mgr.finishedWith(res)
 
-    def make_all(self):
+    def _make_all(self):
         """Make the dependencies of this resource and this resource."""
         dependency_resources = {}
         for name, resource in self.resources:
@@ -305,9 +305,23 @@
         result.append(self)
         return result
 
-    def _resetResource(self, old_resource):
-        self.clean_all(old_resource)
-        self._setResource(self.make_all())
+    def reset(self, old_resource):
+        """Override this to reset resources.
+
+        By default, the resource will be cleaned then remade if it had
+        previously been `dirtied`.
+
+        This function needs to take the dependent resource stack into
+        consideration as _make_all and _clean_all do.
+
+        :return: The new resource.
+        """
+        if self._dirty:
+            self._clean_all(old_resource)
+            resource = self._make_all()
+        else:
+            resource = old_resource
+        return resource
 
     def _setResource(self, new_resource):
         """Set the current resource to a new value."""
 
=== modified file 'lib/testresources/tests/test_test_resource.py'
--- lib/testresources/tests/test_test_resource.py	2009-06-17 10:23:42 +0000
+++ lib/testresources/tests/test_test_resource.py	2009-06-18 10:36:00 +0000
@@ -57,6 +57,19 @@
         return MockResourceInstance("Boo!")
 
 
+class MockResettableResource(MockResource):
+    """Mock resource that logs the number of reset calls too."""
+
+    def __init__(self):
+        MockResource.__init__(self)
+        self.resets = 0
+
+    def reset(self, resource):
+        self.resets += 1
+        resource._name += "!"
+        return resource
+
+
 class TestTestResource(testtools.TestCase):
 
     def testUnimplementedGetResource(self):
@@ -183,6 +196,33 @@
         resource_manager.getResource()
         self.assertEqual(1, resource_manager.makes)
 
+    def testGetResourceResetsUsedResource(self):
+        resource_manager = MockResettableResource()
+        resource_manager.getResource()
+        resource = resource_manager.getResource()
+        self.assertEqual(1, resource_manager.makes)
+        resource_manager.dirtied(resource)
+        resource_manager.getResource()
+        self.assertEqual(1, resource_manager.makes)
+        self.assertEqual(1, resource_manager.resets)
+        resource_manager.finishedWith(resource)
+
+    def testUsedResourceResetBetweenUses(self):
+        resource_manager = MockResettableResource()
+        # take two refs; like happens with OptimisingTestSuite.
+        resource_manager.getResource()
+        resource = resource_manager.getResource()
+        resource_manager.dirtied(resource)
+        resource_manager.finishedWith(resource)
+        # Get again, but its been dirtied.
+        resource = resource_manager.getResource()
+        resource_manager.finishedWith(resource)
+        resource_manager.finishedWith(resource)
+        # The resource is made once, reset once and cleaned once.
+        self.assertEqual(1, resource_manager.makes)
+        self.assertEqual(1, resource_manager.resets)
+        self.assertEqual(1, resource_manager.cleans)
+
     def testFinishedWithDecrementsUses(self):
         resource_manager = MockResource()
         resource = resource_manager.getResource()
@@ -237,6 +277,8 @@
         resource_manager.finishedWith(resource)
         self.assertIs(resource, resource_manager._currentResource)
 
+    # The default implementation of reset() performs a make/clean if
+    # the dirty flag is set.
     def testDirtiedSetsDirty(self):
         resource_manager = MockResource()
         resource = resource_manager.getResource()
@@ -257,15 +299,23 @@
         resource_manager.finishedWith(resource1)
         self.assertEqual(2, resource_manager.cleans)
 
-    def testDirtyingResourceTriggersRemake(self):
+    def testDefaultResetMethodPreservesCleanResource(self):
+        resource_manager = MockResource()
+        resource = resource_manager.getResource()
+        self.assertEqual(1, resource_manager.makes)
+        self.assertEqual(False, resource_manager._dirty)
+        resource_manager.reset(resource)
+        self.assertEqual(1, resource_manager.makes)
+        self.assertEqual(0, resource_manager.cleans)
+
+    def testDefaultResetMethodRecreatesDirtyResource(self):
         resource_manager = MockResource()
         resource = resource_manager.getResource()
         self.assertEqual(1, resource_manager.makes)
         resource_manager.dirtied(resource)
-        resource_manager.getResource()
+        resource_manager.reset(resource)
+        self.assertEqual(2, resource_manager.makes)
         self.assertEqual(1, resource_manager.cleans)
-        self.assertEqual(2, resource_manager.makes)
-        self.assertEqual(False, resource_manager._dirty)
 
     def testDirtyingWhenUnused(self):
         resource_manager = MockResource()