Fix for warning test arguments with default values #13044
Conversation
huangbenny
commented
Dec 9, 2024
huangbenny
commented
- Closes Warn on test arguments with default values? #12693
src/_pytest/python.py
Outdated
| + 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.
src/_pytest/python.py
Outdated
| @@ -143,9 +144,29 @@ def async_fail(nodeid: str) -> None: | |||
| 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.
src/_pytest/python.py
Outdated
| @@ -143,9 +144,29 @@ def async_fail(nodeid: str) -> None: | |||
| fail(msg, pytrace=False) | |||
|
|
|||
|
|
|||
| 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.
src/_pytest/python.py
Outdated
| + 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?
| warnings.simplefilter("always", PytestDefaultArgumentWarning) | ||
| warnings.warn(PytestDefaultArgumentWarning(msg)) | ||
|
|
||
|
|
||
| @hookimpl(trylast=True) | ||
| def pytest_pyfunc_call(pyfuncitem: Function) -> object | None: |
There was a problem hiding this comment.
I'm not entirely convinced this is the right place for this. I wonder if something like FixtureManager.getfixtureinfo (where we already take care of reading the function's arguments) would make more sense maybe?
There was a problem hiding this comment.
In my most recent commit, I updated the warning checks to be in the getfixtureinfo. After looking into the function more, I also thought that it would be better to include the checks in that function.
…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
