This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: callables registered in TestCase.addCleanup should be run before tearDown
Type: behavior Stage: resolved
Components: Library (Lib) Versions:
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: Nosy List: neologix, rbcollins, yselivanov
Priority: normal Keywords:

Created on 2015-07-23 13:43 by neologix, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Messages (7)
msg247196 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2015-07-23 13:43
Consider this code:

-----------------------------------------------------
from __future__ import print_function

from pyccp.unittest import SafeTestCase


class MyTest(SafeTestCase):

    def setUp(self):
        print("setUp")

    def tearDown(self):
        print("tearDown")

    def test(self):
        print("creating")
        self.addCleanup(lambda: print("destroying"))
-----------------------------------------------------


When run:

setUp
creating
tearDown
destroying


We lose the LIFO ordering between between setUP and addCleanup, which is highly counter-intuitive, and almost always incorrect (despite addCleanup being docuemented to be run after tearDown).
msg247197 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-07-23 13:50
This is a documented behaviour, changing it would likely break too much code.

Moreover, I don't think that calling addCleanup callbacks before tearDown makes more sense than after.  tearDown is a common cleanup method for all tests of testcase, and addCleanup callbacks are for additional cleanup procedures.

-1 on changing this.
msg247198 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-23 13:57
The ordering is deliberate to support folk migrating from tearDown to cleanups - its not a bug. I thought we documented it clearly - https://docs.python.org/dev/library/unittest.html#unittest.TestCase.doCleanups - but if you wanted to submit improvements to the docs, that would be welcome.

The reverse ordering you propose, setup -> cleanups -> teardown means that its nearly impossible to migrate to cleanups bottom-up, you have to migrate top-down, which locks folk in inheritance based frameworks into waiting for the top of the tree to migrate first.

class Base(TestCase):

 def setUp(self):
  super().setUp()
  self.helper = Something()

 def tearDown(self):
  self.helper.finished()
  self.helper = None
  super().tearDown()


class Child(Base):

 def setUp(self):
  super().setUp()
  self.addCleanup(self.helper.release, my_resource)



If the order is
setup
teardown
cleanup

then in the cleanup the helper is already finished, and the release call will fail.

With the order of
setup
cleanup
teardown

The child's cleanup runs before the parents teardowns.

It does of course pose the same reverse problem: the base of an inheritance based frame for tests needs to be more conservative adopting cleanups, or signal backwards incompatibility, but since that is generally true anyway (frameworks move slower than their users), we considered it a reasonable tradeoff.


btw, you don't need a lambda in your example:

 self.addCleanup(print, 'destroying')
msg247199 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-23 13:59
Argh, and my memory had things inverted.

Anyhow - the inheritance story was the big thing, and -1 on changing.
msg247200 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2015-07-23 14:05
I understand the risk of breakeage, but that's still broken, because
we break LIFO ordering.

I'd been using addCleanup() for years and never bothered looking at
the documentation - which is admitedly a mistake - because LIFO
ordering is the natural behavior: since addCleanup() is called once
the test has been entered - so after setUp, logic would expect it to
be undone before tearDown.

But I guess that ship has sailed...
msg247201 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-07-23 14:08
> But I guess that ship has sailed...

Closing the issue.
msg247211 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-23 14:56
So yeah - setup  + cleanup is LIFO.
setup + teardown *can* be LIFO [depends on where you upcall]

setup + teardown + cleanup CANNOT be LIFO in all cases: we have to pick one, and either local-first or local-last.

So it is ultimately somewhat arbitrary and transitionary: one shouldn't use tearDown + cleanups, because tearDown is just a poor API by comparison.
History
Date User Action Args
2022-04-11 14:58:19adminsetgithub: 68882
2015-07-23 14:56:28rbcollinssetmessages: + msg247211
2015-07-23 14:08:21yselivanovsetstatus: open -> closed
resolution: not a bug
messages: + msg247201

stage: resolved
2015-07-23 14:05:36neologixsetmessages: + msg247200
2015-07-23 13:59:58rbcollinssetmessages: + msg247199
2015-07-23 13:57:17rbcollinssetnosy: + rbcollins
messages: + msg247198
2015-07-23 13:50:39yselivanovsetnosy: + yselivanov
messages: + msg247197
2015-07-23 13:43:04neologixcreate