| 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 for returning the actual exception. |
|
|
| msg242659 - (view) |
Author: Steven D'Aprano (steven.daprano) *  |
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) * |
Date: 2015-05-06 12:49 |
| Possible solution is to use special sentinel instead of None. |
|
|
| msg242661 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2015-05-06 12:57 |
| The patch looks like a nice improvement. |
|
|
| msg242665 - (view) |
Author: R. David Murray (r.david.murray) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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 :). |
|
|