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.

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.

Possible solution is to use special sentinel instead of None.

The patch looks like a nice improvement.

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.

"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

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.

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

Patch looks good to me.

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

Thank you David for your corrections.

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.

Given one failure there are probably more. So we should probably back this out of 2.7 and 3.4.

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.

Yeah, the general case of wrappers is something I hadn't considered.

Probably we should go the deprecation route. Robert, what's your opinion?

I didn't find any problems while testing your proposed new patch for cpython and your proposed patch for Django together.

New changeset 0c93868f202e by Serhiy Storchaka in branch '2.7':
Reverted issue bpo-24134 changes.
https://hg.python.org/cpython/rev/0c93868f202e

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

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

Interesting, the last patch exposed a flaw in test_slice.

    def test_hash(self):
        # Verify clearing of SF bug python/cpython#39183
        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.

New changeset cbe28273fd8d by Serhiy Storchaka in branch '2.7':
Issue bpo-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 bpo-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 bpo-24134: Use assertRaises() in context manager form in test_slice to
https://hg.python.org/cpython/rev/36c4f8af99da

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?

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 :).