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: setUpClass equivalent for addCleanup
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: axitkhurana, barry, dablanc, ezio.melotti, lisroach, michael.foord, r.david.murray, rbcollins, serhiy.storchaka, vajrasky
Priority: normal Keywords: easy, patch

Created on 2015-06-08 18:31 by r.david.murray, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
addCleanupClass.patch vajrasky, 2015-07-02 15:21 review
Pull Requests
URL Status Linked Edit
PR 9190 merged lisroach, 2018-09-11 22:02
PR 10794 merged xtreak, 2018-11-29 12:42
Messages (18)
msg245030 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-06-08 18:31
Since tearDownClass isn't run if setUpClass fails, there is a need for the class level equivalent of addCleanup.  addClassCleanup?
msg245293 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-13 06:31
Is this really needed? One can use try/except/raise, and since addClassCleanup() would only be called from setUpClass(), I don't quite see the utility of it.
msg245295 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-06-13 06:43
It would be nice for symmetry. I mean, setUpClass isn't needed either, and we have it ;).

however, we actually have two contexts this would be called from - setUpClass and setUpModule; both share their internals. So we probably need a decoupled cleanups implementation, and two new binding points to it.
msg245299 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-13 07:02
I'm not convinced this would be worth the effort required to implement and maintain it.

Can someone find examples from existing test suites where this would clearly be useful? For example, a setUpClass() or setUpModule() function with multiple try/finally clauses.
msg245301 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-06-13 07:23
addCleanup() is helpful because it can be used in test methods. addClassCleanup() and addModuleCleanup() can't be used in test methods, and setUpClass() and setUpModule() are used less than setUp(), therefore the benefit of these methods are less than of addCleanup().
msg245318 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-06-13 14:57
Not having addClassCleanup means that my setUpClass method will have four try/excepts in it, as will my tearDownClass (I have four fixtures I'm setting up for the tests).

So, no, it isn't strictly needed, but it is prettier :).  As Robert says, though, it makes for a nice symmetry in the API.  Basically, I like to see tearDown deprecated, as I find the setup/addcleanup pattern much easier to write and, more importantly, to read.
msg245323 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-13 19:06
Ahh, that makes sense. Sounds good to me!
msg245530 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-06-19 22:16
As further motivation, I actually tried to implement this using try/except.  First I wrote a loop in tearDownClass that ran each of the cleanups inside a try/except and printed the exception if there was one.  But of course that doesn't run if setUpClass fails.  So I started to add a similar try/except loop in setUpClass...and then realized that in order to have the cleanups run correctly, I needed to add each cleanup to a list on the class if and only if the corresponding setup succeeded, and then run only those cleanups in the tearDownClass.

So, I ended up implementing addClassCleanup.  However, in order to make it generic (I have two classes where I need it currently, and will probably be adding more), I have a base class with setUpClass that calls a safeSetUpClass inside a try/except, my safeSetUpClass on the actual test class does the setup and calls to addClassCleanUp, and my tearDownClass does the loop over the accumulated cleanups.

So, yeah, it would be really handy to have this as an actual feature :)
msg245531 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-06-19 22:33
contextlib.ExitStack could help you.
msg245534 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-06-19 22:51
That would not make it simpler.  In fact it would make the test code more complex.  I'd still need the safeSetUpClass dodge (or repeat the try/except/log in every setUpClass method), and I'd have to have a wrapper function that I passed each cleanup to as an argument to wrap it in a try/except/log, since close would just propagate the exceptions.  

I think implementing this parallel to addCleanUp is much cleaner, even if the feature isn't (yet :) in the stdlib.
msg245540 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-20 05:24
I'm convinced. +1
msg246084 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2015-07-02 15:21
Here is the patch.
msg275890 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2016-09-11 23:58
So, thank you for the patch. However - there's no need for a metaclass here, and its actively harmful to comprehending the code.

As I suggested earlier, please decouple the cleanups implementation and then consume it from the two places that it will be needed (testcase and testsuite).
msg275895 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2016-09-12 00:10
Btw some things to be aware of in addressing this:

 - we will need tests that catchable exceptions raised from a setUpModule or setUpClass cause cleanups already registered to run
 - if (and I'd need to check this) setUpModule or setUpClass can nest - I think setUpModule may, in package.module contexts) - that non-local cleanups definitively run too
msg326531 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-27 08:01
There is a design issue with the order of cleaning up. tearDown() is purposed to release resources acquired in setUp(). Resources acquired in test methods will be released in doCleanup() if use addCleanup(), but since doCleanup() is called after tearDown(), resources will be released not in reversed order of the time of acquiring. This causes issues if resources acquired in a test method depends on the resource acquired in setUp().

Adding doClassCleanups() and doModuleCleanups() with the same semantic will introduce this problem at class and module level.
msg326533 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2018-09-27 08:14
Serhiy, thats not a design flaw, its a feature.

in a class hierarchy, setup and teardown ordering is undefined: implementors can choose whatever order they want to execute in. e.g.

class A(TestCase):
 def setUp(self):
  super().setUp()
  acquire1()

class B(A):
 def setUp(self):
  acquire2()
  super().setUp()

class C(B):
 def setUp(self):
  super().setUp()
  acquire3()

will acquire 2, then 1, then 3.

tearDown() is similarly ill defined.

Secondly, consider

class Example(TestCase):
 def setUp(self):
  self._1 = acquire()
  self.addCleanUp(acquire())
  self._3 = acquire()

 def tearDown(self):
  self._3.cleanUp()
  self._1.cleanUp()
  super().tearDown()

As such, there is no ordering of cleanup + teardown that is 'right' in all cases.

The question then becomes, which ordering *most facilitates* migration from tearDown to cleanup.

The allowable orders are:
 - with a more complex implementation, per-class (no)
 - before tearDown starts
 - after tearDown finishes

The common case tearDown tends to look like this:

def tearDown(self):
  <local cleanup>
  super().tearDown()

so, by placing doCleanup after tearDown, we make it possible for base classes in a test hierarchy to migrate to cleanups without breaking compatibility with child classes. The cost, as you note is that we make it harder for child classes to migrate until the base class has migrated.

But it is strictly better to permit the base class to migrate, because in projects you cannot be sure of all your subclasses out there in the wild, but you can be sure of all your base classes.
msg329494 - (view) Author: Lisa Roach (lisroach) * (Python committer) Date: 2018-11-09 02:34
New changeset 0f221d09cad46bee38d1b7a7822772df66c53028 by Lisa Roach in branch 'master':
bpo-24412: Adds cleanUps for setUpClass and setUpModule. (GH-9190)
https://github.com/python/cpython/commit/0f221d09cad46bee38d1b7a7822772df66c53028
msg330624 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-28 17:54
/home/serhiy/py/cpython/Lib/unittest/test/test_runner.py:259: DeprecationWarning: Please use assertEqual instead.
  self.assertEquals(e, 'cleanup1')
History
Date User Action Args
2022-04-11 14:58:17adminsetgithub: 68600
2019-06-10 21:23:12lisroachsetstatus: open -> closed
stage: patch review -> resolved
2018-11-29 12:42:36xtreaksetpull_requests: + pull_request10041
2018-11-28 17:54:07serhiy.storchakasetmessages: + msg330624
2018-11-09 02:34:38lisroachsetnosy: + lisroach
messages: + msg329494
2018-09-27 08:14:16rbcollinssetmessages: + msg326533
2018-09-27 08:01:15serhiy.storchakasetversions: + Python 3.8, - Python 3.6
2018-09-27 08:01:06serhiy.storchakasetmessages: + msg326531
2018-09-11 22:02:55lisroachsetstage: needs patch -> patch review
pull_requests: + pull_request8628
2017-09-28 06:58:44taleinatsetnosy: - taleinat
2017-06-29 08:17:15dablancsetnosy: + dablanc
2016-09-12 00:10:18rbcollinssetmessages: + msg275895
2016-09-11 23:58:15rbcollinssetmessages: + msg275890
2015-07-02 15:21:57vajraskysetfiles: + addCleanupClass.patch

nosy: + vajrasky
messages: + msg246084

keywords: + patch
2015-06-22 22:16:50axitkhuranasetnosy: + axitkhurana
2015-06-20 05:24:11taleinatsetmessages: + msg245540
2015-06-19 22:51:12r.david.murraysetmessages: + msg245534
2015-06-19 22:33:02serhiy.storchakasetmessages: + msg245531
2015-06-19 22:16:50r.david.murraysetkeywords: + easy

messages: + msg245530
2015-06-13 19:06:56taleinatsetmessages: + msg245323
2015-06-13 14:57:16r.david.murraysetmessages: + msg245318
2015-06-13 07:23:01serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg245301
2015-06-13 07:02:53taleinatsetmessages: + msg245299
2015-06-13 06:43:10rbcollinssetmessages: + msg245295
2015-06-13 06:31:31taleinatsetnosy: + taleinat
messages: + msg245293
2015-06-08 18:52:50barrysetnosy: + barry
2015-06-08 18:31:39r.david.murraycreate