python / cpython

The Python programming language
https://www.python.org
Other
63.2k stars 30.26k forks source link

cleanUp stack for unittest #49929

Closed voidspace closed 15 years ago

voidspace commented 15 years ago
BPO 5679
Nosy @gpshead, @pitrou, @rbtcollins, @ezio-melotti, @ngie-eign, @voidspace
Files
  • unittest-cleanup.diff
  • unittest-no-exit.patch
  • unittest-cleanup.patch
  • 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: ```python assignee = 'https://github.com/voidspace' closed_at = created_at = labels = ['type-feature', 'library'] title = 'cleanUp stack for unittest' updated_at = user = 'https://github.com/voidspace' ``` bugs.python.org fields: ```python activity = actor = 'ezio.melotti' assignee = 'michael.foord' closed = True closed_date = closer = 'michael.foord' components = ['Library (Lib)'] creation = creator = 'michael.foord' dependencies = [] files = ['13612', '13787', '13791'] hgrepos = [] issue_num = 5679 keywords = ['patch'] message_count = 45.0 messages = ['85321', '85323', '85324', '85325', '85344', '85404', '85407', '85413', '85432', '85434', '85437', '85439', '85443', '85444', '85445', '85446', '85447', '85463', '85465', '85466', '85471', '85494', '85497', '85562', '85565', '85567', '85568', '85569', '85570', '85572', '85589', '85590', '85592', '85593', '85594', '85609', '85612', '85613', '86568', '86569', '86576', '86607', '86978', '86998', '130459'] nosy_count = 9.0 nosy_names = ['gregory.p.smith', 'pitrou', 'rbcollins', 'vdupras', 'jml', 'ezio.melotti', 'kumar303', 'ngie', 'michael.foord'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'closed' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue5679' versions = ['Python 2.7'] ```

    voidspace commented 15 years ago

    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)
    gpshead commented 15 years ago

    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.

    voidspace commented 15 years ago

    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

    voidspace commented 15 years ago

    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.

    voidspace commented 15 years ago

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

    voidspace commented 15 years ago

    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

    pitrou commented 15 years ago

    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.

    voidspace commented 15 years ago

    I'm agnostic on before / after tearDown, so happy to make that change.

    rbtcollins commented 15 years ago

    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,

    rbtcollins commented 15 years ago

    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.

    pitrou commented 15 years ago

    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.

    pitrou commented 15 years ago

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

    rbtcollins commented 15 years ago

    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

    voidspace commented 15 years ago

    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.

    pitrou commented 15 years ago

    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.

    voidspace commented 15 years ago

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

    pitrou commented 15 years ago

    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.

    rbtcollins commented 15 years ago

    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

    cd099432-c439-4196-9816-d22314ec360c commented 15 years ago

    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:

    Another thought: Why not have an option for defining a method called incrementalTearDown', which replacestearDown' 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.

    rbtcollins commented 15 years ago

    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 replacestearDown' 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

    pitrou commented 15 years ago

    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.

    rbtcollins commented 15 years ago

    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

    pitrou commented 15 years ago

    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.

    voidspace commented 15 years ago

    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.

    voidspace commented 15 years ago

    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. :-)

    2e122ceb-1a00-4209-b1e7-0f43b93643e9 commented 15 years ago

    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.

    pitrou commented 15 years ago

    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 :-)

    voidspace commented 15 years ago

    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.

    cd099432-c439-4196-9816-d22314ec360c commented 15 years ago

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

    rbtcollins commented 15 years ago

    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

    voidspace commented 15 years ago

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

    rbtcollins commented 15 years ago

    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

    voidspace commented 15 years ago

    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

    rbtcollins commented 15 years ago

    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

    d59526fe-79e8-4e27-ab58-d948dca6ef5c commented 15 years ago

    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.

    ffd62514-7df1-41fd-a0c2-d5f3b09736e0 commented 15 years ago

    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.

    d59526fe-79e8-4e27-ab58-d948dca6ef5c commented 15 years ago

    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.

    d59526fe-79e8-4e27-ab58-d948dca6ef5c commented 15 years ago

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

    voidspace commented 15 years ago

    Damn - proper patch without extraneous stuff this time.

    voidspace commented 15 years ago

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

    rbtcollins commented 15 years ago

    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

    voidspace commented 15 years ago

    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.

    voidspace commented 15 years ago

    Committed in revision 72219.

    cd099432-c439-4196-9816-d22314ec360c commented 15 years ago

    Cool! Thanks for all of the hard work Michael :D.

    ezio-melotti commented 13 years ago

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