search

pytest-dev / pytest

The pytest framework makes it easy to write small tests, yet scales to support complex functional testing
https://pytest.org
MIT License
12.1k stars 2.68k forks source link

compat::getfuncargnames removes self from instance method twice by mistake #6750

Open SeanXu1984 opened 4 years ago

SeanXu1984 commented 4 years ago
SeanXu1984 commented 4 years ago 
def signature(obj):
    '''Get a signature object for the passed callable.'''

    if not callable(obj):
        raise TypeError('{0!r} is not a callable object'.format(obj))

    if isinstance(obj, types.MethodType):
        sig = signature(obj.__func__)
        if obj.__self__ is None:
            # Unbound method - preserve as-is.
            return sig
        else:
            # Bound method. Eat self - if we can.
            params = tuple(sig.parameters.values())

            if not params or params[0].kind in (_VAR_KEYWORD, _KEYWORD_ONLY):
                raise ValueError('invalid method signature')

            kind = params[0].kind
            if kind in (_POSITIONAL_OR_KEYWORD, _POSITIONAL_ONLY):
                # Drop first parameter:
                # '(p1, p2[, ...])' -> '(p2[, ...])'
                params = params[1:]
            else:
                if kind is not _VAR_POSITIONAL:
                    # Unless we add a new parameter type we never
                    # get here
                    raise ValueError('invalid argument type')
                # It's a var-positional parameter.
                # Do nothing. '(*args[, ...])' -> '(*args[, ...])'

            return sig.replace(parameters=params)

    try:
        sig = obj.__signature__
    except AttributeError:
        pass
    else:
        if sig is not None:
            return sig

From funcsigs, self is already removed so compat::getfuncargnames should not try to remove it again. And this is how my fixture got removed from argnames

SeanXu1984 commented 4 years ago

`

_pytest/python.py

class Function(FunctionMixin, nodes.Item, fixtures.FuncargnamesCompatAttr): """ a Function Item is responsible for setting up and executing a Python test function. """

# disable since functions handle it themselves
_ALLOW_MARKERS = False

def __init__(
    self,
    name,
    parent,
    args=None,
    config=None,
    callspec=None,
    callobj=NOTSET,
    keywords=None,
    session=None,
    fixtureinfo=None,
    originalname=None,
):
    super(Function, self).__init__(name, parent, config=config, session=session)
    self._args = args
    if callobj is not NOTSET:
        self.obj = callobj

    self.keywords.update(self.obj.__dict__)
    self.own_markers.extend(get_unpacked_marks(self.obj))
    if callspec:
        self.callspec = callspec
        # this is total hostile and a mess
        # keywords are broken by design by now
        # this will be redeemed later
        for mark in callspec.marks:
            # feel free to cry, this was broken for years before
            # and keywords cant fix it per design
            self.keywords[mark.name] = mark
        self.own_markers.extend(normalize_mark_list(callspec.marks))
    if keywords:
        self.keywords.update(keywords)

    # todo: this is a hell of a hack
    # https://github.com/pytest-dev/pytest/issues/4569

    self.keywords.update(
        dict.fromkeys(
            [
                mark.name
                for mark in self.iter_markers()
                if mark.name not in self.keywords
            ],
            True,
        )
    )

    if fixtureinfo is None:
        fixtureinfo = self.session._fixturemanager.getfixtureinfo(
            self, self.obj, self.cls, funcargs=True          <======= getfixtureinfo is called with cls = self.cls
        )

_pytest/fixtures.py

def getfixtureinfo(self, node, func, cls, funcargs=True):
    if funcargs and not getattr(node, "nofuncargs", False):
        argnames = getfuncargnames(func, cls=cls)    <======== getfuncargnames is called with cls=cls
    else:
        argnames = ()

_pytest/compat.py

def getfuncargnames(function, is_method=False, cls=None): """Returns the names of a function's mandatory arguments.

This should return the names of all function arguments that:
    * Aren't bound to an instance or type as in instance or class methods.
    * Don't have default values.
    * Aren't bound with functools.partial.
    * Aren't replaced with mocks.

The is_method and cls arguments indicate that the function should
be treated as a bound method even though it's not unless, only in
the case of cls, the function is a static method.

@RonnyPfannschmidt: This function should be refactored when we
revisit fixtures. The fixture mechanism should ask the node for
the fixture names, and not try to obtain directly from the
function object well after collection has occurred.

"""
# The parameters attribute of a Signature object contains an
# ordered mapping of parameter names to Parameter instances.  This
# creates a tuple of the names of the parameters that don't have
# defaults.
try:
    parameters = signature(function).parameters     <======== self should already be removed from here
except (ValueError, TypeError) as e:
    fail(
        "Could not determine arguments of {!r}: {}".format(function, e),
        pytrace=False,
    )

arg_names = tuple(
    p.name
    for p in parameters.values()
    if (
        p.kind is Parameter.POSITIONAL_OR_KEYWORD
        or p.kind is Parameter.KEYWORD_ONLY
    )
    and p.default is Parameter.empty
)
# If this function should be treated as a bound method even though
# it's passed as an unbound method or function, remove the first
# parameter name.
if is_method or (
    cls and not isinstance(cls.__dict__.get(function.__name__, None), staticmethod)
):
    arg_names = arg_names[1:]       <====== trying to remove self again and caused the problem

`

RonnyPfannschmidt commented 4 years ago

this needs more debugging

as memory serves me, pytest is set up to collect the actually class level callable, where self is supposed to be part of the signature and needs to be removed unless its an actual static method

if we have bound methods in that place, something is certainly wrong and i can see why the first instinct is to note our simplification as culprit,

unfortunately i can't verify the details soon

RonnyPfannschmidt commented 4 years ago

i apologize, after re-reading the original text i understood the issue is indeed abouta bound method on a own item,

we should support bound methods there and with signature and a type check we probably can do that correctly

SeanXu1984 commented 4 years ago

I suppose I should create PR against 4.6.x branch?