Proposal to add a cleanUp stack to unittest.TestCase. This is a list of
callables to be called (LIFO) to cleanup resources. If there are items
on the stack it should be called even if setUp fails. Otherwise it
should be called after tearDown.

Similar functionality is in the testing frameworks used by bzr, Twisted,
and Zope.

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 do it at all, I'm not used to setUp failing).

I guess this would be a convenience to avoid the need for this pattern?
I'm not sure we really need a list of cleanup callbacks. Got pointers
to good examples of this in bzr/twisted/zope?

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
and unlink the database as well as just closing it for a unittest.
maybe not the best example.

This is a nice (simple to use and understand) pattern for resource
allocation / deallocation.

Supporting the cleaning up of resources when setUp fails (without
duplicating clean up code) is just one use case. (I agree setUp failure
is unusual.)

It provides a clean way to clean up just the resources you have
allocated in the event of test failure, without having to keep track
yourself of how far the test has got. Manually tracking resource usage
is easy to get wrong.

bzr:

http://bazaar.launchpad.net/~jml/testtools/trunk/annotate/head%3A/testtools/testcase.py

And from zope.testing:

The documentation:
http://svn.zope.org/zope.testing/trunk/src/zope/testing/setupstack.txt?rev=92340&view=auto

The module:
http://svn.zope.org/zope.testing/trunk/src/zope/testing/setupstack.py?rev=92340&view=auto

From your example this would completely remove the need for the
conditional checking in the cleanup code. Only resources actually
allocated would be in the stack.

And actually your conditionally_undo_setup() call in setUp is incorrect.
It should in an except block that re-raises rather than a finally block
(which will do the cleanup even in non-exceptional circumstances). Easy
code to get wrong...

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
Testing In Python mailing list that this was a good thing (tm).

Only one -1 (thought that cleanUp should be a method) and Holger Krekel
was only +0 because he thinks changes in unittest should be conservative
(but he isn't actually using it).

Many others were +1 - Fred Drake, Titus Brown, Laura Creighton, Robert
Collins, Jonathan Lange, Kumar McMillan and others.

It provides a clean and simple pattern for the cleaning up of resources

• avoiding the need for nested try finallys and the manual tracking of
  resources. It can also avoid the need for duplication of cleanup code.

I'm not sure why it is called after tearDown. It would be better to call
it before tearDown, so that test cases can also use it to initialize
additional resources and finalize them in a LIFO way wrt the main tearDown.

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
require cleanup; because the cleanups run in LIFO you can acquire
resources in setUp and have cleanups clean them up,

Actually let me phrase that differently.
standard practice for setUp is
super.setUp()
my_setup_code()

and tearDown is
my_teardown_code()
super.tearDown()

because of the LIFO need.

If you imagine that clean ups are being done in the base class teardown,
at the end of that method, then it becomes a lot more obvious why it
should run after tearDown - because thats the right place, and because
many users may be failing to call the base class tearDown which has
historically done nothing.

teardown

Why should they? It's only an implementation choice, and not a wise one
I would say (precisely because people are used to the fact that the
standard tearDown() method does nothing, and doesn't need to be called).

I explained my proposal in terms of actual use cases, but I don't see
any actual use case of addCleanup() in your argument.

Sorry, roundup screwed the quoting. I was responding to the following
sentence:
“If you imagine that clean ups are being done in the base class teardown”.

On Sat, 2009-04-04 at 22:09 +0000, Antoine Pitrou wrote:

Antoine Pitrou <pitrou@free.fr> added the comment:

teardown

Why should they? It's only an implementation choice, and not a wise one
I would say (precisely because people are used to the fact that the
standard tearDown() method does nothing, and doesn't need to be called).

I explained my proposal in terms of actual use cases, but I don't see
any actual use case of addCleanup() in your argument.

I was arguing by analogy: if we were to implement addCleanup as
something called at the end of the base class tearDown, then it would
clearly support LIFO tearing down of anything people have done [except
that we can't rely on them having called the base tearDown]. The next
best thing then is to call it from run() after calling tearDown.

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
teardown will blow up (and it must be torn down to release locks
correctly on windows).

-Rob

Antoine, Robert suggests calling it after tearDown so that tearDown can
also perform actions that need clean up. Not a common use case but at
least a use case. What do you mean by:

"so that test cases can also use it to initialize
additional resources and finalize them in a LIFO way wrt the main tearDown"

Can you give an example? You seem to imply that clean up be used to
initialize resources which seems very odd.

def setUp(self):
dirname = mkdtemp()
self.addCleanup(shutils.rmtree, dirname, ignore_errors=True)
db = make_db(dirname)
self.addCleanup(db.tearDown)

Sure, but that's an example of doing something which is already doable
without addCleanup (resource allocation in setUp).
I was talking about doing resource allocation in the test methods
themselves, which is /only/ possible using addCleanup, and needs
cleanups to be run before tearDown, not after.

The main use case for addCleanup is resource allocation in tests. Why
does this require clean ups to be executed before tearDown?

The main use case for addCleanup is resource allocation in tests. Why
does this require clean ups to be executed before tearDown?

If your cleanup relies on something which has been set up during setUp
and will be dropped during tearDown (a database connection, a temp dir,
an ssh session, whatever), then the cleanup must be run before the
teardown.

On Sat, 2009-04-04 at 23:06 +0000, Antoine Pitrou wrote:

Antoine Pitrou <pitrou@free.fr> added the comment:

> The main use case for addCleanup is resource allocation in tests. Why
> does this require clean ups to be executed before tearDown?

If your cleanup relies on something which has been set up during setUp
and will be dropped during tearDown (a database connection, a temp dir,
an ssh session, whatever), then the cleanup must be run before the
teardown.

I don't think you can sensibly define an ordering between setup/teardown
and cleanups that works for all cases.

Either cleanups happen after teardown, and then you can use them when
allocating things in setup/run/teardown, or they happen before teardown
and you cannot safely use them except in run because things setup by a
child will not have been torn down when cleanups run. [see the two
where-it-breaks sequences below]. The issue is that cleanups are not
connected to the inheritance heirarchy. They are safe and trivial to use
with resources that are not connected to each other *however* they are
defined, but the interaction with things being freed in tearDown cannot
meet all cases unless you have per-class-in the-MRO-queues (which is
overly complex).

I assert that its better to be able to use cleanups in all three test
methods rather than only in run, all other things being equal.

Our experience in bzr (we use this heavily, and migrated to it
incrementally across our 17K fixture suite) is that we rarely need to
use cleanups on dependent resources, and when we need to it has been
very easy to migrate the dependent resource to use cleanups as well.

-Rob

sequence 1: cleanup after teardowns prevents using cleanups on things
freed in teardown:

setup
self.foo = thing()
run
self.bar = something(self.foo)
self.addCleanUp(free, self.bar)
teardown
unthing(self.foo)
cleanups
free(self.bar) *boom* self.foo is already gone

sequence 2: cleanup before teardown prevents using cleanups in base
class setup methods

base.setup
self.foo = thing()
self.addCleanup(unthing, self.foo)
setup
super.setup()
self.bar = something(self.foo)
run
assertWhatever
cleanups
unthing(self.foo)
teardown
free(self.bar) *boom* self.foo is already gone

I think some perspective is required on this enhancement request. I
originally filed this issue -- http://bugs.python.org/issue5538 --
because of the unneeded complexity involved with duplicating
teardown-related code in setUp because of a step in setUp failing.

From my perspective, there are two issues:

• setUp failing doesn't cleanup on failure unless the test writer
  explicitly adds cleanup logic.
• cleanup shouldn't partially replace tearDown -- either supplement it
  or completely replace it longterm. Otherwise the unittest code and
  expectations associated with it will potentially confuse end users.

Another thought: Why not have an option for defining a method called
incrementalTearDown', which replaces tearDown' from a functional
standpoint? A method like that would clearly convey that this is
designed to replace tearDown, it's not the same functionally, and would
ease migration over the long-term if people chose to use this design
when writing testcases.

I personally think that doing something like this would be trivial (yet
novel) functionality as it makes more sense than the current
implementation of setUp->test->tearDown.

On Sun, 2009-04-05 at 07:25 +0000, Garrett Cooper wrote:

Garrett Cooper <yanegomi@gmail.com> added the comment:

I think some perspective is required on this enhancement request. I
originally filed this issue -- http://bugs.python.org/issue5538 --
because of the unneeded complexity involved with duplicating
teardown-related code in setUp because of a step in setUp failing.

Indeed, and in bzr and other unittest extension libraries one of the
first things done is to make tearDown be called always.

>From my perspective, there are two issues:

• setUp failing doesn't cleanup on failure unless the test writer
  explicitly adds cleanup logic.

the proposed patch in 5679 runs cleanups always, so this shouldn't be an
issue with the addCleanup functionality.

• cleanup shouldn't partially replace tearDown -- either supplement it
  or completely replace it longterm. Otherwise the unittest code and
  expectations associated with it will potentially confuse end users.

I'm conceptually fine with this, certainly I've not written a tearDown
for tests in bzr in a very long time.

Another thought: Why not have an option for defining a method called
incrementalTearDown', which replaces tearDown' from a functional
standpoint? A method like that would clearly convey that this is
designed to replace tearDown, it's not the same functionally, and would
ease migration over the long-term if people chose to use this design
when writing testcases.

Its much more complex than addCleanup [it will be tied to the MRO], not
as flexible (and in big suites that starts to matter a lot), and unable
to trivially hook into actually used resources [which addCleanup does].

-Rob

Our experience in bzr (we use this heavily, and migrated to it
incrementally across our 17K fixture suite) is that we rarely need to
use cleanups on dependent resources, and when we need to it has been
very easy to migrate the dependent resource to use cleanups as well.

I'm baffled. If you say you don't care about the order, why are you
arguing at all?

[...]

sequence 2: cleanup before teardown prevents using cleanups in base
class setup methods

The point is that sequence 2 can already be emulated using careful
"try...finally" in tearDown, while sequence 1 cannot. That is, sequence
1 *needs* the addCleanup, while for sequence 2 it is a mere additional
convenience.

On Sun, 2009-04-05 at 10:15 +0000, Antoine Pitrou wrote:

Antoine Pitrou <pitrou@free.fr> added the comment:

> Our experience in bzr (we use this heavily, and migrated to it
> incrementally across our 17K fixture suite) is that we rarely need to
> use cleanups on dependent resources, and when we need to it has been
> very easy to migrate the dependent resource to use cleanups as well.

I'm baffled. If you say you don't care about the order, why are you
arguing at all?

I didn't say I don't care; I do - I care that it is robust and hard to
misuse. Having addCleanup() when called from a tearDown lead to cleanups
not being called would be an easy route to misuse.

[...]
> sequence 2: cleanup before teardown prevents using cleanups in base
> class setup methods

The point is that sequence 2 can already be emulated using careful
"try...finally" in tearDown, while sequence 1 cannot. That is, sequence
1 *needs* the addCleanup, while for sequence 2 it is a mere additional
convenience.

I don't understand; neither sequence works - they are showing how any
choice [that retains the current simple proposed mechanism] cannot
interact without some failure modes with tearDown. Whichever point we
choose to have cleanups execute can be entirely emulated using careful
try:finally: in tearDown methods, so surely this is not an argument for
either order.

-Rob

I don't understand; neither sequence works

????

• they are showing how any
  choice [that retains the current simple proposed mechanism] cannot
  interact without some failure modes with tearDown.

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
things possible that would otherwise not be possible.

If your cleanup relies on something which has been set up during setUp
and will be dropped during tearDown (a database connection, a temp dir,
an ssh session, whatever), then the cleanup must be run before the
teardown.

But this is a function of whichever way we do it - and so not an
argument for one way or the other. Conversely if you write a tearDown
that relies on resources existing that will later be removed by a clean
up then clean ups must be run afterwards.

The point is that sequence 2 can already be emulated using careful
"try...finally" in tearDown, while sequence 1 cannot. That is, sequence
1 *needs* the addCleanup, while for sequence 2 it is a mere additional
convenience.

Which is an argument in favour of running clean ups afterwards.

A use case for running clean ups before tearDown (in which case
addCleanUp would need to raise an exception if called during tearDown).

You have existing code which (for example) creates an SSH connection in
setUp and removes it in tearDown.

If clean ups are run after tearDown then you can't use addCleanup inside
tests to deallocate resources that rely on the SSH connection being
there (i.e. issue a remote command).

Switching to use clean ups would require rewriting the existing code.
Running clean ups before tearDown is more likely to fit in with existing
test code.

The downsides are that it makes adding clean ups in tearDown impossible
(but why would you want to do that?) and you can't use clean ups for any
resource accessed in tearDown (which you may want to do but won't be the
case for any code that currently exists...).

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
cleanup is performed before tearDown or after tearDown. Don't we risk
confusing developers who are not familiar with the cleanup order?

Do we really want to add this feature? The added functionality doesn't
seem so attractive compared to the risk of confusion. We might as well
continue to let the developers implement this feature in their own
TestCase subclasses (for example, my own subclass already implement a
TestCase.create_tmpdir(name) (which does cleanup, of course), which is
easier to use than the rmtree example that was given as a use case). At
least, when they do, they know about the cleanup order.

So, we are talking about adding a feature that could cause problem whether
cleanup is performed before tearDown or after tearDown. Don't we risk
confusing developers who are not familiar with the cleanup order?

Well, we could do both. Call cleanups before tearDown (all the while
popping them), call tearDown, and call the new cleanups.

If the cleanup machinery is written carefully enough, you may even be
able to add another cleanup during a cleanup :-)

This is actually a minor point about the order things happen in. I don't
think it will cause confusion in practise once we have made a decision
and documented it.

Particularly if we decide to call clean ups before tearDown then I will
make adding new ones during tearDown raise an exception.

I did think about having two separate clean up stacks - one for those
added during setUp and one for those added during tests, but that is
starting to get silly.

The point about this patch is that it makes the deallocation of
resources a lot simpler without having to manually track them inside
your test (nested try finallys for this are not uncommon in the tests
I've seen) and the fact that this mechanism is already in use in the
test frameworks of three major projects indicate that it is definitely
worthwhile.

I see the validity in the concern raised by Virgil:

I think this can be simplified from a user perspective by saying `I'm
electing to use cleanUp over tearDown'. The two systems are similar, but
also very different semantically as tearDown gets executed in the event
of test completion (regardless of whether or not it passed) whereas
cleanUp gets executed based on what occurs in setUp.

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
that will confuse people...

On Sun, 2009-04-05 at 21:31 +0000, Antoine Pitrou wrote:

Antoine Pitrou <pitrou@free.fr> added the comment:

> So, we are talking about adding a feature that could cause problem whether
> cleanup is performed before tearDown or after tearDown. Don't we risk
> confusing developers who are not familiar with the cleanup order?

Well, we could do both. Call cleanups before tearDown (all the while
popping them), call tearDown, and call the new cleanups.

I think this makes cleanups harder to explain, but yes we could do it.
(Its harder because its no longer clearly a LIFO as cleanups added
before teardown at not executed last).

If the cleanup machinery is written carefully enough, you may even be
able to add another cleanup during a cleanup :-)

I've been meaning to tweak the patch -
rather than
+ for function, args, kwargs in reversed(self._cleanups):
it should be
+ while self._cleanups:
+ function, args, kwargs = self._cleanups.pop(-1)

precisely to allow this.

The point I've been trying to make isn't just that after everything is
clearly best (which is subjective), but that dropping cleanups from user
code is clearly wrong (something I hope we all agree on) and that its
harder to have bugs like that if cleanups run after teardown.

@antoine I appreciate that you feel very strongly about this; I'm at a
bit of a loss why, given that I've been working with this feature for
over 3 *years* and we've not once had headaches or confusion related to
the discussed sequencing failure modes - and our code runs it after all
the user code. I've tried my level best to document why it works best
after running after all the user code, but ultimately whoever is
committing this code to python needs to make a call about what you think
is best. It would be nice if you can learn from our experience, but I'm
about out of energy to discuss this; certainly when we're down to
authority by posturing it seems like fruitful discussion is rather over.

@michael - you may have a convenience function that uses addCleanup - if
that gets called during tearDown then while its a little odd its still
correct for cleanups the convenience function added to get called. It's
only in the light of having to really explain why it should be
afterwards that I've realised we had a latent bug - fixed in the change
above [and a test for it is pretty obvious].

-Rob

@garrett / Virgil

I don't think anything other than very short term confusion is possible.
Whichever decision is made a developer who assumes the opposite will
actually get an error. In both cases the downside is that in certain
circumstances you could attempt to use a resource that has already been
disposed of.

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:

As an interesting data point, the Bzr code does clean ups *before*
tearDown.

No it doesn't:

We subclass unittest.TestCase. We also override run() to make tearDown
run always.

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,
even though we implement it by calling it from our base-most 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:

Michael Foord <michael@voidspace.org.uk> added the comment:

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

Ah, indeed it does. Well, FWIW my use has nearly all been with bzr's
that does it after :)

Jono - was there some particular reason testtools does it after? (We're
discussing the merits of pre-and-post tearDown, ad-nauseum :)).

-Rob

On Mon, Apr 6, 2009 at 10:06 AM, Robert Collins
<robertc@robertcollins.net> wrote:

On Sun, 2009-04-05 at 23:57 +0000, Michael Foord wrote:
> Michael Foord <michael@voidspace.org.uk> added the comment:
>
> 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

Ah, indeed it does. Well, FWIW my use has nearly all been with bzr's
that does it after :)

Jono - was there some particular reason testtools does it after? (We're
discussing the merits of pre-and-post tearDown, ad-nauseum :)).

No particular reason.

I like this patch. However, a nice-to-have would be that _doCleanups()
prints a traceback from function(*args, **kwargs) (if there is one) the
same way that atexit does. That would aid in debugging mis-written
cleanup functions yet would not intrude on your test program.

On Mon, Apr 6, 2009 at 2:16 PM, Kumar McMillan <report@bugs.python.org> wrote:

Kumar McMillan <kumar.mcmillan@gmail.com> added the comment:

I like this patch.  However, a nice-to-have would be that _doCleanups()
prints a traceback from function(*args, **kwargs) (if there is one) the
same way that atexit does.  That would aid in debugging mis-written
cleanup functions yet would not intrude on your test program.

I think reporting the errors via addError is the right thing. If your
cleanups fail, then your test suite becomes unreliable, so they should
be treated like tests that raise unexpected errors.

FWIW, I kind of like the tests here:
http://bazaar.launchpad.net/~jml/testtools/trunk/annotate/head%3A/testtools/tests/test_testtools.py#L221

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:

Michael Foord <michael@voidspace.org.uk> added the comment:

Proper patch and proper issue this time! Not my evening.

----------
Added file: http://bugs.python.org/file13787/unittest-no-exit.patch

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
couple of days.

I've settled on calling doCleanups *after* tearDown. The issues and
reasoning explained below.

One point of view concerns using addCleanups with existing tests.

If the setUp allocates resources that are deallocated by tearDown, the
natural LIFO pattern would seem to be setUp -> tests that create cleanup
functions -> do cleanups (which may use resources allocated by setUp) ->
tearDown.

This pattern doesn't allow tearDown to create cleanup functions and
doesn't permit setUp to create cleanup functions if tearDown need to
access those resources (unless tearDown itself becomes one big cleanup
function).

If you look at the situation with new tests then it is perfectly natural
for setUp, tests and tearDown to all create cleanup functions. The
natural LIFO order is for cleanups to be run after tearDown.

The solution I've opted for is to make doCleanups public. If you are
adding the use of cleanups to old tests and need the cleanup functions
run before tearDown then you are free to decorate the test with a
function that calls doCleanups on success or failure.

This takes into account the experience of those already using test
frameworks that provide this mechanism.

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".