Use funcsigs and inspect.signature to do function argument analysis

[ceridwen]

This is not finished. I'm opening the PR now because I need to ask questions about implementation details.

The specific detail I'm looking at right now is that getfuncargnames's handling of functions with arguments with defaults or keyword-only arguments and arguments bound with functools.partial is distinctly... odd, and arguably buggy. Here's an example of functools.partial without any Python 3 features:

>>> def f0(b, c=0, *a, **k): print(b, c, a, d, k)
...
>>> g0 = functools.partial(f0, 2)
>>> def h0(b=2, c=0, *a, **k): print(b, c, a, k)
>>> _pytest.compat.getfuncargnames(f0)
('b',)
>>> _pytest.compat.getfuncargnames(g0)
('c',)
>>> _pytest.compat.getfuncargnames(h0)
()
>>> inspect.signature(f0)
<Signature (b, c=0, *a, **k)>
>>> inspect.signature(g0)
<Signature (c=0, *a, **k)>
>>> inspect.signature(h0)
<Signature (b=2, c=0, *a, **k)>

If I understand the use of getfuncargnames in fixtures.py correctly, it's to determine which arguments aren't bound and thus are available for substitution with the return values of fixtures. In this case, pytest would call f0 with the fixture b but not c, even if c is available. However, it would call g0, the partially bound version of that same test function, with c if c is available. If I instead create a function that gives b a default value without using partial, getfuncargnames thinks that function should receive no fixtures.

With keyword-only arguments in the mix, it gets weirder:

>>> def f(b, c=0, *a, d=1, **k): print(b, c, a, d, k)
...
>>> g = functools.partial(f, 2)
>>> h = functools.partial(f, 2, c=3)
>>> _pytest.compat.getfuncargnames(f)
('b', 'c')
>>> _pytest.compat.getfuncargnames(g)
('c', 'd')
>>> _pytest.compat.getfuncargnames(h)
('d',)
>>> inspect.signature(f)
<Signature (b, c=0, *a, d=1, **k)>
>>> inspect.signature(g)
<Signature (c=0, *a, d=1, **k)>
>>> inspect.signature(h)
<Signature (*, c=3, d=1, **k)>

In the first case, even though f is the same function as f0 if you look only at mandatory arguments (it requires an argument b), the existence of the keyword-only argument d means that getfuncargnames believes that it should accept fixtures for both b and c. Binding b with partial makes it so that g accepts c and d fixtures, if pytest passes fixture values as keyword arguments rather than positional arguments (otherwise it will crash). Binding b and c makes it so that h will only be given d fixture, even though f doesn't accept a d fixture at all.

In the following example, adding to f an e argument with a default makes b, c, and d available for fixtures:

>>> def f(b, c=0, *a, d, e=1, **k): print(b, c, a, d, e, k)
...
>>> _pytest.compat.getfuncargnames(f)
('b', 'c', 'd')

getfuncargnames's effective API isn't documented anywhere, AFAICT. The unit tests for this function (in fixture.py) aren't very extensive, only covering methods, static methods, and positional-or-keyword arguments with defaults and not in all possible combinations. There are integration tests for mocking and partials in collect.py, integration.py, and metafunc.py. (Did I miss any?) These test those features in isolation, though, not in combination. Keyword-only arguments aren't tested anywhere, I'm sure because of pytest's age.

If it were up to me, I'd simplify this problem by treating only mandatory arguments (those without defaults) as available for fixtures, whether or not those mandatory arguments are keyword-or-positional or keyword-only. This would change the existing behavior. I believe this will make all the existing tests pass, though I haven't checked that yet. As I look into what happens with mock and other combinations of things, I may run into more of this, so stay tuned.

[coveralls]

Coverage increased (+0.02%) to 92.689% when pulling de664a4 on ceridwen:features into c750a5b on pytest-dev:features.

[nicoddemus]

Hi @ceridwen, thanks for the detailed description.

If it were up to me, I'd simplify this problem by treating only mandatory arguments (those without defaults) as available for fixtures, whether or not those mandatory arguments are keyword-or-positional or keyword-only.

I agree. functools.partial support has only recently been added and as you noted, apparently not extensively tested unfortunately.

Regarding partial, I think the expected behavior regarding fixtures is to treat the partial function as if the original arguments bound by the call were effectively removed from the signature.

I don't expect this change in behavior will break any test suites, save for those doing some very exotic things.

Thanks again for looking into this problem @ceridwen!

[ceridwen]

I've replaced the core functionality of getfuncargnames() with inspect.signature. There are two tests still failing:

1. test_funcargnames. This test checks that getfuncargnames() returns only the names of the arguments after the first for an unbound method, only on Python 2. Does the collection code ever pass an unbound method to getfuncargnames() without also passing startindex or cls, and if it does, how does it handle the Python 3 case, where an unbound method is indistinguishable from a function outside of the context it's defined in? To be clear, I don't think that the collection code should be passing unbound methods stripped of context to getfuncargnames() specifically because of Python 3, so I think this test should be removed.

2. TestShowFixtures.test_show_fixtures_indented_in_class. I don't understand what this test is testing or why it's failing. Can someone explain that to me?

There's a lot of commented-out debugging code still. I think startindex should probably be renamed since the way it's really used at the moment is to pass a boolean if and only if a test is collected from a unittest class.

[RonnyPfannschmidt]

this is broken for universal wheels
please use extras_requires

[nicoddemus]

Please prefer to use explicit version checking instead of relying on ImportError:

if _PY3:
    from inspect import signature, Parameter as Parameter
else:
    from funcsigs import signature as signature, Parameter as Parameter

I've seen before a broken installation throwing an ImportError, which would go to the fallback statement and also fail, leaving me baffled for a awhile until finally figuring it out. 😅

[nicoddemus]

About test_getfuncargnames, I can't see an use case for passing an unbound method around like that as well. Looking at the history, that was added in 2009, so perhaps it was added just to "hey this works" rather than "this should work because of real use cases".

test_show_fixtures_indented_in_class tests that the docstring is preserved when using --fixtures. You can try it yourself by executing this:

import pytest
class TestClass:
    @pytest.fixture
    def fixture1():
        """line1
        line2
            indented line
        """

I would guess the problem is that the fixture is missing self.

[ceridwen]

You're correct, it's getting passed to signature() as a bound method, but it's invalid because it lacks the self argument. I added the self argument and deleted the other test, and all the tests now pass locally.

[coveralls]

Coverage decreased (-0.02%) to 92.653% when pulling 10b998f on ceridwen:features into c750a5b on pytest-dev:features.

[nicoddemus]

Weird, for some reasons your PR now includes some commits that are already on features.

[ceridwen]

I fixed that and squashed commits.

[coveralls]

Coverage decreased (-0.005%) to 92.653% when pulling 3da2806 on ceridwen:features into 5c71151 on pytest-dev:features.

[nicoddemus]

Is this now ready @ceridwen? (I've changed the PR to WIP a few hours ago)

Nevermind, the code certainly does look ready! 👍

[nicoddemus]

Awesome work!

[nicoddemus]

Nice docs!

[coveralls]

Coverage decreased (-0.005%) to 92.653% when pulling f7387e4 on ceridwen:features into 5c71151 on pytest-dev:features.

[ceridwen]

Yes, it's ready!

[RonnyPfannschmidt]

well done, the failures are unrelated

[RonnyPfannschmidt]

we might need to alter this bit when people start to use signature to fix up wrappers/decorators

[RonnyPfannschmidt]

we ought to obtain the constants via the instance in order to account for code that accidentially mixes different implementations of funcsigs

cc @nicoddemus #2828