assertRaises can behave differently

[magnusc]

BPO	24134
Nosy	@rhettinger, @rbtcollins, @ezio-melotti, @stevendaprano, @bitdancer, @voidspace, @serhiy-storchaka, @timgraham
Files	assert_raises_sentinel.patch assert_raises_sentinel_2.patch assert_raises_args.patch: Raises TypeError assert_raises_args_deprecation.patch: Emits DeprecationWarning

^{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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2015-05-20.15:39:48.316>
created_at = <Date 2015-05-06.12:16:57.869>
labels = ['type-bug', 'library']
title = 'assertRaises can behave differently'
updated_at = <Date 2015-06-29.07:05:18.512>
user = 'https://bugs.python.org/magnusc'

bugs.python.org fields:

activity = <Date 2015-06-29.07:05:18.512>
actor = 'rbcollins'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-05-20.15:39:48.316>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2015-05-06.12:16:57.869>
creator = 'magnusc'
dependencies = []
files = ['39305', '39306', '39334', '39340']
hgrepos = []
issue_num = 24134
keywords = ['patch']
message_count = 20.0
messages = ['242658', '242659', '242660', '242661', '242665', '242673', '242675', '242681', '242683', '242684', '242845', '242852', '242855', '242872', '242996', '243313', '243356', '243677', '244740', '245923']
nosy_count = 11.0
nosy_names = ['rhettinger', 'rbcollins', 'ezio.melotti', 'Arfrever', 'steven.daprano', 'r.david.murray', 'michael.foord', 'python-dev', 'serhiy.storchaka', 'Tim.Graham', 'magnusc']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24134'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

[magnusc]

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 bpo-9587 for returning the actual exception.

[stevendaprano]

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.

[serhiy-storchaka]

Possible solution is to use special sentinel instead of None.

[rhettinger]

The patch looks like a nice improvement.

[bitdancer]

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.

[magnusc]

"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

[serhiy-storchaka]

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.

[bitdancer]

Made a couple of review comments on the English in the test comments.

Patch looks good to me.

[python-dev]

New changeset 111ec3d5bf19 by Serhiy Storchaka in branch '2.7':
Issue bpo-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 bpo-24134: assertRaises(), assertRaisesRegex(), assertWarns() and
https://hg.python.org/cpython/rev/5418ab3e5556

New changeset 679b5439b9a1 by Serhiy Storchaka in branch 'default':
Issue bpo-24134: assertRaises(), assertRaisesRegex(), assertWarns() and
https://hg.python.org/cpython/rev/679b5439b9a1

[serhiy-storchaka]

Thank you David for your corrections.

[timgraham]

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 django/django#4637.