Allow tests declared as @staticmethod to use fixtures #2700
Conversation
nicoddemus
force-pushed
the
staticmethods-fixtures
branch
from
16b248f
to
a993add
Compare
_pytest/compat.py
Outdated
| @@ -82,7 +82,10 @@ def num_mock_patch_args(function): | |||
| return len(patchings) | |||
|
|
|||
|
|
|||
| def getfuncargnames(function, startindex=None): | |||
| def getfuncargnames(function, startindex=None, cls=None): | |||
| if startindex is None and cls is not None: | |||
There was a problem hiding this comment.
this bit should be documented as a hack and factored out when revisiting fixtures
There was a problem hiding this comment.
Why is it a hack? It is not entirely clear to me why it this is there and I would like to document it properly then. 😉
There was a problem hiding this comment.
it moves a new responsibility into getfuncargnames, and fills the code with a lot of conditionals that are strictly not needed at that place and interaction
while reviewing i noticed that this also creates a dependence on how things are named, it will work incorrectly in some cases
There was a problem hiding this comment.
it moves a new responsibility into getfuncargnames, and fills the code with a lot of conditionals that are strictly not needed at that place and interaction
Oh I see. I decided to move cls here so that callers didn't have to handle known that cls might affect the results of this call themselves; the previous code was splitting that responsibility between two places, getfixtureinfo in _pytest/fixtures.py and here (see the diff). IMHO if getfuncargnames also needs a cls argument to figure things out, it should receive it and not require callers to handle that special case themselves.
while reviewing i noticed that this also creates a dependence on how things are named, it will work incorrectly in some cases
Which things? The function name or the argument names?
There was a problem hiding this comment.
@nicoddemus from my pov the fixture mechanism should ask the node for the requested fixtures, not extract it somewhere down the line from the function object, in fact it should never even see the function object simply to separate concerns -
There was a problem hiding this comment.
I see thanks. But from my understanding this is quite a refactor on how fixture names are obtained and out of scope of this PR.
This PR attempts to fix fixture support for static methods and doesn't add complexity to the current system IMHO. If you disagree we should close this PR, which I'm OK with.
There was a problem hiding this comment.
im fine with the pr, i just want to see a note there detailing what is made worse to get it done
| @@ -957,11 +957,7 @@ def __init__(self, session): | |||
|
|
|||
| def getfixtureinfo(self, node, func, cls, funcargs=True): | |||
| if funcargs and not hasattr(node, "nofuncargs"): | |||
| if cls is not None: | |||
There was a problem hiding this comment.
Here's what I mean about the "split" responsibility.
Note as well that getfuncargnames is an internal utility function used in two places in the code base, it might as well make things simpler for callers to avoid mistakes and keep the calling code clean (as it does here, IMHO).
|
@RonnyPfannschmidt added the comment like you asked, let me know if you would like to add more information. 👍 |
|
thanks for the followup yay 👍 |
Fix #2699