classification
Title: unittest assertRaises should verify excClass is actually a BaseException class
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Claudiu.Popa, berker.peksag, daniel.wagner-hall, ezio.melotti, martin.panter, michael.foord, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2012-09-01 00:25 by daniel.wagner-hall, last changed 2015-05-21 17:57 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
assertRaises.patch daniel.wagner-hall, 2012-09-01 00:25 review
assertRaises.patch daniel.wagner-hall, 2012-09-01 01:35 review
issue15836-2.7.patch daniel.wagner-hall, 2012-09-01 02:39 review
issue15836_2.patch serhiy.storchaka, 2015-05-19 09:14 review
Messages (27)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2012-09-01 02:42
You are welcome, and thanks for your contribution.
msg169653 - (view) Author: Michael Foord (michael.foord) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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: Claudiu Popa (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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2015-05-16 16:27
Ping.
msg243335 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 issue24134).
msg243770 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-21 17:57
Thank you for your contribution Daniel.
History
Date User Action Args
2015-05-21 17:57:46serhiy.storchakasetmessages: + msg243770
2015-05-21 17:57:14serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2015-05-21 17:20:02serhiy.storchakasettype: behavior -> enhancement
messages: + msg243766
versions: - Python 2.7, Python 3.4
2015-05-21 17:16:18python-devsetnosy: + python-dev
messages: + msg243765
2015-05-21 16:26:51serhiy.storchakasetassignee: michael.foord -> serhiy.storchaka
versions: + Python 2.7
2015-05-19 10:45:38martin.pantersetmessages: + msg243574
2015-05-19 09:14:28serhiy.storchakasetfiles: + issue15836_2.patch

messages: + msg243567
2015-05-16 17:10:03berker.peksagsetmessages: + msg243335
2015-05-16 16:27:57serhiy.storchakasetmessages: + msg243331
2015-03-30 08:52:21michael.foordsetmessages: + msg239584
2015-03-23 09:40:55michael.foordsetmessages: + msg238999
2015-03-21 19:39:07rhettingersetversions: - Python 2.7
2015-03-21 17:53:35serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg238822
2015-01-13 01:17:00berker.peksagsetnosy: + berker.peksag
stage: patch review -> commit review

versions: + Python 3.4
2015-01-13 00:29:11martin.pantersetnosy: + martin.panter
2014-06-29 15:32:33r.david.murraysetmessages: + msg221849
2014-06-14 15:05:48Claudiu.Popasetnosy: + Claudiu.Popa

messages: + msg220560
versions: + Python 3.5, - Python 3.2, Python 3.3, Python 3.4
2012-09-18 16:07:54michael.foordsetmessages: + msg170655
2012-09-17 20:40:06daniel.wagner-hallsetmessages: + msg170625
2012-09-04 13:14:54ezio.melottisetmessages: + msg169830
2012-09-04 13:07:54r.david.murraysetmessages: + msg169827
2012-09-04 11:16:50ezio.melottisetassignee: michael.foord
messages: + msg169823
2012-09-03 13:30:59daniel.wagner-hallsetmessages: + msg169763
2012-09-01 18:08:15r.david.murraysetmessages: + msg169658
2012-09-01 17:44:44ezio.melottisetnosy: + ezio.melotti
2012-09-01 17:43:52michael.foordsetmessages: + msg169653
2012-09-01 02:42:32r.david.murraysetmessages: + msg169607
components: + Library (Lib), - Tests
2012-09-01 02:39:14daniel.wagner-hallsetfiles: + issue15836-2.7.patch

messages: + msg169606
versions: + Python 2.7
2012-09-01 02:08:23r.david.murraysetnosy: + michael.foord
2012-09-01 02:08:12r.david.murraysetmessages: + msg169604
2012-09-01 01:35:35daniel.wagner-hallsetfiles: + assertRaises.patch

messages: + msg169601
2012-09-01 01:01:49r.david.murraysetmessages: + msg169599
2012-09-01 00:40:28r.david.murraysetversions: - Python 2.6, Python 3.1, Python 2.7
nosy: + r.david.murray

messages: + msg169598

stage: patch review
2012-09-01 00:25:57daniel.wagner-hallcreate