Merge lp:~cbrandily/python-fixtures/cleanup-context into lp:python-fixtures

[Cedric Brandily]

Ensure cleanups are called even if setUp fails with fixture context

This change ensures that a fixture cleanups will be called even if
its setUp fails when used as a context.

Example:

  with MyFixture() as fixture1:
    pass

fixture1.cleanUp will be called even if fixture1.setUp (called in
fixture1.__enter__) fails.

[Robert Collins (lifeless)]

Thanks for this patch. This is a better, closer to the cause fix. It doesn't address the issue of setUp being fragile though.

Additionally - and I'm sorry about this - you got mislead by the somewhat confusing thing of us having a mirror of the code here: the code is actually on github at https://testing-cabal/fixtures. (LP does have a link to that but its not obvious - I hope I've made it clearer by editing the homepage a bit).

I've put up a patch https://github.com/testing-cabal/fixtures/pull/11 which I think fixes both angles systematically. It will require fixture authors to update, but they can do that incrementally in their own time, and the update is very easy. Reasoning behind this is at https://rbtcollins.wordpress.com/2015/06/22/revisiting-the-fixture-api-handling-leaky-resources/

review: Disapprove

Unmerged revisions

96. By Cedric Brandily on 2015-05-21

Ensure cleanups are called even if setUp fails with fixture context

This change ensures that a fixture cleanups will be called even if
its setUp fails when used as a context.

Example:

  with MyFixture() as fixture1:
    pass

fixture1.cleanUp will be called even if fixture1.setUp (called in
fixture1.__enter__) fails.

=== modified file 'fixtures/fixture.py'
--- fixtures/fixture.py	2014-09-25 03:01:50 +0000
+++ fixtures/fixture.py	2015-05-21 06:22:53 +0000
@@ -127,14 +127,15 @@
         self._detail_sources = []
 
     def __enter__(self):
-        self.setUp()
+        try:
+            self.setUp()
+        except:
+            self.cleanUp()
+            raise
         return self
 
     def __exit__(self, exc_type, exc_val, exc_tb):
-        try:
-            self._cleanups()
-        finally:
-            self._clear_cleanups()
+        self.cleanUp()
         return False # propogate exceptions from the with body.
 
     def getDetails(self):
 
=== modified file 'fixtures/tests/test_fixture.py'
--- fixtures/tests/test_fixture.py	2014-09-25 03:01:50 +0000
+++ fixtures/tests/test_fixture.py	2015-05-21 06:22:53 +0000
@@ -311,3 +311,51 @@
         self.assertEqual(126, obj.value)
         fixture.cleanUp()
         self.assertEqual(84, obj.value)
+
+    def test_context_cleanUp(self):
+        class SimpleFixture(fixtures.Fixture):
+            def __init__(self):
+                self.cleanup_called = False
+            def cleanUp(self):
+                self.cleanup_called = True
+
+        fixture = SimpleFixture()
+        self.assertFalse(fixture.cleanup_called)
+        with fixture:
+            pass
+        self.assertTrue(fixture.cleanup_called)
+
+    def test_context_cleanUp_with_failed_suite(self):
+        class SimpleFixture(fixtures.Fixture):
+            def __init__(self):
+                self.cleanup_called = False
+            def cleanUp(self):
+                self.cleanup_called = True
+
+        fixture = SimpleFixture()
+        def func():
+            with fixture:
+                raise ValueError()
+
+        self.assertFalse(fixture.cleanup_called)
+        self.assertRaises(ValueError, func)
+        self.assertTrue(fixture.cleanup_called)
+
+    def test_context_cleanUp_with_failed_setUp(self):
+        class FailedSetUpFixture(fixtures.Fixture):
+            def __init__(self):
+                self.cleanup_called = False
+            def setUp(self):
+                super(FailedSetUpFixture, self).setUp()
+                raise ValueError()
+            def cleanUp(self):
+                self.cleanup_called = True
+
+        fixture = FailedSetUpFixture()
+        def func():
+            with fixture:
+                pass
+
+        self.assertFalse(fixture.cleanup_called)
+        self.assertRaises(ValueError, func)
+        self.assertTrue(fixture.cleanup_called)