unittest assertRaises should verify excClass is actually a BaseException class

[danielwagner-hall]

BPO	15836
Nosy	@ezio-melotti, @bitdancer, @voidspace, @PCManticore, @berkerpeksag, @vadmium, @serhiy-storchaka
Files	assertRaises.patch assertRaises.patch issue15836-2.7.patch issue15836_2.patch

^{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-21.17:57:14.747>
created_at = <Date 2012-09-01.00:25:57.186>
labels = ['type-feature', 'library']
title = 'unittest assertRaises should verify excClass is actually a BaseException class'
updated_at = <Date 2015-05-21.17:57:46.866>
user = 'https://bugs.python.org/danielwagner-hall'

bugs.python.org fields:

activity = <Date 2015-05-21.17:57:46.866>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-05-21.17:57:14.747>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2012-09-01.00:25:57.186>
creator = 'daniel.wagner-hall'
dependencies = []
files = ['27079', '27080', '27081', '39426']
hgrepos = []
issue_num = 15836
keywords = ['patch']
message_count = 27.0
messages = ['169596', '169598', '169599', '169601', '169604', '169606', '169607', '169653', '169658', '169763', '169823', '169827', '169830', '170625', '170655', '220560', '221849', '238822', '238999', '239584', '243331', '243335', '243567', '243574', '243765', '243766', '243770']
nosy_count = 9.0
nosy_names = ['ezio.melotti', 'r.david.murray', 'michael.foord', 'Claudiu.Popa', 'python-dev', 'berker.peksag', 'martin.panter', 'daniel.wagner-hall', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue15836'
versions = ['Python 3.5']

[danielwagner-hall]

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.

[bitdancer]

Sounds like a reasonable suggestion. However, the patch is not valid for 2.7, since there exceptions can be old style classes.

[bitdancer]

I put some review comments in rietveld (you should have gotten an email).

[danielwagner-hall]

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!

[bitdancer]

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.

[danielwagner-hall]

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.

[bitdancer]

You are welcome, and thanks for your contribution.

[voidspace]

Yep, certainly worth fixing. When 3.3 is out the door I will look at applying this to all branches.

[bitdancer]

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

[danielwagner-hall]

Cool, my contributor agreement has been received, please merge if happy!

[ezio-melotti]

Would using assertRaises to test assertRaises in the tests be to meta?