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

Fixed handling of invalid regex pattern #12526

Merged
merged 14 commits into from
Jul 2, 2024

Conversation

virendrapatil24
Copy link
Contributor

@virendrapatil24 virendrapatil24 commented Jun 23, 2024

closes #12505

  • Updated the RaisesContext class in src/_pytest/python_api.py to catch re.error and call pytest.fail with a clear error message.
  • Added a test case in testing/python/raises.py to validate the new behavior.
  • Created a changelog fragment to document this improvement.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jun 23, 2024
@virendrapatil24 virendrapatil24 marked this pull request as ready for review June 24, 2024 04:43
@The-Compiler
Copy link
Member

The-Compiler commented Jun 25, 2024

This definitely makes things a bit nicer:

test_raises.py:3: in <module>
    raise ValueError()
E   ValueError

During handling of the above exception, another exception occurred:
/usr/lib/python3.12/re/__init__.py:177: in search
    return _compile(pattern, flags).search(string)
/usr/lib/python3.12/re/__init__.py:307: in _compile
    p = _compiler.compile(pattern, flags)
/usr/lib/python3.12/re/_compiler.py:745: in compile
    p = _parser.parse(p, flags)
/usr/lib/python3.12/re/_parser.py:979: in parse
    p = _parse_sub(source, state, flags & SRE_FLAG_VERBOSE, 0)
/usr/lib/python3.12/re/_parser.py:460: in _parse_sub
    itemsappend(_parse(source, state, verbose, nested + 1,
/usr/lib/python3.12/re/_parser.py:568: in _parse
    raise source.error("unterminated character set",
E   re.error: unterminated character set at position 24

During handling of the above exception, another exception occurred:
test_raises.py:2: in <module>
    with pytest.raises(ValueError, match="invalid regex character ["):
E   Failed: Invalid regex pattern provided to 'match': unterminated character set at position 24

But it's still pretty verbose due to the chaining... This could be improved by using raise fail.Exception(...) from None:

test_raises.py:2: in <module>
    with pytest.raises(ValueError, match="invalid regex character ["):
E   Failed: Invalid regex pattern provided to 'match': unterminated character set at position 24

though we don't really do this kind of thing elsewhere so far. I'd wait to see what others think before proceeding with that.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvement.

The proposed solution of catching the exception in __exit__ has the disadvantage that the fail is raised from the __exit__ (and is thus chained to the exception under test), and is not checked at all in the "did not raise" case. I therefore think it'd be better to check it eagerly in the __init__.

I also agree with @The-Compiler. We can use fail.Exception, but another way is to just make sure to call fail outside of the except block, something like:

re_error = None
try:
    self.excinfo.match(self.match_expr)
except re.error as e:
    re_error = e
if re_error is not None:               
    fail(f"Invalid regex pattern provided to 'match': {e}")

a little verbose, but OK.

src/_pytest/python_api.py Outdated Show resolved Hide resolved
changelog/12505.bugfix.rst Outdated Show resolved Hide resolved
@virendrapatil24
Copy link
Contributor Author

I have added the changes you asked for, please let me know if anything else needs to be done. Thanks for the input!

@bluetech
Copy link
Member

@virendrapatil24 The proposed solution of catching the exception in __exit__ has the disadvantage that the fail is raised from the __exit__ (and is thus chained to the exception under test), and is not checked at all in the "did not raise" case. I therefore think it'd be better to check it eagerly in the __init__.

@virendrapatil24
Copy link
Contributor Author

@bluetech So I need to compile the regex pattern in the __init__ itself to check it for the valid pattern. Let me know if I have gotten this right.

@bluetech
Copy link
Member

Right, that would be a good way to do it.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me!

@bluetech bluetech merged commit 49bb5c8 into pytest-dev:main Jul 2, 2024
29 checks passed
Glyphack pushed a commit to Glyphack/pytest that referenced this pull request Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pytest.raises with invalid regex in match kwarg fails with internal re.error
3 participants