Fix failing staticmethod tests if they are inherited

[antonblr]

Fixes #8061

inspect.getattr_static() (since python 3.2) seem to work better than cls.__dict__.get() here (if i don't miss anything).

[RonnyPfannschmidt]

Lovely, thanks

[bluetech]

getattr_static looks useful, didn't know about it. It might also help in a few other places, like for the annoying issue we had in #6936 when probing for fixtures.

However the getattr_static code looks quite complex (just from glancing at it) so I'd like to verify that it doesn't regress performance too much. I'll do it when I get a chance.

[antonblr]

@bluetech - yes, it is slower than the __dict__.get(), but it does not seem to be slow enough to "feel" the difference.
I've tried benchmarking using pytest-benchmark with the following setup (perhaps it could be simplified or should be more complex):

import inspect


class A:
    def base(self):
        pass

    @staticmethod
    def base_static():
        pass

    @classmethod
    def base_class(cls):
        pass


class B(A):
    def child(self):
        pass

    @staticmethod
    def child_static():
        pass


def test_dict_get(benchmark):
    def dict_get():
        return B.__dict__.get("child_static")

    result = benchmark.pedantic(dict_get, iterations=10, rounds=100)
    assert isinstance(result, staticmethod)


def test_inspect(benchmark):
    def inspect_getattr_static():
        return inspect.getattr_static(B, "child_static")

    result = benchmark.pedantic(inspect_getattr_static, iterations=10, rounds=100)
    assert isinstance(result, staticmethod)

And the results are:

------------------------------------------------------------------------------------------- benchmark: 2 tests ------------------------------------------------------------------------------------------
Name (time in ns)            Min                   Max                  Mean              StdDev                Median                IQR            Outliers  OPS (Kops/s)            Rounds  Iterations
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_dict_get           252.1000 (1.0)        471.5000 (1.0)        265.6320 (1.0)       21.6033 (1.0)        264.8500 (1.0)       8.4500 (1.0)           1;2    3,764.6067 (1.0)         100          10
test_inspect          2,290.4000 (9.09)     3,760.9000 (7.98)     2,359.9490 (8.88)     145.8678 (6.75)     2,339.7000 (8.83)     63.6000 (7.53)          1;1      423.7380 (0.11)        100          10
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Legend:
  Outliers: 1 Standard Deviation from Mean; 1.5 IQR (InterQuartile Range) from 1st Quartile and 3rd Quartile.
  OPS: Operations Per Second, computed as 1 / Mean

Is there a specific way of benchmarking pytest?

[bluetech]

I usually try to construct some relevant synthetic test file which exercises the relevant code and check for regressions with cProfile (Python's builtin tracing profiler). In this I tried this:

import pytest

class TestIt:
    for i in range(5000):
        exec(f"@staticmethod\ndef test_{i}(): pass")

The additional getattr_static calls barely register. So even though your benchmark shows that getattr_static is definitely slower on its own, it doesn't matter for this code. So we're good on this front. (BTW, even if it did slow things down, correctness is more important than performance. But it's still good to check).

[bluetech]

LGTM with a couple comments/suggestions.

[bluetech]

To keep the same behavior as before, should do

Suggested change

	cls and not isinstance(inspect.getattr_static(cls, name), staticmethod)
	cls and not isinstance(inspect.getattr_static(cls, name, None), staticmethod)

unless you find it's not necessary.

[antonblr]

let's be explicit - fixed in 0bf451b

[RonnyPfannschmidt]

actually its unnecessary, as far as i understood the method existence is already known, the difference is that now its looked up in all of the mro

[bluetech]

Since the getattr_static is a bit subtle, I suggest adding a short comment about it, something like:

# Not using getattr because we don't want to resolve the staticmethod.
# Not using cls.__dict__ because we want to check the entire MRO.

[antonblr]

added in 0bf451b - thanks @bluetech