Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
themulhern mannequin opened this issue Jan 6, 2014 · 14 comments
Closed

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

themulhern mannequin opened this issue Jan 6, 2014 · 14 comments
Labels
easy type-feature A feature request or enhancement

Comments

@themulhern
Copy link
Mannequin

themulhern mannequin commented Jan 6, 2014

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
    Copy link
    Mannequin Author

    themulhern mannequin commented Jan 6, 2014

    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.

    @themulhern themulhern mannequin added the type-bug An unexpected behavior, bug, or error label Jan 6, 2014
    @terryjreedy terryjreedy added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jan 11, 2014
    @ezio-melotti
    Copy link
    Member

    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
    Copy link
    Contributor

    Agreed.

    @themulhern
    Copy link
    Mannequin Author

    themulhern mannequin commented Feb 17, 2014

    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
    Copy link
    Mannequin

    kamie mannequin commented Mar 8, 2014

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

    @voidspace
    Copy link
    Contributor

    That's correct.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 9, 2014

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

    @bitdancer
    Copy link
    Member

    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
    Copy link
    Mannequin

    kamie mannequin commented Mar 11, 2014

    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
    Copy link
    Mannequin

    kamie mannequin commented Mar 11, 2014

    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
    Copy link
    Mannequin Author

    themulhern mannequin commented Mar 12, 2014

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 23, 2014

    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
    Copy link
    Member

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 25, 2014

    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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    easy type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants