Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setUpClass equivalent for addCleanup #68600

Closed
bitdancer opened this issue Jun 8, 2015 · 18 comments
Closed

setUpClass equivalent for addCleanup #68600

bitdancer opened this issue Jun 8, 2015 · 18 comments
Labels
3.8 (EOL) end of life easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@bitdancer
Copy link
Member

BPO 24412
Nosy @warsaw, @rbtcollins, @ezio-melotti, @bitdancer, @voidspace, @axitkhurana, @serhiy-storchaka, @vajrasky, @lisroach
PRs
  • bpo-24412: Adds cleanUps for setUpClass and setUpModule. #9190
  • bpo-24412: Use assertEqual to fix DeprecationWarning #10794
  • Files
  • addCleanupClass.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:

    assignee = None
    closed_at = <Date 2019-06-10.21:23:12.336>
    created_at = <Date 2015-06-08.18:31:39.309>
    labels = ['easy', '3.8', 'type-feature', 'library']
    title = 'setUpClass equivalent for addCleanup'
    updated_at = <Date 2019-06-10.21:23:12.335>
    user = 'https://github.com/bitdancer'

    bugs.python.org fields:

    activity = <Date 2019-06-10.21:23:12.335>
    actor = 'lisroach'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-06-10.21:23:12.336>
    closer = 'lisroach'
    components = ['Library (Lib)']
    creation = <Date 2015-06-08.18:31:39.309>
    creator = 'r.david.murray'
    dependencies = []
    files = ['39844']
    hgrepos = []
    issue_num = 24412
    keywords = ['patch', 'easy']
    message_count = 18.0
    messages = ['245030', '245293', '245295', '245299', '245301', '245318', '245323', '245530', '245531', '245534', '245540', '246084', '275890', '275895', '326531', '326533', '329494', '330624']
    nosy_count = 10.0
    nosy_names = ['barry', 'rbcollins', 'ezio.melotti', 'r.david.murray', 'michael.foord', 'axitkhurana', 'serhiy.storchaka', 'vajrasky', 'lisroach', 'dablanc']
    pr_nums = ['9190', '10794']
    priority = 'normal'
    resolution = None
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue24412'
    versions = ['Python 3.8']

    @bitdancer
    Copy link
    Member Author

    Since tearDownClass isn't run if setUpClass fails, there is a need for the class level equivalent of addCleanup. addClassCleanup?

    @bitdancer bitdancer added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jun 8, 2015
    @taleinat
    Copy link
    Contributor

    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.

    @rbtcollins
    Copy link
    Member

    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.

    @taleinat
    Copy link
    Contributor

    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.

    @serhiy-storchaka
    Copy link
    Member

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

    @bitdancer
    Copy link
    Member Author

    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.

    @taleinat
    Copy link
    Contributor

    Ahh, that makes sense. Sounds good to me!

    @bitdancer
    Copy link
    Member Author

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

    @bitdancer bitdancer added the easy label Jun 19, 2015
    @serhiy-storchaka
    Copy link
    Member

    contextlib.ExitStack could help you.

    @bitdancer
    Copy link
    Member Author

    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.

    @taleinat
    Copy link
    Contributor

    I'm convinced. +1

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Jul 2, 2015

    Here is the patch.

    @rbtcollins
    Copy link
    Member

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

    @rbtcollins
    Copy link
    Member

    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

    @serhiy-storchaka
    Copy link
    Member

    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.

    @serhiy-storchaka serhiy-storchaka added the 3.8 (EOL) end of life label Sep 27, 2018
    @rbtcollins
    Copy link
    Member

    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.

    @lisroach
    Copy link
    Contributor

    lisroach commented Nov 9, 2018

    New changeset 0f221d0 by Lisa Roach in branch 'master':
    bpo-24412: Adds cleanUps for setUpClass and setUpModule. (GH-9190)
    0f221d0

    @serhiy-storchaka
    Copy link
    Member

    /home/serhiy/py/cpython/Lib/unittest/test/test_runner.py:259: DeprecationWarning: Please use assertEqual instead.
    self.assertEquals(e, 'cleanup1')

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 (EOL) end of life easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants