Title: setUpClass equivalent for addCleanup
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.6
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: axitkhurana, barry, dablanc, ezio.melotti, 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 2018-09-11 22:02 by lisroach.

File name Uploaded Description Edit
addCleanupClass.patch vajrasky, 2015-07-02 15:21 review
Pull Requests
URL Status Linked Edit
PR 9190 open lisroach, 2018-09-11 22:02
Messages (14)
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
Date User Action Args
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