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

pytest should use the inspect.signature algorithm to determine fixtures to use #2267

Closed
anntzer opened this issue Feb 19, 2017 · 8 comments
Closed
Labels
type: feature-branch new feature or API change, should be merged into features branch type: refactoring internal improvements to the code

Comments

@anntzer
Copy link
Contributor

anntzer commented Feb 19, 2017

pytest 3.0.6 on python 3.6, Arch Linux (distro packages).

Currently, pytest follows the entire __wrapped__ chain and uses low-level code introspection to determine the signature of the test function, and thus the fixtures to use (https://github.com/pytest-dev/pytest/blob/master/_pytest/compat.py#L84). It would be preferable if it used the algorithm of inspect.signature (follow the __wrapped__ chain until either reaching an object with a __signature__ attribute (use the attribute in that case), or the bottom of the chain) instead.

This would allow things such as

import functools
import inspect
import pytest


def decorator(func):
    @functools.wraps(func)
    def tester(x):
        assert func()
    tester.__signature__ = inspect.signature(tester, follow_wrapped=False)
    print(inspect.signature(tester))  # prints "(x)"
    return tester


@pytest.mark.parametrize("x", [1])
@decorator
def test_1(*args):
    return args

(i.e., where the wrapper code calls the wrapped function but does the asserts on its return value) to work (currently, this fails at collection time with ValueError: <function test_1 at 0x7fbb804841e0> uses no argument 'x').

An example use case of this pattern is matplotlib's image comparison test suite, which looks like

@image_comparison([image_filenames])
def test_foo():
    # plotting commands

which runs some plotting commands and then compares the results to the image files specified by the image_comparison decorator.

@flub
Copy link
Member

flub commented Feb 20, 2017

I think it is fair enough to use the same algorithm as inspect.signature in recent python versions. I think the reason pytest does it's own low-level stuff currently is because inspect.signature used to not do enough on old versions, so it would still need to code this itself. There's a small chance that it would break someones existing tests, so I guess this should go on the features branch.

Would you be up to creating a pull request with the required changes?

@anntzer
Copy link
Contributor Author

anntzer commented Feb 20, 2017

Would a dependency on funcsigs (https://pypi.python.org/pypi/funcsigs) be acceptable? I don't want to get into the vendoring debate (#944)...

@flub
Copy link
Member

flub commented Feb 24, 2017

Historically we have a general tendency to try and keep the number of dependencies low and even then only on packages controlled by pytest-dev. It sounds like we have most of the logic already so I think it'd be preferable to try to modify it - after all we don't need full PEP 362 support but only need to do the right thing for looking up required fixtures. If this turns out to be really hard or a minefield for some reason I guess we could consider an external dependency.

@nicoddemus nicoddemus added the type: refactoring internal improvements to the code label Apr 7, 2017
@ceridwen
Copy link
Contributor

I have a use-case where I'd like to use __signature__ to define functions so that I can control which fixtures get used without having to create my own code objects. I'd be willing to write this patch, but I don't think it would be possible to reasonably avoid using funcsigs. The new signature code introduces several new kinds of objects and handles a number of corner cases that pytest currently doesn't.

@ceridwen
Copy link
Contributor

Can I get a yes or no about whether you would accept a patch using funcsigs, either as an import or by vendoring? I don't want to spend the time to reimplement funcsigs, and note that reimplementing an open-source implementation would probably count as a derivative work, which means the licensing issues are the same regardless. If the answer is no or, "No one can decide whether to add a dependency for another three months," I need to go ahead and work around this problem.

@RonnyPfannschmidt
Copy link
Member

yes

@nicoddemus
Copy link
Member

@ceridwen I second that. I believe we are changing the development philosophy to allow new Python only dependencies if they will help us improve maintenance of the code base.

I believe pytest has been shifting this approach because nowadays the tooling for installing Python packages is very mature, which was not so much the case a few years back.

@ceridwen please make sure to target the features branch.

@nicoddemus nicoddemus added the type: feature-branch new feature or API change, should be merged into features branch label Oct 10, 2017
@nicoddemus
Copy link
Member

Fixed by #2842

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature-branch new feature or API change, should be merged into features branch type: refactoring internal improvements to the code
Projects
None yet
Development

No branches or pull requests

5 participants