| msg169596 - (view) |
Author: Daniel Wagner-Hall (daniel.wagner-hall) * |
Date: 2012-09-01 00:25 |
| The following code in a unittest test is a no-op: self.assertRaises(lambda: 1) I would expect this to fail the test, because I naively assumed omitting the exception class would act as: self.assertRaises(BaseException, lambda: 1) verifying that *any* Exception is raised. I believe the correct behaviour is to raise a TypeError if excClass is not a BaseException-derived type, similar to if a non-type is passed as the first arg to issubclass. Attached is a patch to do so. It also removes a no-op self.assertRaises from libimport's tests (because it started failing when I ran the tests with the patch). That assertion is redundant, because the two lines above perform the assertion being attempted. |
|
|
| msg169598 - (view) |
Author: R. David Murray (r.david.murray) *  |
Date: 2012-09-01 00:40 |
| Sounds like a reasonable suggestion. However, the patch is not valid for 2.7, since there exceptions can be old style classes. |
|
|
| msg169599 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-09-01 01:01 |
| I put some review comments in rietveld (you should have gotten an email). |
|
|
| msg169601 - (view) |
Author: Daniel Wagner-Hall (daniel.wagner-hall) * |
Date: 2012-09-01 01:35 |
| I seem to be getting exceptions why trying to upload a new patch to rietveld, either by the web interface (in several browsers), or by upload.py - attaching new patchset here Also, if I wanted to backport to 2.7 including an isinstance(e, types.ClassType) check, how would I go about doing so? Thanks for the quick review! |
|
|
| msg169604 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-09-01 02:08 |
| Uploading the new patch here is the correct procedure. It will automatically be uploaded to rietveld as well. If by "how" you mean how to submit a backport, just create a patch against 2.7 tip and upload it separately. Revised patch looks good. |
|
|
| msg169606 - (view) |
Author: Daniel Wagner-Hall (daniel.wagner-hall) * |
Date: 2012-09-01 02:39 |
| Added patch for 2.7. I'll sign the contributor form just as soon as I can get to a printer. Thanks for taking me through my first contribution. |
|
|
| msg169607 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-09-01 02:42 |
| You are welcome, and thanks for your contribution. |
|
|
| msg169653 - (view) |
Author: Michael Foord (michael.foord) * |
Date: 2012-09-01 17:43 |
| Yep, certainly worth fixing. When 3.3 is out the door I will look at applying this to all branches. |
|
|
| msg169658 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-09-01 18:08 |
| Since it is a bugfix it can be applied at any time now. Checkins to default will end up in 3.3.1 and 3.4. (Only features need to wait until after 3.3 is branched in the main repo.) |
|
|
| msg169763 - (view) |
Author: Daniel Wagner-Hall (daniel.wagner-hall) * |
Date: 2012-09-03 13:30 |
| Cool, my contributor agreement has been received, please merge if happy! |
|
|
| msg169823 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-09-04 11:16 |
| Would using assertRaises to test assertRaises in the tests be to meta? |
|
|
| msg169827 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-09-04 13:07 |
| Ezio: I don't really care whether or not it would be too meta, if you look at the two versions, it is a *lot* clearer what is being tested in the try/except version than it is in the assertRaises version. |
|
|
| msg169830 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-09-04 13:14 |
| I missed the initial patch. What I was thinking about was to use simply with self.assertRaises(TypeError): self.assertRaises(1) instead of: + ctx = self.assertRaises(TypeError) + with ctx: + self.assertRaises(1) + self.assertIsInstance(ctx.exception, TypeError) or + try: + self.assertRaises(1) + self.fail("Expected TypeError") + except TypeError: + pass Unless I'm missing something, all these should be equivalent. You could even use assertRaisesRegex to check the error message if you think that's appropriate. |
|
|
| msg170625 - (view) |
Author: Daniel Wagner-Hall (daniel.wagner-hall) * |
Date: 2012-09-17 20:40 |
| Is anything blocking this patch's submission? |
|
|
| msg170655 - (view) |
Author: Michael Foord (michael.foord) * |
Date: 2012-09-18 16:07 |
| The patch is just waiting for me to look over it and commit. I'll get to it ASAP. |
|
|
| msg220560 - (view) |
Author: PCManticore (Claudiu.Popa) *  |
Date: 2014-06-14 15:05 |
| This seems to be a reasonable fix. Michael, could you have a look at this patch, please? |
|
|
| msg221849 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2014-06-29 15:32 |
| Ezio requested I comment on his suggestion: I still prefer the try/except form, but I don't feel strongly about it. |
|
|
| msg238822 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-03-21 17:53 |
| I'm +0.5 for the variant suggested by Berker and Ezio. Do you have time to look at the patch Michael? I could commit modified patch (there is one defect in tests). |
|
|
| msg238999 - (view) |
Author: Michael Foord (michael.foord) * |
Date: 2015-03-23 09:40 |
| I like the first variant suggested by Ezio as more concise. I'll try and look at the substance of the patch today. |
|
|
| msg239584 - (view) |
Author: Michael Foord (michael.foord) * |
Date: 2015-03-30 08:52 |
| The change to unittest is fine. I'd prefer the tests tweaking as Ezio suggested. |
|
|
| msg243331 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-05-16 16:27 |
| Ping. |
|
|
| msg243335 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-05-16 17:10 |
| Since the patch has been reviewed by several core developers, I think you can go ahead and commit it. I'm +0 on the 2.7 version of the patch (the isinstance(e, types.ClassType) part looks fine, but I haven't tested it). It's probably not worth to change anything on the 2.7 branch now :) |
|
|
| msg243567 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-05-19 09:14 |
| Core developers left a couple of notes and the patch itself is outdated. Here is updated patch that addresses all comments. It also extends the checking to assertRaisesRegex(), assertWarns() and assertWarnsRegex(). There is a risk to break existing incorrect tests if the argument is a tuple. They can be passed for now because caught exception or warning is found before incorrect value. For example: with self.assertRaises((ValueError, None)): int('a') |
|
|
| msg243574 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2015-05-19 10:45 |
| I posted a question on Reitveld, but the new patch looks fine in general. I wouldn’t worry too much about the (ValueError, None) case, since such code is probably already broken. If it is a problem though, maybe this could only go into the next feature relase (3.5). |
|
|
| msg243765 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-05-21 17:16 |
| New changeset 84d7ec21cc43 by Serhiy Storchaka in branch 'default': Issue #15836: assertRaises(), assertRaisesRegex(), assertWarns() and https://hg.python.org/cpython/rev/84d7ec21cc43 |
|
|
| msg243766 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-05-21 17:20 |
| Applied to 3.5 only, because this issue looks rather as new feature (preventing possible user error) and there is minimal chance to break existing tests (see ). |
|
|
| msg243770 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-05-21 17:57 |
| Thank you for your contribution Daniel. |
|
|