classification
Title: tearDown in unittest should be executed regardless of result in setUp
Type: behavior Stage:
Components: Tests Versions: Python 2.4, Python 2.6, Python 2.5
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: purcell Nosy List: exarkun, kumar303, michael.foord, ngie, purcell, rhettinger
Priority: normal Keywords: needs review

Created on 2009-03-22 21:16 by ngie, last changed 2009-04-03 20:34 by michael.foord. This issue is now closed.

Files
File name Uploaded Description Edit
issue-5538-2.4.5.diff ngie, 2009-03-22 21:25 Patch for 2.4.x
Messages (21)
msg83983 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-22 21:16
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'
<assert fails>
`Calling teardown_module'

2. Removing assert False from setup_module...

`Calling setup_module'
- `Calling function_setup'
<assert fails>
- `Calling function_teardown'
- `Calling TestClass:setUp'
<assert fails>
- `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.
msg83985 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-22 21:25
That patch wasn't complete -_-... here's a more complete 2.4.x patch.
msg83986 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-22 21:43
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
msg83987 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-03-22 23:19
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.
msg83998 - (view) Author: Steve Purcell (purcell) (Python triager) Date: 2009-03-23 08:48
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.
msg83999 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-23 09:11
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
msg84004 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2009-03-23 11:41
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)
msg84011 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-23 15:54
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*.
msg84012 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2009-03-23 15:56
> 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.
msg84013 - (view) Author: Steve Purcell (purcell) (Python triager) Date: 2009-03-23 16:00
Sounds good to me.
msg84014 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-23 16:03
Ok, sounds good then ;). I'll open another issue with the enhancement
later on this week.

Cheers!
msg84017 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-23 16:34
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!
msg84018 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2009-03-23 16:36
/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.
msg84035 - (view) Author: Kumar McMillan (kumar303) Date: 2009-03-23 21:13
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.
msg84040 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-23 21:35
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 :).
msg84046 - (view) Author: Kumar McMillan (kumar303) Date: 2009-03-23 22:47
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().
msg84047 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-23 23:11
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*.
msg84509 - (view) Author: Enji Cooper (ngie) * Date: 2009-03-30 05:15
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.
msg85309 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2009-04-03 16:55
+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.
msg85310 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2009-04-03 17:00
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...
msg85328 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2009-04-03 20:34
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 (#5679) for the cleanUp idea which I think is good.
History
Date User Action Args
2009-04-03 20:34:09michael.foordsetmessages: + msg85328
2009-04-03 17:00:10michael.foordsetmessages: + msg85310
2009-04-03 16:55:24michael.foordsetnosy: + michael.foord
messages: + msg85309
2009-03-30 05:15:57ngiesetmessages: + msg84509
2009-03-23 23:11:46ngiesetmessages: + msg84047
2009-03-23 22:47:53kumar303setmessages: + msg84046
2009-03-23 21:35:12ngiesetmessages: + msg84040
2009-03-23 21:13:33kumar303setnosy: + kumar303
messages: + msg84035
2009-03-23 16:36:59exarkunsetmessages: + msg84018
2009-03-23 16:34:52ngiesetmessages: + msg84017
2009-03-23 16:03:22ngiesetmessages: + msg84014
2009-03-23 16:00:41purcellsetmessages: + msg84013
2009-03-23 15:56:43exarkunsetmessages: + msg84012
2009-03-23 15:54:18ngiesetmessages: + msg84011
2009-03-23 11:41:19exarkunsetnosy: + exarkun
messages: + msg84004
2009-03-23 09:11:11ngiesetmessages: + msg83999
2009-03-23 08:48:47purcellsetstatus: open -> closed
resolution: rejected
messages: + msg83998
2009-03-22 23:19:46rhettingersetnosy: + rhettinger, purcell
messages: + msg83987

assignee: purcell
keywords: + needs review, - patch
2009-03-22 21:43:06ngiesetmessages: + msg83986
2009-03-22 21:25:32ngiesetfiles: - issue-blah-2.4.5.diff
2009-03-22 21:25:24ngiesetfiles: + issue-5538-2.4.5.diff

messages: + msg83985
2009-03-22 21:16:34ngiesettype: behavior
2009-03-22 21:16:21ngiecreate