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

[SeanXu1984]

• a detailed description of the bug or suggestion
  I have a pyfuncitem which is an instance method and its signature looks like this:
  def run_test(self, testbed_obj): <== This testbed_obj is a fixture
  When getfuncargnames got called to return the argnames, it removed this testbed_obj in line 168 of compat.py, which I think is wrong, because the parameter got from line 146 of compat.py has self removed from the signature.
• output of pip list from the virtual environment you are using
  Package Version Location

---

astroid 1.4.9
atomicwrites 1.3.0
attrs 19.3.0
automatos-tool-library 15
avro 1.9.1
backports.functools-lru-cache 1.6.1
backports.shutil-get-terminal-size 1.0.0
backports.ssl-match-hostname 3.7.0.1
bcrypt 3.1.7
certifi 2019.11.28
cffi 1.13.2
chardet 3.0.4
colorama 0.4.3
configparser 4.0.2
confluent-kafka 1.3.0
contextlib2 0.6.0.post1
coverage 5.0.3
cryptography 2.8
decorator 4.4.1
dicttoxml 1.7.4
docker-py 1.10.6
docker-pycreds 0.4.0
enum34 1.1.6
filelock 3.0.12
flake8 3.6.0
FormEncode 1.3.1
funcsigs 1.0.2
future 0.18.2
futures 3.3.0
gitdb2 2.0.6
GitPython 2.1.14
httpretty 0.9.7
idna 2.8
importlib-metadata 1.5.0
ipaddress 1.0.23
isort 4.3.21
lazy-object-proxy 1.4.3
logutils 0.3.5
mccabe 0.6.1
mock 3.0.5
more-itertools 5.0.0
ndg-httpsclient 0.5.1
packaging 20.1
paramiko 2.7.1
pathlib2 2.3.5
pip 20.0.2
pluggy 0.13.1
py 1.8.1
pyasn1 0.4.8
pybase64 0.5.0
pycodestyle 2.4.0
pycparser 2.19
pycyclone 0.9.277
pyflakes 2.0.0
pylint 1.6.4
PyNaCl 1.3.0
pyOpenSSL 19.1.0
pyparsing 2.4.6
pysftp 0.2.9
pytest 4.6.9
pytest-html 1.22.0
pytest-metadata 1.8.0
python-nmap 0.6.1
pytz 2019.3
pyvmomi 6.7.3
PyYAML 5.3
redis 2.10.6
requests 2.22.0
requests-mock 1.6.0
requests-toolbelt 0.9.1
retrying 1.3.3
scandir 1.10.0
setuptools 44.0.0
setuptools-scm 3.2.0
six 1.14.0
smmap2 2.0.5
spur 0.3.21
suds 0.4
toml 0.10.0
tox 3.14.3
urllib3 1.23
uuid 1.30
validators 0.11.3
virtualenv 16.7.9
waiting 1.4.1
walrus 0.3.5
wcwidth 0.1.8
websocket-client 0.57.0
wheel 0.34.2
wrapt 1.11.2
xmltodict 0.12.0
zipp 1.1.0
We have some private Python packages that I don't want to list here.

• pytest and operating system versions
  pytest 4.6.9
  Windows 10
• minimal example if possible
  class MyTest(BaseTestCase):
  def run_test(self, testbed_obj):

[SeanXu1984]

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]

`

_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]

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]

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]

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