Deal with resource leaks during setUp.

Fixture.setUp should no longer be overridden in subclasses. Instead
override _setUp. This permits the Fixture base class to detect failures
during _setUp and trigger any registered cleanups, attach any details
to the failure exception and propogate that to callers.
(Robert Collins, #1456361, #1456353)

18 files changed, 198 insertions, 59 deletions

diff --git a/NEWS b/NEWS
index e9b2837..bc346a7 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,14 @@ NEXT
 * Add MockPatch, MockPatchMultiple, MockPatchObject - adapters to mock.
   (Julien Danjou, Robert Collins)
 
+* Fixture.setUp should no longer be overridden in subclasses. Instead
+  override _setUp. This permits the Fixture base class to detect failures
+  during _setUp and trigger any registered cleanups, attach any details
+  to the failure exception and propogate that to callers. Overriding of
+  setUp is still supported: this adds a new interface for simpler
+  implementation of the contract of a fixture.
+  (Robert Collins, #1456361, #1456353)
+
 1.2.0
 ~~~~~
 
diff --git a/README b/README
index 3c96032..e963f47 100644
--- a/README
+++ b/README
@@ -79,19 +79,20 @@ Most fixtures have complete ``pydoc`` documentation, so be sure to check
 Creating Fixtures
 =================
 
-Minimally, subclass Fixture, define setUp to initialize your state and schedule
+Minimally, subclass Fixture, define _setUp to initialize your state and schedule
 a cleanup for when cleanUp is called and you're done::
 
   >>> import unittest
   >>> import fixtures
   >>> class NoddyFixture(fixtures.Fixture):
-  ...     def setUp(self):
-  ...         super(NoddyFixture, self).setUp()
+  ...     def _setUp(self):
   ...         self.frobnozzle = 42
   ...         self.addCleanup(delattr, self, 'frobnozzle')
 
-This will initialize frobnozzle when setUp is called, and when cleanUp is
-called get rid of the frobnozzle attribute.
+This will initialize frobnozzle when ``setUp`` is called, and when ``cleanUp``
+is called get rid of the frobnozzle attribute. Prior to version 1.3.0 fixtures
+recommended overriding ``setUp``. This is still supported, but since it is
+harder to write leak-free fixtures in this fashion, it is not recommended.
 
 If your fixture has diagnostic data - for instance the log file of an
 application server, or log messages, it can expose that by creating a content
@@ -99,8 +100,7 @@ object (``testtools.content.Content``) and calling ``addDetail``.
 
   >>> from testtools.content import text_content
   >>> class WithLog(fixtures.Fixture):
-  ...     def setUp(self):
-  ...         super(WithLog, self).setUp()
+  ...     def _setUp(self):
   ...         self.addDetail('message', text_content('foo bar baz'))
 
 The method ``useFixture`` will use another fixture, call ``setUp`` on it, call
@@ -108,8 +108,7 @@ The method ``useFixture`` will use another fixture, call ``setUp`` on it, call
 the fixture. This allows simple composition of different fixtures.
 
   >>> class ReusingFixture(fixtures.Fixture):
-  ...     def setUp(self):
-  ...         super(ReusingFixture, self).setUp()
+  ...     def _setUp(self):
   ...         self.noddy = self.useFixture(NoddyFixture())
 
 There is a helper for adapting a function or function pair into Fixtures. it
@@ -154,9 +153,9 @@ The example above introduces some of the Fixture API. In order to be able to
 clean up after a fixture has been used, all fixtures define a ``cleanUp``
 method which should be called when a fixture is finished with.
 
-Because its nice to be able to build a particular set of related fixtures in
-advance of using them, fixtures also have define a ``setUp`` method which
-should be called before trying to use them.
+Because it's nice to be able to build a particular set of related fixtures in
+advance of using them, fixtures also have a ``setUp`` method which should be
+called before trying to use them.
 
 One common desire with fixtures that are expensive to create is to reuse them
 in many test cases; to support this the base Fixture also defines a ``reset``
@@ -203,8 +202,7 @@ rather than choosing an arbitrary single exception to raise::
   >>> import sys
   >>> from fixtures.fixture import MultipleExceptions
   >>> class BrokenFixture(fixtures.Fixture):
-  ...     def setUp(self):
-  ...         fixtures.Fixture.setUp(self)
+  ...     def _setUp(self):
   ...         self.addCleanup(lambda:1/0)
   ...         self.addCleanup(lambda:1/0)
   >>> fixture = BrokenFixture()
@@ -218,12 +216,25 @@ rather than choosing an arbitrary single exception to raise::
 
 Fixtures often expose diagnostic details that can be useful for tracking down
 issues. The ``getDetails`` method will return a dict of all the attached
-details. Each detail object is an instance of ``testtools.content.Content``.
+details, but can only be called before ``cleanUp`` is called. Each detail
+object is an instance of ``testtools.content.Content``.
 
   >>> with WithLog() as l:
   ...     print(l.getDetails()['message'].as_text())
   foo bar baz
 
+Errors in setUp
++++++++++++++++
+
+The examples above used ``_setUp`` rather than ``setUp`` because the base
+class implementation of ``setUp`` acts to reduce the chance of leaking
+external resources if an error is raised from ``_setUp``. Specifically,
+``setUp`` contains a try:/except: block which catches all exceptions, captures
+any registered detail objects, and calls ``self.cleanUp`` before propogating
+the error. As long as you take care to register any cleanups before calling
+the code that may fail, this will cause them to be cleaned up. The captured
+detail objects are provided to the args of the raised exception.
+
 Shared Dependencies
 +++++++++++++++++++
 
diff --git a/fixtures/__init__.py b/fixtures/__init__.py
index 577d2e1..0d15408 100644
--- a/fixtures/__init__.py
+++ b/fixtures/__init__.py
@@ -58,11 +58,13 @@ __all__ = [
     'MockPatchMultiple',
     'MockPatchObject',
     'MonkeyPatch',
+    'MultipleExceptions',
     'NestedTempfile',
     'PackagePathEntry',
     'PopenFixture',
     'PythonPackage',
     'PythonPathEntry',
+    'SetupError',
     'StringStream',
     'TempDir',
     'TempHomeDir',
@@ -79,6 +81,8 @@ from fixtures.fixture import (
     Fixture,
     FunctionFixture,
     MethodFixture,
+    MultipleExceptions,
+    SetupError,
     )
 from fixtures._fixtures import (
     ByteStream,
diff --git a/fixtures/_fixtures/environ.py b/fixtures/_fixtures/environ.py
index 5494429..e4275f9 100644
--- a/fixtures/_fixtures/environ.py
+++ b/fixtures/_fixtures/environ.py
@@ -40,8 +40,7 @@ class EnvironmentVariable(Fixture):
         self.varname = varname
         self.newvalue = newvalue
 
-    def setUp(self):
-        super(EnvironmentVariable, self).setUp()
+    def _setUp(self):
         varname = self.varname
         orig_value = os.environ.get(varname)
         if orig_value is not None:
diff --git a/fixtures/_fixtures/logger.py b/fixtures/_fixtures/logger.py
index d6f2d25..0bf2e97 100644
--- a/fixtures/_fixtures/logger.py
+++ b/fixtures/_fixtures/logger.py
@@ -46,8 +46,7 @@ class LogHandler(Fixture):
         self._level = level
         self._nuke_handlers = nuke_handlers
 
-    def setUp(self):
-        super(LogHandler, self).setUp()
+    def _setUp(self):
         logger = getLogger(self._name)
         if self._level:
             self.addCleanup(logger.setLevel, logger.level)
@@ -95,8 +94,7 @@ class FakeLogger(Fixture):
         self._nuke_handlers = nuke_handlers
         self._formatter = formatter
 
-    def setUp(self):
-        super(FakeLogger, self).setUp()
+    def _setUp(self):
         name = _u("pythonlogging:'%s'") % self._name
         output = self.useFixture(StringStream(name)).stream
         self._output = output
diff --git a/fixtures/_fixtures/mockpatch.py b/fixtures/_fixtures/mockpatch.py
index bdf1c03..276f65f 100644
--- a/fixtures/_fixtures/mockpatch.py
+++ b/fixtures/_fixtures/mockpatch.py
@@ -25,8 +25,7 @@ mock_default = extras.try_imports(
 
 
 class _Base(fixtures.Fixture):
-    def setUp(self):
-        super(_Base, self).setUp()
+    def _setUp(self):
         _p = self._get_p()
         self.addCleanup(_p.stop)
         self.mock = _p.start()
diff --git a/fixtures/_fixtures/monkeypatch.py b/fixtures/_fixtures/monkeypatch.py
index bfb7351..7db3671 100644
--- a/fixtures/_fixtures/monkeypatch.py
+++ b/fixtures/_fixtures/monkeypatch.py
@@ -42,8 +42,7 @@ class MonkeyPatch(Fixture):
         self.name = name
         self.new_value = new_value
     
-    def setUp(self):
-        Fixture.setUp(self)
+    def _setUp(self):
         location, attribute = self.name.rsplit('.', 1)
         # Import, swallowing all errors as any element of location may be
         # a class or some such thing.
diff --git a/fixtures/_fixtures/packagepath.py b/fixtures/_fixtures/packagepath.py
index 43a9bf3..86b7925 100644
--- a/fixtures/_fixtures/packagepath.py
+++ b/fixtures/_fixtures/packagepath.py
@@ -39,8 +39,7 @@ class PackagePathEntry(Fixture):
         self.packagename = packagename
         self.directory = directory
 
-    def setUp(self):
-        Fixture.setUp(self)
+    def _setUp(self):
         path = sys.modules[self.packagename].__path__
         if self.directory in path:
             return
diff --git a/fixtures/_fixtures/popen.py b/fixtures/_fixtures/popen.py
index 728e980..198b561 100644
--- a/fixtures/_fixtures/popen.py
+++ b/fixtures/_fixtures/popen.py
@@ -95,8 +95,7 @@ class FakePopen(Fixture):
         super(FakePopen, self).__init__()
         self.get_info = get_info
 
-    def setUp(self):
-        super(FakePopen, self).setUp()
+    def _setUp(self):
         self.addCleanup(setattr, subprocess, 'Popen', subprocess.Popen)
         subprocess.Popen = self
         self.procs = []
diff --git a/fixtures/_fixtures/pythonpackage.py b/fixtures/_fixtures/pythonpackage.py
index 4fbd278..97ccc16 100644
--- a/fixtures/_fixtures/pythonpackage.py
+++ b/fixtures/_fixtures/pythonpackage.py
@@ -47,8 +47,7 @@ class PythonPackage(Fixture):
         self.modulelist = modulelist
         self.init = init
 
-    def setUp(self):
-        Fixture.setUp(self)
+    def _setUp(self):
         self.base = self.useFixture(TempDir()).path
         base = self.base
         root = os.path.join(base, self.packagename)
diff --git a/fixtures/_fixtures/pythonpath.py b/fixtures/_fixtures/pythonpath.py
index 89c1968..ca11a43 100644
--- a/fixtures/_fixtures/pythonpath.py
+++ b/fixtures/_fixtures/pythonpath.py
@@ -35,8 +35,7 @@ class PythonPathEntry(Fixture):
         """
         self.directory = directory
 
-    def setUp(self):
-        Fixture.setUp(self)
+    def _setUp(self):
         if self.directory in sys.path:
             return
         self.addCleanup(sys.path.remove, self.directory)
diff --git a/fixtures/_fixtures/streams.py b/fixtures/_fixtures/streams.py
index 188d8e3..8ceb4fc 100644
--- a/fixtures/_fixtures/streams.py
+++ b/fixtures/_fixtures/streams.py
@@ -42,8 +42,7 @@ class Stream(Fixture):
         self._detail_name = detail_name
         self._stream_factory = stream_factory
 
-    def setUp(self):
-        super(Stream, self).setUp()
+    def _setUp(self):
         write_stream, read_stream = self._stream_factory()
         self.stream = write_stream
         self.addDetail(self._detail_name,
diff --git a/fixtures/_fixtures/tempdir.py b/fixtures/_fixtures/tempdir.py
index 663d3eb..78b4d3e 100644
--- a/fixtures/_fixtures/tempdir.py
+++ b/fixtures/_fixtures/tempdir.py
@@ -39,8 +39,7 @@ class TempDir(fixtures.Fixture):
         """
         self.rootdir = rootdir
 
-    def setUp(self):
-        super(TempDir, self).setUp()
+    def _setUp(self):
         self.path = tempfile.mkdtemp(dir=self.rootdir)
         self.addCleanup(shutil.rmtree, self.path, ignore_errors=True)
 
@@ -63,8 +62,7 @@ class NestedTempfile(fixtures.Fixture):
     down.
     """
 
-    def setUp(self):
-        super(NestedTempfile, self).setUp()
+    def _setUp(self):
         tempdir = self.useFixture(TempDir()).path
         patch = fixtures.MonkeyPatch("tempfile.tempdir", tempdir)
         self.useFixture(patch)
diff --git a/fixtures/_fixtures/temphomedir.py b/fixtures/_fixtures/temphomedir.py
index 2601a8d..2fcceb9 100644
--- a/fixtures/_fixtures/temphomedir.py
+++ b/fixtures/_fixtures/temphomedir.py
@@ -27,6 +27,6 @@ class TempHomeDir(TempDir):
     :ivar path: the path of the temporary directory.
     """
 
-    def setUp(self):
-        super(TempHomeDir, self).setUp()
+    def _setUp(self):
+        super(TempHomeDir, self)._setUp()
         self.useFixture(fixtures.EnvironmentVariable("HOME", self.path))
diff --git a/fixtures/_fixtures/timeout.py b/fixtures/_fixtures/timeout.py
index 7863b0d..9c6fb99 100644
--- a/fixtures/_fixtures/timeout.py
+++ b/fixtures/_fixtures/timeout.py
@@ -51,8 +51,7 @@ class Timeout(fixtures.Fixture):
     def signal_handler(self, signum, frame):
         raise TimeoutException()
 
-    def setUp(self):
-        super(Timeout, self).setUp()
+    def _setUp(self):
         if self.alarm_fn is None:
             return  # Can't run on Windows
         if self.gentle:
diff --git a/fixtures/_fixtures/warnings.py b/fixtures/_fixtures/warnings.py
index b42a818..8184403 100644
--- a/fixtures/_fixtures/warnings.py
+++ b/fixtures/_fixtures/warnings.py
@@ -34,8 +34,7 @@ class WarningsCapture(fixtures.Fixture):
     def _showwarning(self, *args, **kwargs):
         self.captures.append(warnings.WarningMessage(*args, **kwargs))
 
-    def setUp(self):
-        super(WarningsCapture, self).setUp()
+    def _setUp(self):
         patch = fixtures.MonkeyPatch("warnings.showwarning", self._showwarning)
         self.useFixture(patch)
         self.captures = []
diff --git a/fixtures/fixture.py b/fixtures/fixture.py
index 2cf966d..be5dd8a 100644
--- a/fixtures/fixture.py
+++ b/fixtures/fixture.py
@@ -18,6 +18,7 @@ __all__ = [
     'FunctionFixture',
     'MethodFixture',
     'MultipleExceptions',
+    'SetupError',
     ]
 
 import itertools
@@ -49,6 +50,13 @@ def combine_details(source_details, target_details):
         target_details[name] = content_object
 
 
+class SetupError(Exception):
+    """Setup failed.
+
+    args[0] will be a details dict.
+    """
+
+
 class Fixture(object):
     """A Fixture representing some state or resource.
 
@@ -97,6 +105,10 @@ class Fixture(object):
 
         This should not typically be overridden, see addCleanup instead.
 
+        cleanUp may be called once and only once after setUp() has been called.
+        The base implementation of setUp will automatically call cleanUp if
+        an exception occurs within setUp itself.
+
         :param raise_first: Deprecated parameter from before testtools gained
             MultipleExceptions. raise_first defaults to True. When True
             if a single exception is raised, it is reraised after all the
@@ -111,7 +123,7 @@ class Fixture(object):
         try:
             return self._cleanups(raise_errors=raise_first)
         finally:
-            self._clear_cleanups()
+            self._remove_state()
 
     def _clear_cleanups(self):
         """Clean the cleanup queue without running them.
@@ -126,6 +138,15 @@ class Fixture(object):
         self._details = {}
         self._detail_sources = []
 
+    def _remove_state(self):
+        """Remove the internal state.
+
+        Called from cleanUp to put the fixture back into a not-ready state.
+        """
+        self._cleanups = None
+        self._details = None
+        self._detail_sources = None
+
     def __enter__(self):
         self.setUp()
         return self
@@ -134,7 +155,7 @@ class Fixture(object):
         try:
             self._cleanups()
         finally:
-            self._clear_cleanups()
+            self._remove_state()
         return False # propogate exceptions from the with body.
 
     def getDetails(self):
@@ -153,16 +174,49 @@ class Fixture(object):
     def setUp(self):
         """Prepare the Fixture for use.
 
-        This should be overridden by most concrete fixtures. When overriding
-        be sure to include self.addCleanup calls to restore the fixture to
-        an un-setUp state, so that a single Fixture instance can be reused.
+        This should not be overridden.
+        
+        Concrete fixtures should implement _setUp.
 
-        After setUp is called, the fixture will have one or more attributes
+        After setUp has completed, the fixture will have one or more attributes
         which can be used (these depend totally on the concrete subclass).
 
+        :raises: MultipleExceptions if _setUp fails. The last exception
+            captured within the MultipleExceptions will be a SetupError
+            exception.
         :return: None.
+
+        :changed in 1.3: The recommendation to override setUp has been
+            reversed - before 1.3, setUp() should be overridden, now it should
+            not be.
         """
         self._clear_cleanups()
+        try:
+            self._setUp()
+        except Exception:
+            err = sys.exc_info()
+            details = {}
+            if gather_details is not None:
+                # Materialise all details since we're about to cleanup.
+                gather_details(self.getDetails(), details)
+            else:
+                details = self.getDetails()
+            errors = [err] + self.cleanUp(raise_first=False)
+            try:
+                raise SetupError(details)
+            except SetupError as e:
+                errors.append(sys.exc_info())
+                raise MultipleExceptions(*errors)
+
+    def _setUp(self):
+        """Template method for subclasses to override.
+
+        Override this to customise the fixture. When overriding
+        be sure to include self.addCleanup calls to restore the fixture to
+        an un-setUp state, so that a single Fixture instance can be reused.
+
+        :return: None.
+        """
 
     def reset(self):
         """Reset a setUp Fixture to the 'just setUp' state again.
@@ -183,15 +237,23 @@ class Fixture(object):
         """Use another fixture.
 
         The fixture will be setUp, and self.addCleanup(fixture.cleanUp) called.
+        If the fixture fails to set up, useFixture will attempt to gather its
+        details into this fixture's details to aid in debugging.
 
         :param fixture: The fixture to use.
         :return: The fixture, after setting it up and scheduling a cleanup for
            it.
+        :raises: Any errors raised by the fixture's setUp method.
         """
         try:
             fixture.setUp()
+        except MultipleExceptions as e:
+            if e.args[-1][0] is SetupError:
+                combine_details(e.args[-1][1].args[0], self._details)
+            raise
         except:
-            # The child failed to come up, capture any details it has (copying
+            # The child failed to come up and didn't raise MultipleExceptions
+            # which we can understand... capture any details it has (copying
             # the content, it may go away anytime).
             if gather_details is not None:
                 gather_details(fixture.getDetails(), self._details)
@@ -249,8 +311,7 @@ class FunctionFixture(Fixture):
         self.cleanup_fn = cleanup_fn
         self.reset_fn = reset_fn
 
-    def setUp(self):
-        super(FunctionFixture, self).setUp()
+    def _setUp(self):
         fn_result = self.setup_fn()
         self._maybe_cleanup(fn_result)
 
@@ -323,8 +384,7 @@ class MethodFixture(Fixture):
             reset = getattr(obj, 'reset', None)
         self._reset = reset
 
-    def setUp(self):
-        super(MethodFixture, self).setUp()
+    def _setUp(self):
         self._setup()
 
     def cleanUp(self):
diff --git a/fixtures/tests/test_fixture.py b/fixtures/tests/test_fixture.py
index 74e6ad0..737ae41 100644
--- a/fixtures/tests/test_fixture.py
+++ b/fixtures/tests/test_fixture.py
@@ -122,7 +122,7 @@ class TestFixture(testtools.TestCase):
     @require_gather_details
     def test_useFixture_details_captured_from_setUp(self):
         # Details added during fixture set-up are gathered even if setUp()
-        # fails with an exception.
+        # fails with an unknown exception.
         class SomethingBroke(Exception): pass
         class BrokenFixture(fixtures.Fixture):
             def setUp(self):
@@ -143,6 +143,24 @@ class TestFixture(testtools.TestCase):
             {"content": text_content("foobar")},
             simple_fixture.getDetails())
 
+    @require_gather_details
+    def test_useFixture_details_captured_from_setUp_MultipleExceptions(self):
+        # Details added during fixture set-up are gathered even if setUp()
+        # fails with (cleanly - with MultipleExceptions / SetupError).
+        class SomethingBroke(Exception): pass
+        class BrokenFixture(fixtures.Fixture):
+            def _setUp(self):
+                self.addDetail('content', text_content("foobar"))
+                raise SomethingBroke()
+        class SimpleFixture(fixtures.Fixture):
+            def _setUp(self):
+                self.useFixture(BrokenFixture())
+        simple = SimpleFixture()
+        e = self.assertRaises(fixtures.MultipleExceptions, simple.setUp)
+        self.assertEqual(
+            {"content": text_content("foobar")},
+            e.args[-1][1].args[0])
+
     def test_getDetails(self):
         fixture = fixtures.Fixture()
         with fixture:
@@ -162,7 +180,7 @@ class TestFixture(testtools.TestCase):
             self.assertEqual({}, parent.getDetails())
             # After cleanup the child details are still gone.
             child.addDetail('foo', 'content')
-        self.assertEqual({}, parent.getDetails())
+        self.assertRaises(TypeError, parent.getDetails)
 
     def test_duplicate_details_are_disambiguated(self):
         parent = fixtures.Fixture()
@@ -185,7 +203,59 @@ class TestFixture(testtools.TestCase):
             self.assertEqual({}, fixture.getDetails())
             fixture.addDetail('foo', 'content')
         # Cleanup clears the details too.
-        self.assertEqual({}, fixture.getDetails())
+        self.assertRaises(TypeError, fixture.getDetails)
+
+    def test_setUp_subclassed(self):
+        # Even though its no longer recommended, we need to be sure that
+        # overriding setUp and calling super().setUp still works.
+        class Subclass(fixtures.Fixture):
+            def setUp(self):
+                super(Subclass, self).setUp()
+                self.fred = 1
+                self.addCleanup(setattr, self, 'fred', 2)
+        with Subclass() as f:
+            self.assertEqual(1, f.fred)
+        self.assertEqual(2, f.fred)
+
+    def test__setUp(self):
+        # _setUp is called, and cleanups can be registered by it.
+        class Subclass(fixtures.Fixture):
+            def _setUp(self):
+                self.fred = 1
+                self.addCleanup(setattr, self, 'fred', 2)
+        with Subclass() as f:
+            self.assertEqual(1, f.fred)
+        self.assertEqual(2, f.fred)
+
+    def test__setUp_fails(self):
+        # when _setUp fails, the fixture is left ready-to-setUp, and any
+        # details added during _setUp are captured.
+        class Subclass(fixtures.Fixture):
+            def _setUp(self):
+                self.addDetail('log', text_content('stuff'))
+                1/0
+        f = Subclass()
+        e = self.assertRaises(fixtures.MultipleExceptions, f.setUp)
+        self.assertRaises(TypeError, f.cleanUp)
+        self.assertIsInstance(e.args[0][1], ZeroDivisionError)
+        self.assertIsInstance(e.args[1][1], fixtures.SetupError)
+        self.assertEqual('stuff', e.args[1][1].args[0]['log'].as_text())
+
+    def test__setUp_fails_cleanUp_fails(self):
+        # when _setUp fails, cleanups are called, and their failure is captured
+        # into the MultipleExceptions instance.
+        class Subclass(fixtures.Fixture):
+            def _setUp(self):
+                self.addDetail('log', text_content('stuff'))
+                self.addCleanup(lambda: 1/0)
+                raise Exception('fred')
+        f = Subclass()
+        e = self.assertRaises(fixtures.MultipleExceptions, f.setUp)
+        self.assertRaises(TypeError, f.cleanUp)
+        self.assertEqual(Exception, e.args[0][0])
+        self.assertEqual(ZeroDivisionError, e.args[1][0])
+        self.assertEqual(fixtures.SetupError, e.args[2][0])
+        self.assertEqual('stuff', e.args[2][1].args[0]['log'].as_text())
 
 
 class TestFunctionFixture(testtools.TestCase):