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

[Robert Collins]

[James Henstridge (jamesh)]

I'm not sure of the exact uses Jonathan had for this API, but this does not look like what I'd have expected.

To turn on tracing with this API, it looks like I'd have to look through the entire test suite to find out where all the resource manages get instantiated and change the constructors. I'd have expected something closer to slotting in a different TestResult implementation into the test runner: something that can be done in one place and affects all resources. I'm not sure about the best way to do this though.

Second, it would be better to pass the TestResource instance and event type to the tracer rather than a formatted string. This would make it easier to do things like collect statistics about resource use and give the tracer a choice about how to format things.

Lastly, it might be worth adding a trace point before and after making/cleaning a resource. That would allow using the tracer API to calculate how much time those tasks are taking per-run and overall.

review: Needs Fixing

[Robert Collins (lifeless)]

On Thu, 2009-06-18 at 09:50 +0000, James Henstridge wrote:
>
> I'm not sure of the exact uses Jonathan had for this API, but this
> does not look like what I'd have expected.
>
> To turn on tracing with this API, it looks like I'd have to look
> through the entire test suite to find out where all the resource
> manages get instantiated and change the constructors. I'd have
> expected something closer to slotting in a different TestResult
> implementation into the test runner: something that can be done in one
> place and affects all resources. I'm not sure about the best way to
> do this though.

Neither am I; I felt it was a good idea to get something up and running
to meet the need. Currently there are three points this could be set:
TestResource
OptimisingTestSuite
ResourcedTestCase

Of those:
- RTC is constructed by test discovery and is not positioned to set
state early enough.
- OTS can be got at more easily, but won't suffice if its not being
used.

which leaves TestResource. It can either take a parameter or use global
state. I have a very strong aversion to global state.

So while I agree with the sentiment about how to turn this on, I don't
have any better ideas that will work and be robust.

> Second, it would be better to pass the TestResource instance and event
> type to the tracer rather than a formatted string. This would make it
> easier to do things like collect statistics about resource use and
> give the tracer a choice about how to format things.
>
> Lastly, it might be worth adding a trace point before and after
> making/cleaning a resource. That would allow using the tracer API to
> calculate how much time those tasks are taking per-run and overall.

I will admit to having gone on the 'easiest' approach here; but sure I
can change it to send two parameters.

-Rob

[lp:~lifeless/testresources/bug-284125]

36. By Robert Collins on 2009-06-18

Merge and simply James Henstridge's reset patch.

[Robert Collins (lifeless)]

I've pushed a new commit that sends structured data and surrounds the operations.

I don't have any ideas vis-a-vis easy setup of this, so if noone else does I suggest landing it now as that could always be added later.

[Jonathan Lange (jml)]

In the bug report, you suggest that "we could extend the protocol more deeply and say that we'll emite to the TestResult object being used, IFF it supports an additional function[s] for recording TestResources." I think I like that approach.

However, I think you're right when you suggest that we land this now and incorporate better ideas when we actually have them.

It seems that the docstring for 'trace_function' is out of date. It says "A callable that takes strings to output." However your patch currently calls it with operation, phase & resource manager. Please update the docstring before landing the patch.

review: Approve

[lp:~lifeless/testresources/bug-284125]

37. By Robert Collins on 2009-07-07

Review feedback.

38. By Robert Collins on 2009-07-07

Merge support for trace_functions in TestResource.

=== modified file 'NEWS'
--- NEWS	2009-06-17 10:23:42 +0000
+++ NEWS	2009-06-17 12:01:49 +0000
@@ -21,6 +21,10 @@
 
     * Started keeping a NEWS file! (Jonathan Lange)
 
+    * A trace_function can be supplied when constructing TestResource objects,
+      to allow debugging of when resources are made/cleaned. (Robert Collins,
+      #284125)
+
   BUG FIXES:
 
     * Calling getResource on a dirty resource now triggers a clean and re-make

=== modified file 'lib/testresources/__init__.py'
--- lib/testresources/__init__.py	2009-06-17 10:23:42 +0000
+++ lib/testresources/__init__.py	2009-06-17 12:01:49 +0000
@@ -197,14 +197,22 @@
     setUpCost = 1
     tearDownCost = 1
 
-    def __init__(self):
+    def __init__(self, trace_function=None):
+        """Create a TestResource object.
+
+        :param trace_function: A callable that takes strings to output. This
+            will be called with trace messages when the resource is made and
+            cleaned.
+        """
         self._dirty = False
         self._uses = 0
         self._currentResource = None
         self.resources = list(getattr(self.__class__, "resources", []))
+        self._trace = trace_function or (lambda x:"")
 
-    def clean_all(self, resource):
+    def _clean_all(self, resource):
         """Clean the dependencies from resource, and then resource itself."""
+        self._trace("clean: %s\n" % self)
         self.clean(resource)
         for name, manager in self.resources:
             manager.finishedWith(getattr(resource, name))
@@ -233,7 +241,7 @@
         """
         self._uses -= 1
         if self._uses == 0:
-            self.clean_all(resource)
+            self._clean_all(resource)
             self._setResource(None)
 
     def getResource(self):
@@ -245,7 +253,7 @@
         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._uses += 1
@@ -269,8 +277,9 @@
             finally:
                 mgr.finishedWith(res)
 
-    def make_all(self):
+    def _make_all(self):
         """Make the dependencies of this resource and this resource."""
+        self._trace("make: %s\n" % self)
         dependency_resources = {}
         for name, resource in self.resources:
             dependency_resources[name] = resource.getResource()
@@ -306,8 +315,8 @@
         return result
 
     def _resetResource(self, old_resource):
-        self.clean_all(old_resource)
-        self._setResource(self.make_all())
+        self._clean_all(old_resource)
+        self._setResource(self._make_all())
 
     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-17 12:01:49 +0000
@@ -44,8 +44,8 @@
 class MockResource(testresources.TestResource):
     """Mock resource that logs the number of make and clean calls."""
 
-    def __init__(self):
-        testresources.TestResource.__init__(self)
+    def __init__(self, trace_function=None):
+        testresources.TestResource.__init__(self, trace_function=trace_function)
         self.makes = 0
         self.cleans = 0
 
@@ -275,3 +275,13 @@
         self.assertEqual(1, resource_manager.makes)
         resource = resource_manager.getResource()
         self.assertEqual(2, resource_manager.makes)
+
+    def testTraceFunction(self):
+        output = []
+        resource_manager = MockResource(trace_function=output.append)
+        expected = ["make: %s\n" % resource_manager]
+        r = resource_manager.getResource()
+        self.assertEqual(expected, output)
+        expected.append("clean: %s\n" % resource_manager)
+        resource_manager.finishedWith(r)
+        self.assertEqual(expected, output)