unittest.assert*Regex functions should verify that expected_regex has a valid type

[themulhern]

BPO	20145
Nosy	@pitrou, @ezio-melotti, @bitdancer, @voidspace, @ethanfurman
Files	validate_regex.patch: patch to check expected_regex validate_regex_improved.patch: patch to check expected_regex improved

^{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 = None
closed_at = <Date 2014-03-23.19:50:08.935>
created_at = <Date 2014-01-06.14:28:17.040>
labels = ['easy', 'type-feature']
title = 'unittest.assert*Regex functions should verify that expected_regex has a valid type'
updated_at = <Date 2014-03-25.19:35:36.693>
user = 'https://bugs.python.org/themulhern'

bugs.python.org fields:

activity = <Date 2014-03-25.19:35:36.693>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2014-03-23.19:50:08.935>
closer = 'r.david.murray'
components = []
creation = <Date 2014-01-06.14:28:17.040>
creator = 'the.mulhern'
dependencies = []
files = ['34346', '34364']
hgrepos = []
issue_num = 20145
keywords = ['patch', 'easy']
message_count = 14.0
messages = ['207436', '210804', '210828', '211417', '212923', '212944', '212961', '212963', '213110', '213186', '213254', '214628', '214629', '214846']
nosy_count = 8.0
nosy_names = ['pitrou', 'ezio.melotti', 'r.david.murray', 'michael.foord', 'ethan.furman', 'python-dev', 'the.mulhern', 'kamie']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue20145'
versions = ['Python 3.5']

[themulhern]

A normal thing for a developer to do is to convert a use of an assert* function to a use of an assert*Regex function and foolishly forget to actually specify the expected regular expression.

If they do this, the test will always pass because the callable expression will be in the place of the expected regular expression and the callable expression will, therefore, be None. It would be nice if in the constructor in AssertRaisesBaseContext (for 3.5) not only was the expected regex converted to a regex (if actually a string or bytes) but if it's not if it were checked if it were a valid regular expression object and an exception raised if this is not the case.

In the current version of AssertRaisesBaseContext.handle the comments say that if the callable object is None then the function is being used as a context manager. Not always the case, alas, perhaps the developer just forgot to add that necessary regular expression before the callable object.

[ezio-melotti]

The two signatures of assertRaisesRegex are:
assertRaisesRegex(exception, regex, callable, *args, **kwds)
assertRaisesRegex(exception, regex, msg=None)

IIUC what you are saying is that if you forget the regex and call assertRaisesRegex(exception, callable) the test will pass.

I think it would be ok to add a check to see if the second argument is a callable or None (or maybe check if it's a string or regex object?), and give an appropriate error message if it's not.

[voidspace]

Agreed.

[themulhern]

Yes. I'ld check if it was a string or a regex object...there is already code that converts the string to a regular expression in there.

[kamie]

Just to be sure, the check must be implemented inside the assertRaisesRegex method, right?

[voidspace]

That's correct.

[pitrou]

A simple way of checking is to actually re.compile(regex), I think. It should automatically raise TypeError on invalid input.

[bitdancer]

Heh. If unittest had used duck typing instead of isinstance for its string-to-regex conversion check in the first place, this issue wouldn't even have come up.

[kamie]

I've implemented a piece of code to check if the expected_regex is a string or regex object. If it's not it raises a TypeError exception. The check is inside the __init__ method of the _AssertRaisesBaseContext class so it will always check the expected_regex in all methods that use the _AssertRaises.

I've added tests too. They are verifying the assertRaisesRegex and the assertWarnsRegex methods.

[kamie]

Applying changes as suggested by R. David Murray in the Core-mentorship e-mail list.

Instead of doing the if tests I've replaced the existing

   if isinstance(expected_regex, (bytes, str)):

by

   if expected_regex is not None:

And also made a change in one of the tests because I figure out it was wrong.

[themulhern]

Thanks, I'ld definitely be _much_ happier w/ a TypeError than with silent success.

[python-dev]

New changeset ec556e45641a by R David Murray in branch 'default':
bpo-20145: assert[Raises|Warns]Regex now raise TypeError on bad regex.
http://hg.python.org/cpython/rev/ec556e45641a

[bitdancer]

Thanks, Kammie. I removed the extra whitespace from your fix and simplified the tests a bit.

[python-dev]

New changeset 3f8b801e7e76 by R David Murray in branch '2.7':
backport: bpo-20145: assertRaisesRegexp now raises a TypeError on bad regex.
http://hg.python.org/cpython/rev/3f8b801e7e76

New changeset 32407a677215 by R David Murray in branch '3.4':
backport: bpo-20145: assert[Raises|Warns]Regex now raise TypeError on bad regex.
http://hg.python.org/cpython/rev/32407a677215

New changeset 8f72f8359987 by R David Murray in branch 'default':
Merge bpo-20145 backport: delete whatsnew entry.
http://hg.python.org/cpython/rev/8f72f8359987