New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cleanUp stack for unittest #49929
Comments
Proposal to add a cleanUp stack to unittest.TestCase. This is a list of Similar functionality is in the testing frameworks used by bzr, Twisted, I will create a patch similar to the bzr implementation. The public API is via a new method: def addCleanUpItem(self, callable, *args, **kwargs): Usage is: self.db = self.createDB()
self.addCleanUpItem(self.db.close) |
I'm used to doing this using a finally clause in the setUp() method (if I guess this would be a convenience to avoid the need for this pattern? def setUp(self):
try:
do_a_bunch_of_stuff_that_could_fail()
finally:
conditionally_undo_setup()
do_more()
def conditionally_undo_setup(self):
if self.foo:
self.foo.close()
if self.bar:
shutil.rmtree(self.bar)
def tearDown(self):
conditionally_undo_setup()
undo_more() fwiw, in your self.db.close example, normally i'd also want to remove |
This is a nice (simple to use and understand) pattern for resource Supporting the cleaning up of resources when setUp fails (without It provides a clean way to clean up just the resources you have bzr: http://bazaar.launchpad.net/~jml/testtools/trunk/annotate/head%3A/testtools/testcase.py And from zope.testing: The documentation: The module: |
From your example this would completely remove the need for the |
And actually your conditionally_undo_setup() call in setUp is incorrect. |
Patch attached. No docs, if it is agreed I can apply I'll write docs. After a long discussion we arrived at some semblance of consensus on the Only one -1 (thought that cleanUp should be a method) and Holger Krekel Many others were +1 - Fred Drake, Titus Brown, Laura Creighton, Robert It provides a clean and simple pattern for the cleaning up of resources
|
I'm not sure why it is called after tearDown. It would be better to call |
I'm agnostic on before / after tearDown, so happy to make that change. |
It should run after tearDown so that teardown can do actions that may |
Actually let me phrase that differently. and tearDown is because of the LIFO need. If you imagine that clean ups are being done in the base class teardown, |
teardown Why should they? It's only an implementation choice, and not a wise one I explained my proposal in terms of actual use cases, but I don't see |
Sorry, roundup screwed the quoting. I was responding to the following |
On Sat, 2009-04-04 at 22:09 +0000, Antoine Pitrou wrote:
I was arguing by analogy: if we were to implement addCleanup as If you want a worked example: class TestSample(TestCase):
def setUp(self):
dirname = mkdtemp()
self.addCleanup(shutils.rmtree, dirname, ignore_errors=True)
db = make_db(dirname)
self.addCleanup(db.tearDown)
--- This depends on db being torn down before the rmtree, or else the db -Rob |
Antoine, Robert suggests calling it after tearDown so that tearDown can "so that test cases can also use it to initialize Can you give an example? You seem to imply that clean up be used to |
Sure, but that's an example of doing something which is already doable |
The main use case for addCleanup is resource allocation in tests. Why |
If your cleanup relies on something which has been set up during setUp |
On Sat, 2009-04-04 at 23:06 +0000, Antoine Pitrou wrote:
I don't think you can sensibly define an ordering between setup/teardown Either cleanups happen after teardown, and then you can use them when I assert that its better to be able to use cleanups in all three test Our experience in bzr (we use this heavily, and migrated to it -Rob sequence 1: cleanup after teardowns prevents using cleanups on things setup sequence 2: cleanup before teardown prevents using cleanups in base base.setup |
I think some perspective is required on this enhancement request. I From my perspective, there are two issues:
Another thought: Why not have an option for defining a method called I personally think that doing something like this would be trivial (yet |
On Sun, 2009-04-05 at 07:25 +0000, Garrett Cooper wrote:
Indeed, and in bzr and other unittest extension libraries one of the
the proposed patch in 5679 runs cleanups always, so this shouldn't be an
I'm conceptually fine with this, certainly I've not written a tearDown
Its much more complex than addCleanup [it will be tied to the MRO], not -Rob |
I'm baffled. If you say you don't care about the order, why are you [...]
The point is that sequence 2 can already be emulated using careful |
On Sun, 2009-04-05 at 10:15 +0000, Antoine Pitrou wrote:
I didn't say I don't care; I do - I care that it is robust and hard to
I don't understand; neither sequence works - they are showing how any -Rob |
????
And I'm telling you one failure mode is more desireable than the other. |
I'm in favour of running clean ups afterwards on the basis that it makes
But this is a function of whichever way we do it - and so not an
Which is an argument in favour of running clean ups afterwards. |
A use case for running clean ups before tearDown (in which case You have existing code which (for example) creates an SSH connection in If clean ups are run after tearDown then you can't use addCleanup inside Switching to use clean ups would require rewriting the existing code. The downsides are that it makes adding clean ups in tearDown impossible We probably need a third party arbiter to pick which is the most sane. :-) |
So, we are talking about adding a feature that could cause problem whether Do we really want to add this feature? The added functionality doesn't |
Well, we could do both. Call cleanups before tearDown (all the while If the cleanup machinery is written carefully enough, you may even be |
This is actually a minor point about the order things happen in. I don't Particularly if we decide to call clean ups before tearDown then I will I did think about having two separate clean up stacks - one for those The point about this patch is that it makes the deallocation of |
I see the validity in the concern raised by Virgil: I think this can be simplified from a user perspective by saying `I'm Or am I missing the boat here on what's trying to be accomplished? We really shouldn't be mixing and matching both implementations because |
On Sun, 2009-04-05 at 21:31 +0000, Antoine Pitrou wrote:
I think this makes cleanups harder to explain, but yes we could do it.
I've been meaning to tweak the patch - precisely to allow this. The point I've been trying to make isn't just that after everything is @antoine I appreciate that you feel very strongly about this; I'm at a @michael - you may have a convenience function that uses addCleanup - if -Rob |
@garrett / Virgil I don't think anything other than very short term confusion is possible. As an interesting data point, the Bzr code does clean ups *before* tearDown. |
On Sun, 2009-04-05 at 23:49 +0000, Michael Foord wrote:
No it doesn't: We subclass unittest.TestCase. We also override run() to make tearDown Our base test case class has it's tearDown: def tearDown(self):
self._bzr_test_tearDown_run = True
self._runCleanups()
self._log_contents = ''
unittest.TestCase.tearDown(self) (which is to say, _runCleanups runs after any child classes tearDown, -Rob |
My apologies - the jml code on launchpad runs clean ups before taerDown. http://bazaar.launchpad.net/~jml/testtools/trunk/annotate/head%3A/testtools/testcase.py |
On Sun, 2009-04-05 at 23:57 +0000, Michael Foord wrote:
Ah, indeed it does. Well, FWIW my use has nearly all been with bzr's Jono - was there some particular reason testtools does it after? (We're -Rob |
On Mon, Apr 6, 2009 at 10:06 AM, Robert Collins
No particular reason. |
I like this patch. However, a nice-to-have would be that _doCleanups() |
On Mon, Apr 6, 2009 at 2:16 PM, Kumar McMillan <report@bugs.python.org> wrote:
I think reporting the errors via addError is the right thing. If your |
FWIW, I kind of like the tests here: |
Damn - proper patch without extraneous stuff this time. |
Proper patch and proper issue this time! Not my evening. |
On Sat, 2009-04-25 at 23:17 +0000, Michael Foord wrote:
Did this want a new issue perhaps? [it looks fine, just unrelated]. -Rob |
Updated patch with docs. My intention is to apply this in the next I've settled on calling doCleanups *after* tearDown. The issues and One point of view concerns using addCleanups with existing tests. If the setUp allocates resources that are deallocated by tearDown, the This pattern doesn't allow tearDown to create cleanup functions and If you look at the situation with new tests then it is perfectly natural The solution I've opted for is to make doCleanups public. If you are This takes into account the experience of those already using test |
Committed in revision 72219. |
Cool! Thanks for all of the hard work Michael :D. |
Apparently during a merge from trunk (r72477) the addCleanup and other methods ended up in 3.1rc1, even if they are documented as new in 3.2. I'll update the doc to say "new in 3.1". |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: