classification
Title: assertRaises can behave differently
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, Tim.Graham, ezio.melotti, magnusc, michael.foord, python-dev, r.david.murray, rbcollins, rhettinger, serhiy.storchaka, steven.daprano
Priority: normal Keywords: patch

Created on 2015-05-06 12:16 by magnusc, last changed 2015-06-29 07:05 by rbcollins. This issue is now closed.

Files
File name Uploaded Description Edit
assert_raises_sentinel.patch serhiy.storchaka, 2015-05-06 12:49 review
assert_raises_sentinel_2.patch serhiy.storchaka, 2015-05-06 14:29 review
assert_raises_args.patch serhiy.storchaka, 2015-05-10 12:42 Raises TypeError review
assert_raises_args_deprecation.patch serhiy.storchaka, 2015-05-11 07:47 Emits DeprecationWarning review
Messages (20)
msg242658 - (view) Author: Magnus Carlsson (magnusc) Date: 2015-05-06 12:16
Hi

I have a little issue with the current assertRaises implementation. It seems that it might produce a little different results depending on how you use it.

self.assertRaises(IOError, None)

will not produce the same result as:

with self.assertRaises(IOError):
    None()

In the first case everything will be fine due to the fact that assertRaises will actually return a context if the second callable parameters is None. The second case will throw a TypeError 'NoneType' object is not callable.

I don't use None directly, but replace it with a variable of unknown state and you get a little hole where problems can creep in. In my case I was testing function decorators and that they should raise some exceptions on special cases. It turned our that I forgot to return the decorator and instead got the default None. But my tests didn't warn me about that.

Bottom line is that if I use the first assertRaises(Exception, callable) I can't rely on it to check that the callable is actually something callable.

I do see that there is a benefit of the context way, but in my opinion current implementation will allow problems to go undetected. 

My solution to this would be to rename the context variant into something different:
with self.assertRaisesContext(Exception):
    do_something()

A side note on this is that reverting back to the original behavior would allow you to reevaluate issue9587 for returning the actual exception.
msg242659 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2015-05-06 12:40
I don't think this is a bug. I think it is just a case that you have to be careful when calling functions, you actually do call the function. And that it returns what you think it does.

I think the critical point is this:

"It turned our that I forgot to return the decorator and instead got the default None. But my tests didn't warn me about that."

The solution to that is to always have a test that your decorator actually returns a function. That's what I do.
msg242660 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-06 12:49
Possible solution is to use special sentinel instead of None.
msg242661 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-05-06 12:57
The patch looks like a nice improvement.
msg242665 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-06 13:12
Why not sentinel = Object()? 

+1 for the patch, once tests are added.

This may "break" code in maintenance releases, but presumably that will be finding real bugs.  It is hard to imagine someone intentionally passing None to get the context manager behavior, even though it is documented in the doc strings (but not the main docs, I note).  If anyone can find examples of that, though, we'd need to restrict this to 3.5.
msg242673 - (view) Author: Magnus Carlsson (magnusc) Date: 2015-05-06 13:54
"The solution to that is to always have a test that your decorator actually returns a function. That's what I do."

Yes, I agree that with more tests I would have found the problem, but sometimes you forget things. And to me I want the tests to fail by default or for cases that are unspecified.

I think the sentinel solution would come a long way of solving both the issue that I reported but still keep the context solution intact.

Out of curiosity, would it be a solution to have the sentinel be a real function?

def _sentinel():
    pass
msg242675 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-06 14:29
Updated patch includes tests for the function is None. There were no tests for assertRaises(), the patch adds them, based on tests for assertWarns().

> Why not sentinel = Object()?

Only for better repr in the case the sentinel is leaked. Other variants are to make it named object, such as a function or a class, as Magnus suggested.
msg242681 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-06 15:37
Made a couple of review comments on the English in the test comments.

Patch looks good to me.
msg242683 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-06 16:15
New changeset 111ec3d5bf19 by Serhiy Storchaka in branch '2.7':
Issue #24134: assertRaises() and assertRaisesRegexp() checks are not longer
https://hg.python.org/cpython/rev/111ec3d5bf19

New changeset 5418ab3e5556 by Serhiy Storchaka in branch '3.4':
Issue #24134: assertRaises(), assertRaisesRegex(), assertWarns() and
https://hg.python.org/cpython/rev/5418ab3e5556

New changeset 679b5439b9a1 by Serhiy Storchaka in branch 'default':
Issue #24134: assertRaises(), assertRaisesRegex(), assertWarns() and
https://hg.python.org/cpython/rev/679b5439b9a1
msg242684 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-06 16:16
Thank you David for your corrections.
msg242845 - (view) Author: Tim Graham (Tim.Graham) * Date: 2015-05-09 23:22
I noticed this is backwards incompatible for a small feature in Django. If you want to leave this feature in Python 2.7 and 3.4, it'll break things unless we push out a patch for Django; see https://github.com/django/django/pull/4637.
msg242852 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-10 07:54
Given one failure there are probably more.  So we should probably back this out of 2.7 and 3.4.
msg242855 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-10 12:42
Agree, this change breaks general wrappers around assertRaises, and this breakage is unavoidable. Likely we should rollback changes in maintained releases.

The fix in Django doesn't LGTM. It depends on internal detail.

More correct fix should look like:

    def assertRaisesMessage(self, expected_exception, expected_message,
                            *args, **kwargs):
        return six.assertRaisesRegex(self, expected_exception,
                re.escape(expected_message), *args, **kwargs)

I used special sentinel because it is simple solution, but we should discourage to use this detail outside the module. Proposed patch (for 3.5) uses a little more complex approach, that doesn't attract to use implementation details. In additional, added more strict argument checking, only the msg keyword parameter now is acceptable in context manager mode. Please check changed docstrings.

It is possible also to make transition softer. Accept None as a callable and emit the deprecation warning.
msg242872 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-10 23:23
Yeah, the general case of wrappers is something I hadn't considered.

Probably we should go the deprecation route.  Robert, what's your opinion?
msg242996 - (view) Author: Tim Graham (Tim.Graham) * Date: 2015-05-12 19:21
I didn't find any problems while testing your proposed new patch for cpython and your proposed patch for Django together.
msg243313 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-16 13:33
New changeset 0c93868f202e by Serhiy Storchaka in branch '2.7':
Reverted issue #24134 changes.
https://hg.python.org/cpython/rev/0c93868f202e

New changeset a69a346f0c34 by Serhiy Storchaka in branch '3.4':
Reverted issue #24134 changes (except new tests).
https://hg.python.org/cpython/rev/a69a346f0c34

New changeset ac13f0390866 by Serhiy Storchaka in branch 'default':
Issue #24134: assertRaises(), assertRaisesRegex(), assertWarns() and
https://hg.python.org/cpython/rev/ac13f0390866
msg243356 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-16 18:56
Interesting, the last patch exposed a flaw in test_slice.

    def test_hash(self):
        # Verify clearing of SF bug #800796
        self.assertRaises(TypeError, hash, slice(5))
        self.assertRaises(TypeError, slice(5).__hash__)

But the second self.assertRaises() doesn't call __hash__. It is successful by accident, because slice(5).__hash__ is None.
msg243677 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-20 15:39
New changeset cbe28273fd8d by Serhiy Storchaka in branch '2.7':
Issue #24134: Use assertRaises() in context manager form in test_slice to
https://hg.python.org/cpython/rev/cbe28273fd8d

New changeset 3a1ee0b5a096 by Serhiy Storchaka in branch '3.4':
Issue #24134: Use assertRaises() in context manager form in test_slice to
https://hg.python.org/cpython/rev/3a1ee0b5a096

New changeset 36c4f8af99da by Serhiy Storchaka in branch 'default':
Issue #24134: Use assertRaises() in context manager form in test_slice to
https://hg.python.org/cpython/rev/36c4f8af99da
msg244740 - (view) Author: Tim Graham (Tim.Graham) * Date: 2015-06-03 12:51
Unfortunately, the revert wasn't merged to the 2.7 branch until after the release of 2.7.10. I guess this regression wouldn't be considered serious enough to warrant a 2.7.11 soon, correct?
msg245923 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-06-29 07:05
Hi, catching up (see my mail to -dev about not getting tracker mail). Deprecations++. Being nice for folk whom consume unittest2 which I backport to everything is important to me :).
History
Date User Action Args
2015-06-29 07:05:18rbcollinssetmessages: + msg245923
2015-06-03 12:51:52Tim.Grahamsetmessages: + msg244740
2015-05-20 15:39:48serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2015-05-20 15:39:10python-devsetmessages: + msg243677
2015-05-16 18:56:58serhiy.storchakasetmessages: + msg243356
2015-05-16 13:33:05python-devsetmessages: + msg243313
2015-05-12 19:21:19Tim.Grahamsetmessages: + msg242996
2015-05-11 07:47:01serhiy.storchakasetfiles: + assert_raises_args_deprecation.patch
2015-05-10 23:23:56r.david.murraysetmessages: + msg242872
2015-05-10 12:42:46serhiy.storchakasetstatus: closed -> open
files: + assert_raises_args.patch
messages: + msg242855

resolution: fixed -> (no value)
stage: resolved -> patch review
2015-05-10 10:19:33Arfreversetnosy: + Arfrever
2015-05-10 07:54:33r.david.murraysetmessages: + msg242852
2015-05-09 23:22:47Tim.Grahamsetnosy: + Tim.Graham
messages: + msg242845
2015-05-06 16:16:05serhiy.storchakasetstatus: open -> closed
messages: + msg242684

assignee: serhiy.storchaka
resolution: fixed
stage: patch review -> resolved
2015-05-06 16:15:19python-devsetnosy: + python-dev
messages: + msg242683
2015-05-06 15:37:01r.david.murraysetmessages: + msg242681
2015-05-06 14:29:49serhiy.storchakasetfiles: + assert_raises_sentinel_2.patch

messages: + msg242675
2015-05-06 13:54:58magnuscsetmessages: + msg242673
2015-05-06 13:12:53r.david.murraysetnosy: + r.david.murray
messages: + msg242665
2015-05-06 12:57:47rhettingersetnosy: + rhettinger
messages: + msg242661
2015-05-06 12:49:16serhiy.storchakasetfiles: + assert_raises_sentinel.patch

versions: + Python 3.4, Python 3.5
keywords: + patch
nosy: + rbcollins, serhiy.storchaka, ezio.melotti, michael.foord

messages: + msg242660
stage: patch review
2015-05-06 12:40:11steven.dapranosetnosy: + steven.daprano
messages: + msg242659
2015-05-06 12:16:57magnusccreate