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?

[bitdancer]

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.

[ezio-melotti]

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.

[danielwagner-hall]

Is anything blocking this patch's submission?

[voidspace]

The patch is just waiting for me to look over it and commit. I'll get to it ASAP.

[PCManticore]

This seems to be a reasonable fix. Michael, could you have a look at this patch, please?

[bitdancer]

Ezio requested I comment on his suggestion: I still prefer the try/except form, but I don't feel strongly about it.

[serhiy-storchaka]

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

[voidspace]

I like the first variant suggested by Ezio as more concise. I'll try and look at the substance of the patch today.

[voidspace]

The change to unittest is fine. I'd prefer the tests tweaking as Ezio suggested.

[serhiy-storchaka]

Ping.

[berkerpeksag]

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

[serhiy-storchaka]

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

[vadmium]

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

[python-dev]

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

[serhiy-storchaka]

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

[serhiy-storchaka]

Thank you for your contribution Daniel.