-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 invalid regex handling in filterwarnings #13124
base: main
Are you sure you want to change the base?
Fixed invalid regex handling in filterwarnings #13124
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Definitely much better. New output (with my small suggestion added):
ERROR: while parsing the following warning configuration:
ignore::DeprecationWarning:*
This error occurred:
Invalid regex '*': nothing to repeat at position 0
…com/virendrapatil24/pytest into fix-invalid-regex-in-filterwarnings
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
testing/test_warnings.py
Outdated
result = pytester.runpytest_subprocess() | ||
result.stderr.fnmatch_lines( | ||
[ | ||
"*Invalid regex '*': nothing to repeat at position 0*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more thorough to test on the whole "while parsing ..." output?
it looks like there's no tests for "invalid lineno (negative AND not an integer)", too many fields, invalid action or resolving the warning category perhaps you'd be interested in adding these tests? Should be very similar to test_invalid_regex_in_filterwarning perhaps in this PR or perhaps in a followup? |
Thanks for the review! I’m a bit confused about what else is required for this PR. Could you summarize the key changes you’d like to see here? From my understanding, the missing tests you mentioned might be better suited for a follow-up rather than this ticket. Let me know your thoughts! |
So yeah could you:
|
sure will make those asserts, thanks for clarifying. |
testing/test_warnings.py
Outdated
"", | ||
"This error occurred:", | ||
"", | ||
"Invalid regex '*': nothing to repeat at position 0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to escape this star or it will treat it as a wildcard/fnmatch syntax. I'm not sure how to escape it though
"Invalid regex '*': nothing to repeat at position 0", | |
r"Invalid regex '\*': nothing to repeat at position 0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, I will check what I can do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can escape * using [*]
ref: https://docs.python.org/3/library/fnmatch.html
closes #13119
parse_warning_filter
method to handle invalid regex pattern provided to message/module args.