tearDown in unittest should be executed regardless of result in setUp

[cd099432-c439-4196-9816-d22314ec360c]

BPO	5538
Nosy	@rhettinger, @ngie-eign, @voidspace
Files	issue-5538-2.4.5.diff: Patch for 2.4.x

^{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 = None closed_at = created_at = labels = ['type-bug', 'tests'] title = 'tearDown in unittest should be executed regardless of result in setUp' updated_at = user = 'https://github.com/ngie-eign' ``` bugs.python.org fields: ```python activity = actor = 'michael.foord' assignee = 'purcell' closed = True closed_date = closer = 'purcell' components = ['Tests'] creation = creator = 'ngie' dependencies = [] files = ['13398'] hgrepos = [] issue_num = 5538 keywords = ['needs review'] message_count = 21.0 messages = ['83983', '83985', '83986', '83987', '83998', '83999', '84004', '84011', '84012', '84013', '84014', '84017', '84018', '84035', '84040', '84046', '84047', '84509', '85309', '85310', '85328'] nosy_count = 6.0 nosy_names = ['rhettinger', 'purcell', 'exarkun', 'kumar303', 'ngie', 'michael.foord'] pr_nums = [] priority = 'normal' resolution = 'rejected' stage = None status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue5538' versions = ['Python 2.6', 'Python 2.5', 'Python 2.4'] ```

[cd099432-c439-4196-9816-d22314ec360c]

While trying to deal with some annoying issues with setting up and tearing down consoles via pexpect, I noticed that the teardown functions / methods weren't being executed via nose.

After applying this change to 2.4.5 and 2.6.1, things work as expected (note: this patch only applies cleanly with 2.4.5 AFAICT -- it didn't work with 2.6.1).

My expectations are:

\========

I'd expect that the best fix would be for tearDown to be unconditionally called if setUp is called, s.t. testers can ensure that the operating state of the UUT and testbed are at a predefined state.

So in my testcase provided, I would expect the following flow:

1. Keeping assert False in setup_module... `Calling setup_module'

   `Calling teardown_module'

2. Removing assert False from setup_module...

`Calling setup_module'

• `Calling function_setup'
• `Calling function_teardown'
• `Calling TestClass:setUp'
• Calling TestClass:tearDown' Calling teardown_module'

If this isn't done, it makes some operations, like tearing down consoles with pexpect _extremely_ painful to perform...

\========

Please see \http://code.google.com/p/python-nose/issues/detail?id=234\ for more details.

[cd099432-c439-4196-9816-d22314ec360c]

That patch wasn't complete -_-... here's a more complete 2.4.x patch.

[cd099432-c439-4196-9816-d22314ec360c]

If someone will provide a link to a page with instructions on how to checkout python from cvs / svn / etc I'll gladly apply a patch to the sources on 2.x and 3.x HEAD.

Thanks, -Garrett

[rhettinger]

It's up to Steve to decide wheter to commit. I presume that he will want to stay true to JUnit and to avoid risk of breaking existing test suites.

[90838a04-9daf-41f6-9aa8-5765707f0a3e]

Indeed -- if some of the setUp code is likely to fail, that should be handled right there in setUp, by reversing any other setup code that may have executed.

It's harder to write a tearDown that will work reliably for any partially successful setUp.

[cd099432-c439-4196-9816-d22314ec360c]

I agree with you guys to a certain extent, but doing so only complicates my setup procedure to the extent that I'm calling a lot of my teardown code in a dumb manner, unless I actually kept track of how far into the setup process I got before everything went awry and split it into a multistep rollback process for atomicity.

This in and of itself seems like a lot to track with complicated processes like configuring a daemon interactively via pexpect, or modifying a remote VM instance with multiprocessing being used for injection, and I don't know if I can expect consumers of my test suites to get it right twice.

Could you guys provide some examples of existing test suites where this may potentially break things?

Thanks! -Garrett

[8726d1eb-a365-45b6-b81d-c75988975e5a]

Here's one:

class ReactorShutdownInteraction(unittest.TestCase):
    """Test reactor shutdown interaction"""

    def setUp(self):
        """Start a UDP port"""
        self.server = Server()
        self.port = reactor.listenUDP(0, self.server, interface='127.0.0.1')

    def tearDown(self):
        """Stop the UDP port"""
        return self.port.stopListening()

Perhaps a feature like trial's `addCleanup´ should be added. This lets you deal with cleanup in a somewhat simpler way. For example, rewriting the above:

class ReactorShutdownInteraction(unittest.TestCase):
    """Test reactor shutdown interaction"""

    def setUp(self):
        """Start a UDP port"""
        self.server = Server()
        self.port = reactor.listenUDP(0, self.server, interface='127.0.0.1')
        # Stop the UDP port after the test completes.
        self.addCleanup(self.port.stopListening)

[cd099432-c439-4196-9816-d22314ec360c]

And maybe the addCleanup components could be a stack of callable objects?

This would make life easier for folks like me and would make the overall flow much cleaner / clearer as it would allow us to break things down into atomic steps which could be reverted in a LIFO order.

Steve / Raymond: Do you guys like Jean-Paul's proposal? If so, I'll write up a patch sometime this coming weekend to implement that functionality.

For now I'll just call tearDown blindly on Exceptions *shivers*.

[8726d1eb-a365-45b6-b81d-c75988975e5a]

And maybe the addCleanup components could be a stack of callable objects?

Yea, that's handy. My example didn't show it, but that's what trial implements.

[90838a04-9daf-41f6-9aa8-5765707f0a3e]

Sounds good to me.

[cd099432-c439-4196-9816-d22314ec360c]

Ok, sounds good then ;). I'll open another issue with the enhancement later on this week.

Cheers!

[cd099432-c439-4196-9816-d22314ec360c]

Before I forget though -- should the requirements for the functionality be that it be called strictly in setUp on failure, or should it be executed as a part of tearDown as well?

Thanks!

[8726d1eb-a365-45b6-b81d-c75988975e5a]

/Any/ addCleanup that is successfully executed should cause the cleanup function to be called. Otherwise you have to define all your cleanup twice, and that's no fun.

[ffd62514-7df1-41fd-a0c2-d5f3b09736e0]

fwiw, changing tearDown() so that it executes unconditionally will break a countless number of test suites. No test suite since the dawn of unittest has been designed to tearDown() what may not have been setUp() correctly.

in other words, this is how [in my experience] setUp and tearDown are typically used together. Imageine this error in setUp() :

def setUp(self):
    self.tmp_io = TempIO() # raise NameError("no such name TempIO")
    self.db = DataBase()

def tearDown(self):
    self.tmp_io.destroy()
    self.db.destroy()

With the change, you would need messy code like:

def tearDown(self):
    if hasattr(self, 'tmp_io'):
        self.tmp_io.destroy()
    if hasattar(self, 'db'):
        self.db.destroy()

This is just a simple example; things would get complicated fast.

I think addCleanup() is a good idea though. Or what about a new hook that would act like tearDown() but execute unconditionally. alwaysTearDown() ? (I'm bad with names.) Using a different hook would solve the problem of porting test suites to this new paradigm. But besides that there are alternatives for doing cleanup. I.E. if setUp() in a class fails, then teardown_module() will still be called.

[cd099432-c439-4196-9816-d22314ec360c]

Excellent point Kumar.

Here's what I'm trying to accomplish...

I'm a part of a team that's testing out IOSd on an up and coming Unix foundation platform. This requires starting up a series of services, ensuring that they have come up, and then login to the IOS console, then IF the user gets their configuration correct (because I'm providing a means for the user to configure IOS) the setup stage of the testcase will be complete. IF NOT (and here comes the tricky part), the UUT is in a really questionable / funky state and I can't be sure if the system is really usable for additional tests without restarting / reinitializing it.

This in and of itself is more problematic to deal with as that means I'd need to attach myself to the local console and listen for restart to complete, then issue configuration parameters to boot up the device, etc... This is being done once by another Tcl script, so I'm trying to avoid having to reinvent the wheel for known working methods.

Hopefully that better illustrates the quandary that I'm dealing with :).

[ffd62514-7df1-41fd-a0c2-d5f3b09736e0]

For the record, I too have been plagued by failing setUp's :( and the pattern you describe is not uncommon. I was just pointing out that changing the semantics of tearDown() will affect a lot of existing code. As with any backwards incompatible change the effort of porting to the new functionality should be considered. In this case my fear is that it will be hard to know that tearDown() is not behaving how it used to behave since an exception in tearDown() would be once removed from the actual exception in setUp().

More directly addressing your problem, have you considered switching to context managers?

Could maybe do something like:

with ConfiguredIOS():
    test_something()

the context manager could define __exit__() which would get called unconditionally. Also, these could be chained together as decorators to sort-of do what it seems like you were trying to do in tearDown().

[cd099432-c439-4196-9816-d22314ec360c]

Unfortunately we're still stuck on 2.4.5 because I don't have enough buy-in from tech leads and architects to upgrade right now -_-... Good idea though :] *stores for later*.

[cd099432-c439-4196-9816-d22314ec360c]

As an FYI, I'm going to push this off until next week or the week after because I have more pressing things to take care of and have an OK workaround for this issue.

[voidspace]

+1 on adding a cleanUp list where entries are executed unconditionally even on failure of setUp.

If there is consensus that this is a good thing (looks like it to me from the discussion here) then I can apply the patch to head.

I'll review it first and post any further comments here.

[voidspace]

OK, so the patch as submitted *isn't* for the cleanUp suggestion (d'oh - I should have read it before posting the last comment).

One possibility is that this bug is closed as won't fix (the patch as provided should definitely not be applied as it is a big change to unittest setUp / tearDown semantics) and a new feature request be added for cleanUp.

OTOH if we do have consensus that cleanUp is a worthy feature I'll just do it...

[voidspace]

Closing. The patch as suggested should not be applied. Instead tearDown should be called in a finally block in the setUp for this specific use case.

I've created a new issue (bpo-5679) for the cleanUp idea which I think is good.