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

Proposed by Cedric Brandily
Status: Needs review
Proposed branch: lp:~cbrandily/python-fixtures/cleanup-context
Merge into: lp:python-fixtures
Diff against target: 80 lines (+54/-5)
2 files modified
fixtures/fixture.py (+6/-5)
fixtures/tests/test_fixture.py (+48/-0)
To merge this branch: bzr merge lp:~cbrandily/python-fixtures/cleanup-context
Reviewer Review Type Date Requested Status
Robert Collins Disapprove
Review via email: mp+259725@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

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

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'fixtures/fixture.py'
2--- fixtures/fixture.py 2014-09-25 03:01:50 +0000
3+++ fixtures/fixture.py 2015-05-21 06:22:53 +0000
4@@ -127,14 +127,15 @@
5 self._detail_sources = []
6
7 def __enter__(self):
8- self.setUp()
9+ try:
10+ self.setUp()
11+ except:
12+ self.cleanUp()
13+ raise
14 return self
15
16 def __exit__(self, exc_type, exc_val, exc_tb):
17- try:
18- self._cleanups()
19- finally:
20- self._clear_cleanups()
21+ self.cleanUp()
22 return False # propogate exceptions from the with body.
23
24 def getDetails(self):
25
26=== modified file 'fixtures/tests/test_fixture.py'
27--- fixtures/tests/test_fixture.py 2014-09-25 03:01:50 +0000
28+++ fixtures/tests/test_fixture.py 2015-05-21 06:22:53 +0000
29@@ -311,3 +311,51 @@
30 self.assertEqual(126, obj.value)
31 fixture.cleanUp()
32 self.assertEqual(84, obj.value)
33+
34+ def test_context_cleanUp(self):
35+ class SimpleFixture(fixtures.Fixture):
36+ def __init__(self):
37+ self.cleanup_called = False
38+ def cleanUp(self):
39+ self.cleanup_called = True
40+
41+ fixture = SimpleFixture()
42+ self.assertFalse(fixture.cleanup_called)
43+ with fixture:
44+ pass
45+ self.assertTrue(fixture.cleanup_called)
46+
47+ def test_context_cleanUp_with_failed_suite(self):
48+ class SimpleFixture(fixtures.Fixture):
49+ def __init__(self):
50+ self.cleanup_called = False
51+ def cleanUp(self):
52+ self.cleanup_called = True
53+
54+ fixture = SimpleFixture()
55+ def func():
56+ with fixture:
57+ raise ValueError()
58+
59+ self.assertFalse(fixture.cleanup_called)
60+ self.assertRaises(ValueError, func)
61+ self.assertTrue(fixture.cleanup_called)
62+
63+ def test_context_cleanUp_with_failed_setUp(self):
64+ class FailedSetUpFixture(fixtures.Fixture):
65+ def __init__(self):
66+ self.cleanup_called = False
67+ def setUp(self):
68+ super(FailedSetUpFixture, self).setUp()
69+ raise ValueError()
70+ def cleanUp(self):
71+ self.cleanup_called = True
72+
73+ fixture = FailedSetUpFixture()
74+ def func():
75+ with fixture:
76+ pass
77+
78+ self.assertFalse(fixture.cleanup_called)
79+ self.assertRaises(ValueError, func)
80+ self.assertTrue(fixture.cleanup_called)

Subscribers

People subscribed via source and target branches

to all changes: