Fix for warning test arguments with default values#13044
Fix for warning test arguments with default values#13044huangbenny wants to merge 3 commits intopytest-dev:mainfrom
Conversation
huangbenny
commented
Dec 9, 2024
huangbenny
commented
- Closes Warn on test arguments with default values? #12693
| + str(param.default) | ||
| + "'.\n" | ||
| ) | ||
| warnings.simplefilter("always", PytestDefaultArgumentWarning) |
There was a problem hiding this comment.
Is adding a preexisting filter again a noop? Else this is a problem
There was a problem hiding this comment.
Even if it is, it seems odd for pytest to programmatically override the warning filters on its own. What's the reason for this?
There was a problem hiding this comment.
Previously without this filter, the warnings would include a trackback to the line that made the warning. This is not the desired behavior in my opinion because I intended for this warning to help the user pinpoint the line number for the function that used the default parameters.
The-Compiler
left a comment
The-Compiler
left a comment
There was a problem hiding this comment.
Thanks for getting the ball rolling! I added a couple of comments, and this will also need a test somewhere.
| fail(msg, pytrace=False) | ||
|
|
||
|
|
||
| def async_default_arg_warn(nodeid: str, function_name, param) -> None: |
There was a problem hiding this comment.
Why async_? What's async about it?
There was a problem hiding this comment.
I agree that I shouldn't have added async_ to the function name. I'll update that.
|
|
||
|
|
||
| def async_default_arg_warn(nodeid: str, function_name, param) -> None: | ||
| msg = ( |
There was a problem hiding this comment.
This would be much more readable with f-strings rather than string concatenation.
There was a problem hiding this comment.
Sounds good, I'll change it to f-strings for more readability.
| + str(param.default) | ||
| + "'.\n" | ||
| ) | ||
| warnings.simplefilter("always", PytestDefaultArgumentWarning) |
There was a problem hiding this comment.
Even if it is, it seems odd for pytest to programmatically override the warning filters on its own. What's the reason for this?
…ssage from using string concat to f-strings for more readability, and added a function in compat to grab default parameters and its values
huangbenny
force-pushed
the
warning_test_args
branch
from
3405966 to
8b4bd3a
Compare
| msg = ( | ||
| f"Test function '{function_name}' has a default argument '{param_name}={param_val}'.\n" | ||
| ) | ||
| warnings.simplefilter("always", PytestDefaultArgumentWarning) |
There was a problem hiding this comment.
I still think this isn't a good idea, what's the rationale for it? With it, a
pytestwarnings =
...
ignore::_pytest.warning_types.PytestDefaultArgumentWarningin pytest's configuration would have no effect at all...
(Also, the type should probably be exposed as pytest.PytestDefaultArgumentWarning just like pytest.PytestUnknownMarkWarning is?
4 participants
