classification
Title: Error in setUp not reported as expectedFailure (unittest)
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.2, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: michael.foord Nosy List: hpk, michael.foord, ncoghlan, r.david.murray, rbcollins
Priority: normal Keywords:

Created on 2010-11-27 15:16 by michael.foord, last changed 2014-10-25 15:28 by ncoghlan.

Messages (19)
msg122530 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-11-27 15:16
Reported by Konrad Delong.

class MyTest(unittest.TestCase):
    def setUp(self):
        raise Exception
    @unittest.expectedFailure
    def testSomething(self):
        assert False, "test method failed"




This code will report error, not expected failure.
msg122735 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-11-28 21:37
The same is also true for tearDown and cleanUp functions.
msg122745 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-11-28 23:27
Note that if an error is raised in a tearDown or cleanUp then unexpected-success should not be reported either. Not very important but might as well be fixed at the same time.
msg123485 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-06 18:29
I would expect this code to report an error of some sort, not pass as an expected failure.  The expected failure should be in the test case *only*, not in the setup or teardown methods.  That is, I don't think this is a bug, I think it is a feature that allows one to debug one's test infrastructure.
msg123493 - (view) Author: holger krekel (hpk) Date: 2010-12-06 19:06
FWIW i tend to agree and would probably prefer setup/teardown to result in an error rather than be subsumed in an expected-to-fail marked test.  I guess if one regards setup/teardown as a place to implement pre/post-conditions than the changes suggested by Michael make more sense.  I'd like to see a more "real" use case / user than the abstract case provided here.
msg123508 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-12-06 23:53
(made slightly redundant by Holger's comment but I'll continue anyway)

I think the issue is that setUp / tearDown are used for two different purposes.

The first is setting up and tearing down test infrastructure - where you do want to see to errors.

The other is for asserting pre and post conditions. If these are expected to fail (for whatever reason) then it may be perfectly reasonable to mark them as expectedFailure.

The fact that it was reported as a bug, and also that Antoine has requested being able to skip in a tear down (separate issue) shows that people are doing this.

So on the one hand - a small proportion of tests are marked expectedFailure and a very small subset of those might have a test infrastructure setup error. On the other hand for people who want setUp to test pre-conditions and want expectedFailure to work here will be completely unable to do this. It seems like not having consistent behaviour for expectedFailure will be more of a problem for those who want it than having it would be for those who don't need it.

As expectedFailure is not intended to be widely used anyway I would rather have consistency. It also allows the implementation to be simplified by unifying skip / expected fail / exception handling for all of setUp / tearDown / testMethod / cleanUp.
msg123513 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-07 02:08
I have to say that it would never have occurred to me to assert a pre or post condition and an expected failure where I expected the pre or post condition to fail, but if you've got a real use case and it would make the code simpler, I suppose I have no serious objection.  I don't use expected failure myself.  Just make sure you document it well, since it is not a behavior I would expect when using expected failure, and I'm sure there will be others like me.
msg123538 - (view) Author: holger krekel (hpk) Date: 2010-12-07 09:15
Michael, if you have it i'd like to see the original post/concrete use case. thanks, holger
msg123598 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-12-08 10:26
Well, the original report is here:

http://code.google.com/p/unittest-ext/issues/detail?id=21

I copied all the details provided into this issue though. Obviously the original reporter feels that they have a genuine use case.

There is also issue 9857 where Antoine is asking for test skipping in a tearDown. His use case works just as well for wanting to mark an expected fail in a tearDown (or a clean up function). As soon as we allow skips in tearDown / cleanUp functions it seems wise to also allow them in setUps for consistency (otherwise it becomes difficult to remember which parts of the test can skip, which can be expected fail etc):

http://bugs.python.org/issue9857

Raising SkipTest when in a tearDown method is reported as an error, rather than a skipped test.
Now doing this sounds like a weird use case, but it would be actually useful when you have a worker thread, and the tearDown method collects the exception raised in that thread and raises it again. For the worker thread to be able to use skipTest(), a SkipTest exception raised in tearDown should be properly reported as a skip.
msg144258 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-09-18 23:34
As another data point, this question came up again in the context of issue #12958.

The new test_socket.ThreadableTest uses tearDown() to pick up and reraise any exception that occurred in the client thread. This meant that my initial attempts at flagging some expected failures (due to Mac OS X limitations) didn't work - the client half of the failure was thrown in tearDown() and reported as an error.

While I've determined how to avoid that problem in the test_socket case, the general question of whether or not we consider it legitimate to put common assertions in setUp() and tearDown(), or expect that test code explicitly cope with tearDown() failures that occur due to expected test failures still needs to be addressed.

To my mind, bugs in my test infrastructure are going to get flushed out by tests that I'm neither skipping nor marking as expected failures. If I have a test that is known to fail in a way that invalidates the standard tearDown procedure for the test infrastructure, having to special case that situation in the tearDown code seems to go against the spirit of offering the "expectedFailure" decorator in the first place.

I don't think the same reasoning holds for setUp though - there's no way for a failing test to reach back and force setUp to fail, so any errors raised there are always going to be infrastructure errors.
msg144259 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2011-09-18 23:37
I think Twisted uses the tearDown to fail tests as well. As we have two use cases perhaps we should allow expectedFailure to work with failues in tearDown? (And if we do that it should cover setUp as well for symmetry or it becomes a morass of special cases.)
msg229860 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-10-23 07:42
Just to note that unittest2 tip (unreleased)had michaels proposed fix, which is different to that here. I'm going to back that out before doing a release, because they should be harmonisious.
msg229861 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-10-23 07:54
My take on this, FWIW, is that any methods in the under-test API - setUp, tearDown, test_* and anything registered via addCleanup should all support the same protocol as much as possible, whatever it is.

That is, raising a skip in setUp should skip the test. raising a skip in tearDown should skip the test, and raising a skip from a cleanup should skip the test.

This is complicated by the case where some code is called after exceptions- teardown and cleanups. Thats fairly straight forward: errors are higher precedence than failure than skips than anything which resolved as a pass.
msg229862 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-10-23 08:20
While I agree with Robert's comments in general, the main question to be resolved here is the scope of the "expectedFailure" decorator.

Yes, it's applied specifically to the test execution phase when writing the code, but the question is how the following scenarios should be handled when the test is marked that way:

- setUp throws an exception
- test passes, tearDown or cleanUp throw an exception
- test fails, tearDown or cleanUp throw an exception
- test throws an exception, tearDown or cleanUp throw an exception

From my perspective, those cases represent:

- error
- unexpected success OR error
- expected failure
- expected failure

If the test itself fails as expected, then I'm OK with cleanup code also failing as a consequence.

However, I'd also be OK with the simpler model that treats the decorator as covering the whole setUp/test/cleanUp/tearDown cycle, in which case an exception from *any* of those methods would be categorised as satisfying the "expected failure" decorator.
msg229863 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-10-23 08:47
I'd argue for 
 - An error - its not covered by the decorator.
 - An error - the last two are not covered by the decorator.
 - An error - the last two are not covered by the decorator.
 - An error - the exception isn't a 'failure' [today - we should revisit this but its a separate issue to the code coverage aspect and the last two aren't covered by the decorator.

Perhaps I'm infected by knowing too much about the plumbing, but we can make things *super* complex (both implementation and reasoning-about-for-users) if we are too clever - and making the decorator affect multiple different methods is very much across that line IMO.

"If the test itself fails as expected, then I'm OK with cleanup code also failing as a consequence."

I'm not unless we've got a really specific reason to be OK with it - in my experience that will mask nasty things like leaked temp files which can have bad consequences - and because its masked its then hard for developers to diagnose. So its bad all around.

"However, I'd also be OK with the simpler model that treats the decorator as covering the whole setUp/test/cleanUp/tearDown cycle, in which case an exception from *any* of those methods would be categorised as satisfying the "expected failure" decorator."

So - I think a simpler still model which is that the decorator covers the decorated code is better still, as it doesn't need any explanation about how its different to regular python decorators.
msg229875 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-23 14:51
I agree with Robert.  I'd rather get notified of a failure in the cleanup...if the test is an *expected* failure, then its cleanup should be designed to succeed when the test fails; anything else would be a bug in the design of the test, IMO.
msg230005 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-10-25 14:43
If you want the simple model, there's nothing to change (except perhaps documentation), as that's the way things work today.

subtest support also makes it much easier to factor out common assertions without relying on the setUp/tearDown protocol, so I'd be OK with explicitly declaring having common assertions in those methods to be an abuse of the protocol.
msg230008 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-25 15:21
Oh, now I understand what the issue is.  It never even *occurred* to me that someone might put assertions in setUp or tearDown.  So, yeah, count me in on considering that an abuse of the protocol :)

(If I want to factor out common assertions, and I do it a lot, I put them in a helper method and call it explicitly.)
msg230009 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-10-25 15:28
Heh, yeah, I've written *many* check_* methods in my life - I just hadn't made the link between that practice, and this suggestion.

I think that switches me firmly to the "don't change the behaviour" camp.
History
Date User Action Args
2014-10-25 15:28:17ncoghlansetmessages: + msg230009
2014-10-25 15:21:22r.david.murraysetmessages: + msg230008
2014-10-25 14:43:00ncoghlansetmessages: + msg230005
2014-10-23 14:51:48r.david.murraysetmessages: + msg229875
2014-10-23 08:47:19rbcollinssetmessages: + msg229863
2014-10-23 08:20:59ncoghlansetmessages: + msg229862
2014-10-23 07:54:17rbcollinssetmessages: + msg229861
2014-10-23 07:42:09rbcollinssetnosy: + rbcollins
messages: + msg229860
2011-09-18 23:37:22michael.foordsetmessages: + msg144259
2011-09-18 23:34:59ncoghlansetnosy: + ncoghlan
messages: + msg144258
2010-12-08 10:26:16michael.foordsetmessages: + msg123598
2010-12-07 09:15:23hpksetmessages: + msg123538
2010-12-07 02:08:29r.david.murraysetmessages: + msg123513
2010-12-06 23:53:48michael.foordsetmessages: + msg123508
2010-12-06 19:06:13hpksetmessages: + msg123493
2010-12-06 18:29:27r.david.murraysetnosy: + r.david.murray
messages: + msg123485
2010-12-06 17:28:46hpksetnosy: + hpk
2010-11-28 23:27:06michael.foordsetmessages: + msg122745
2010-11-28 21:37:17michael.foordsetmessages: + msg122735
2010-11-27 15:16:36michael.foordcreate