Make FixtureResource.reset actually call fixture.reset().

5 files changed, 129 insertions, 32 deletions

diff --git a/NEWS b/NEWS
index 6859671..c68ce24 100644
--- a/NEWS
+++ b/NEWS
@@ -9,8 +9,12 @@ IMPROVEMENTS
 ~~~~~~~~~~~~
 
 * FixtureResource was not triggering cleanups or resets between uses, this is
-  fixed (but will make OptimisingTestSuite trigger resets on every transition
-  between tests). (Robert Collins, James Westby, #1023423)
+  fixed (but doing so cleanly involved a new extension point - ``_reset`` on
+  ``TestResourceManager``. This is called from ``reset`` which should no
+  longer be overridden. (Though overridden versions will still behave correctly
+  - the change is backwards compatible). Lastly two new TestResult methods were
+  added to track reset (as opposed to make and clean).
+  (Robert Collins, James Westby, #1023423)
 
 * TestResourceManager.reset() was not taking dependency dirtiness into
   consideration. (Brian Sutherland, #783488)
diff --git a/README b/README
index 0ccc25f..24a3ac8 100644
--- a/README
+++ b/README
@@ -117,14 +117,15 @@ overriding these methods:
     closing a network connection.  By default this does nothing, which may be
     appropriate for resources that are automatically garbage collected.
 
-``reset``
+``_reset``
     Reset a no-longer-used dirty resource to a clean state.  By default this
     just discards it and creates a new one, but for some resources there may be a
     faster way to reset them.
 
 ``isDirty``
-    Check whether an existing resource is dirty.  By default this just reports whether
-    ``TestResourceManager.dirtied`` has been called.
+    Check whether an existing resource is dirty.  By default this just reports
+    whether ``TestResourceManager.dirtied`` has been called or any of the
+    dependency resources are dirty.
 
 For instance::
 
@@ -206,9 +207,10 @@ unittest.TestResult
 -------------------
 
 testresources will log activity about resource creation and destruction to the
-result object tests are run with. 4 extension methods are looked for:
+result object tests are run with. 6 extension methods are looked for:
 ``startCleanResource``, ``stopCleanResource``, ``startMakeResource``,
-``stopMakeResource``. ``testresources.tests.ResultWithResourceExtensions`` is
+``stopMakeResource``, ``startResetResource`` and finally ``stopResetResource``.
+``testresources.tests.ResultWithResourceExtensions`` is
 an example of a ``TestResult`` with these methods present.
 
 Controlling Resource Reuse
diff --git a/lib/testresources/__init__.py b/lib/testresources/__init__.py
index 217280f..bda974a 100644
--- a/lib/testresources/__init__.py
+++ b/lib/testresources/__init__.py
@@ -546,6 +546,7 @@ class TestResourceManager(object):
 
         :param dependency_resources: A dict mapping name -> resource instance
             for the resources specified as dependencies.
+        :return: The made resource.
         """
         raise NotImplementedError(
             "Override make to construct resources.")
@@ -568,24 +569,71 @@ class TestResourceManager(object):
         return result
 
     def reset(self, old_resource, result=None):
-        """Overridable method to return a clean version of old_resource.
+        """Return a clean version of old_resource.
 
         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.
+        previously been `dirtied` by the helper self._reset() - which is the
+        extension point folk should override to customise reset behaviour.
+
+        This function takes the dependent resource stack into consideration as
+        _make_all and _clean_all do. The inconsistent naming is because reset
+        is part of the public interface, but _make_all and _clean_all is not.
+
+        Note that if a resource A holds a lock or other blocking thing on
+        a dependency D, reset will result in this call sequence over a 
+        getResource(), dirty(), getResource(), finishedWith(), finishedWith()
+        sequence:
+        B.make(), A.make(), B.reset(), A.reset(), A.clean(), B.clean()
+        Thus it is important that B.reset not assume that A has been cleaned or
+        reset before B is reset: it should arrange to reference count, lazy
+        cleanup or forcibly reset resource in some fashion.
+
+        As an example, consider that B is a database with sample data, and
+        A is an application server serving content from it. B._reset() should
+        disconnect all database clients, reset the state of the database, and
+        A._reset() should tell the application server to dump any internal
+        caches it might have.
+
+        In principle we might make a richer API to allow before-and-after
+        reset actions, but so far that hasn't been needed.
+
+        :return: The possibly new resource.
         :param result: An optional TestResult to report resource changes to.
         """
-        if self.isDirty():
-            self._clean_all(old_resource, result)
-            resource = self._make_all(result)
-        else:
-            resource = old_resource
+        # Core logic:
+        #  - if neither we nor any parent is dirty, do nothing.
+        # otherwise
+        #  - emit a signal to the test result
+        #  - reset all dependencies all, getting new attributes.
+        #  - call self._reset(old_resource, dependency_attributes)
+        #    [the default implementation does a clean + make]
+        if not self.isDirty():
+            return old_resource
+        self._call_result_method_if_exists(result, "startResetResource", self)
+        dependency_resources = {}
+        for name, mgr in self.resources:
+            dependency_resources[name] = mgr.reset(
+                getattr(old_resource, name), result)
+        resource = self._reset(old_resource, dependency_resources)
+        for name, value in dependency_resources.items():
+            setattr(resource, name, value)
+        self._call_result_method_if_exists(result, "stopResetResource", self)
         return resource
 
+    def _reset(self, resource, dependency_resources):
+        """Override this to reset resources other than via clean+make.
+
+        This method should reset the self._dirty flag (assuming the manager can
+        ever be clean) and return either the old resource cleaned or a fresh
+        one.
+
+        :param resource: The resource to reset.
+        :param dependency_resources: A dict mapping name -> resource instance
+            for the resources specified as dependencies.
+        """
+        self.clean(resource)
+        return self.make(dependency_resources)
+
     def _setResource(self, new_resource):
         """Set the current resource to a new value."""
         self._currentResource = new_resource
@@ -671,6 +719,10 @@ class FixtureResource(TestResourceManager):
         self.fixture.setUp()
         return self.fixture
 
+    def _reset(self, resource, dependency_resources):
+        self.fixture.reset()
+        return self.fixture
+
     def isDirty(self):
         return True
 
diff --git a/lib/testresources/tests/__init__.py b/lib/testresources/tests/__init__.py
index 06b6930..c20e4df 100644
--- a/lib/testresources/tests/__init__.py
+++ b/lib/testresources/tests/__init__.py
@@ -58,3 +58,9 @@ class ResultWithResourceExtensions(TestResult):
 
     def stopMakeResource(self, resource):
         self._calls.append(("make", "stop", resource))
+
+    def startResetResource(self, resource):
+        self._calls.append(("reset", "start", resource))
+
+    def stopResetResource(self, resource):
+        self._calls.append(("reset", "stop", resource))
diff --git a/lib/testresources/tests/test_test_resource.py b/lib/testresources/tests/test_test_resource.py
index 02524e7..5ef7fc7 100644
--- a/lib/testresources/tests/test_test_resource.py
+++ b/lib/testresources/tests/test_test_resource.py
@@ -46,11 +46,11 @@ class MockResourceInstance(object):
         return self._name
 
 
-class MockResource(testresources.TestResource):
+class MockResource(testresources.TestResourceManager):
     """Mock resource that logs the number of make and clean calls."""
 
     def __init__(self):
-        testresources.TestResource.__init__(self)
+        super(MockResource, self).__init__()
         self.makes = 0
         self.cleans = 0
 
@@ -66,12 +66,13 @@ class MockResettableResource(MockResource):
     """Mock resource that logs the number of reset calls too."""
 
     def __init__(self):
-        MockResource.__init__(self)
+        super(MockResettableResource, self).__init__()
         self.resets = 0
 
-    def reset(self, resource, result):
+    def _reset(self, resource, dependency_resources):
         self.resets += 1
         resource._name += "!"
+        self._dirty = False
         return resource
 
 
@@ -214,14 +215,16 @@ class TestTestResource(testtools.TestCase):
 
     def testIsResetIfDependenciesAreDirty(self):
         resource_manager = MockResource()
-        dep1 = MockResource()
+        dep1 = MockResettableResource()
         resource_manager.resources.append(("dep1", dep1))
         r = resource_manager.getResource()
         dep1.dirtied(r.dep1)
-        # if we get the resource again, it should be clean
+        # if we get the resource again, it should be cleaned.
         r = resource_manager.getResource()
         self.assertFalse(resource_manager.isDirty())
         self.assertFalse(dep1.isDirty())
+        resource_manager.finishedWith(r)
+        resource_manager.finishedWith(r)
 
     def testUsedResourceResetBetweenUses(self):
         resource_manager = MockResettableResource()
@@ -294,8 +297,6 @@ class TestTestResource(testtools.TestCase):
         self.assertIs(resource, resource_manager._currentResource)
         resource_manager.finishedWith(resource)
 
-    # 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()
@@ -334,6 +335,39 @@ class TestTestResource(testtools.TestCase):
         self.assertEqual(2, resource_manager.makes)
         self.assertEqual(1, resource_manager.cleans)
 
+    def testDefaultResetResetsDependencies(self):
+        resource_manager = MockResettableResource()
+        dep1 = MockResettableResource()
+        dep2 = MockResettableResource()
+        resource_manager.resources.append(("dep1", dep1))
+        resource_manager.resources.append(("dep2", dep2))
+        # A typical OptimisingTestSuite workflow
+        r_outer = resource_manager.getResource()
+        # test 1
+        r_inner = resource_manager.getResource()
+        dep2.dirtied(r_inner.dep2)
+        resource_manager.finishedWith(r_inner)
+        # test 2
+        r_inner = resource_manager.getResource()
+        dep2.dirtied(r_inner.dep2)
+        resource_manager.finishedWith(r_inner)
+        resource_manager.finishedWith(r_outer)
+        # Dep 1 was clean, doesn't do a reset, and should only have one
+        # make+clean.
+        self.assertEqual(1, dep1.makes)
+        self.assertEqual(1, dep1.cleans)
+        self.assertEqual(0, dep1.resets)
+        # Dep 2 was dirty, so _reset happens, and likewise only one make and
+        # clean.
+        self.assertEqual(1, dep2.makes)
+        self.assertEqual(1, dep2.cleans)
+        self.assertEqual(1, dep2.resets)
+        # The top layer should have had a reset happen, and only one make and
+        # clean.
+        self.assertEqual(1, resource_manager.makes)
+        self.assertEqual(1, resource_manager.cleans)
+        self.assertEqual(1, resource_manager.resets)
+
     def testDirtyingWhenUnused(self):
         resource_manager = MockResource()
         resource = resource_manager.getResource()
@@ -388,10 +422,9 @@ class TestTestResource(testtools.TestCase):
     def testResetActivityForResourceWithExtensions(self):
         result = ResultWithResourceExtensions()
         resource_manager = MockResource()
-        expected = [("clean", "start", resource_manager),
-            ("clean", "stop", resource_manager),
-            ("make", "start", resource_manager),
-            ("make", "stop", resource_manager)]
+        expected = [("reset", "start", resource_manager),
+            ("reset", "stop", resource_manager),
+            ]
         resource_manager.getResource()
         r = resource_manager.getResource()
         resource_manager.dirtied(r)
@@ -488,4 +521,4 @@ class TestFixtureResource(testtools.TestCase):
         mgr.reset(resource)
         mgr.finishedWith(resource)
         self.assertEqual(
-            ['setUp', 'cleanUp', 'setUp', 'cleanUp'], fixture.calls)
+            ['setUp', 'reset', 'cleanUp'], fixture.calls)