From 006058f1f90b00988cc61043f1ea55bdd2c55883 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 9 Mar 2024 10:13:15 +0200 Subject: [PATCH 1/3] fixtures: update outdated comment No longer does unittest stuff. Also the rest of the sentence is not really necessary for a docstring. --- src/_pytest/fixtures.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 4b7c10752..58b515434 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1096,7 +1096,7 @@ def resolve_fixture_function( fixturedef: FixtureDef[FixtureValue], request: FixtureRequest ) -> "_FixtureFunc[FixtureValue]": """Get the actual callable that can be called to obtain the fixture - value, dealing with unittest-specific instances and bound methods.""" + value.""" fixturefunc = fixturedef.func # The fixture function needs to be bound to the actual # request.instance so that code working with "fixturedef" behaves From 774f0c44e63077efb2a7cf35b19b99a255b22332 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 9 Mar 2024 09:11:33 +0200 Subject: [PATCH 2/3] fixtures: only call `instance` property once in function No need to compute the property multiple times. --- src/_pytest/fixtures.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 58b515434..1eca68207 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1101,17 +1101,18 @@ def resolve_fixture_function( # The fixture function needs to be bound to the actual # request.instance so that code working with "fixturedef" behaves # as expected. - if request.instance is not None: + instance = request.instance + if instance is not None: # Handle the case where fixture is defined not in a test class, but some other class # (for example a plugin class with a fixture), see #2270. if hasattr(fixturefunc, "__self__") and not isinstance( - request.instance, + instance, fixturefunc.__self__.__class__, # type: ignore[union-attr] ): return fixturefunc fixturefunc = getimfunc(fixturedef.func) if fixturefunc != fixturedef.func: - fixturefunc = fixturefunc.__get__(request.instance) # type: ignore[union-attr] + fixturefunc = fixturefunc.__get__(instance) # type: ignore[union-attr] return fixturefunc From 0dc036035107b213c9b73bf965cbd7356111b85a Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 9 Mar 2024 09:08:44 +0200 Subject: [PATCH 3/3] python: fix instance handling in static and class method tests and also fixes a regression in pytest 8.0.0 where `setup_method` crashes if the class has static or class method tests. It is allowed to have a test class with static/class methods which request non-static/class method fixtures (including `setup_method` xunit-fixture). I take it as a given that we need to support this somewhat odd scenario (stdlib unittest also supports it). This raises a question -- when a staticmethod test requests a bound fixture, what is that fixture's `self`? stdlib unittest says - a fresh instance for the test. Previously, pytest said - some instance that is shared by all static/class methods. This is definitely broken since it breaks test isolation. Change pytest to behave like stdlib unittest here. In practice, this means stopping to rely on `self.obj.__self__` to get to the instance from the test function's binding. This doesn't work because staticmethods are not bound to anything. Instead, keep the instance explicitly and use that. BTW, I think this will allow us to change `Class`'s fixture collection (`parsefactories`) to happen on the class itself instead of a class instance, allowing us to avoid one class instantiation. But needs more work. Fixes #12065. --- changelog/12065.bugfix.rst | 4 ++++ src/_pytest/fixtures.py | 5 ++-- src/_pytest/python.py | 34 ++++++++++++++++++++------ src/_pytest/unittest.py | 10 ++++---- testing/python/fixtures.py | 45 +++++++++++++++++++++++++++++++++++ testing/python/integration.py | 17 ++++++++++++- 6 files changed, 100 insertions(+), 15 deletions(-) create mode 100644 changelog/12065.bugfix.rst diff --git a/changelog/12065.bugfix.rst b/changelog/12065.bugfix.rst new file mode 100644 index 000000000..ca55b327e --- /dev/null +++ b/changelog/12065.bugfix.rst @@ -0,0 +1,4 @@ +Fixed a regression in pytest 8.0.0 where test classes containing ``setup_method`` and tests using ``@staticmethod`` or ``@classmethod`` would crash with ``AttributeError: 'NoneType' object has no attribute 'setup_method'``. + +Now the :attr:`request.instance ` attribute of tests using ``@staticmethod`` and ``@classmethod`` is no longer ``None``, but a fresh instance of the class, like in non-static methods. +Previously it was ``None``, and all fixtures of such tests would share a single ``self``. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 1eca68207..daf3145aa 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -470,8 +470,9 @@ class FixtureRequest(abc.ABC): @property def instance(self): """Instance (can be None) on which test function was collected.""" - function = getattr(self, "function", None) - return getattr(function, "__self__", None) + if self.scope != "function": + return None + return getattr(self._pyfuncitem, "instance", None) @property def module(self): diff --git a/src/_pytest/python.py b/src/_pytest/python.py index fce2078cd..7b0683b6e 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -302,10 +302,10 @@ class PyobjMixin(nodes.Node): """Python instance object the function is bound to. Returns None if not a test method, e.g. for a standalone test function, - a staticmethod, a class or a module. + a class or a module. """ - node = self.getparent(Function) - return getattr(node.obj, "__self__", None) if node is not None else None + # Overridden by Function. + return None @property def obj(self): @@ -1702,7 +1702,8 @@ class Function(PyobjMixin, nodes.Item): super().__init__(name, parent, config=config, session=session) if callobj is not NOTSET: - self.obj = callobj + self._obj = callobj + self._instance = getattr(callobj, "__self__", None) #: Original function name, without any decorations (for example #: parametrization adds a ``"[...]"`` suffix to function names), used to access @@ -1752,12 +1753,31 @@ class Function(PyobjMixin, nodes.Item): """Underlying python 'function' object.""" return getimfunc(self.obj) - def _getobj(self): - assert self.parent is not None + @property + def instance(self): + try: + return self._instance + except AttributeError: + if isinstance(self.parent, Class): + # Each Function gets a fresh class instance. + self._instance = self._getinstance() + else: + self._instance = None + return self._instance + + def _getinstance(self): if isinstance(self.parent, Class): # Each Function gets a fresh class instance. - parent_obj = self.parent.newinstance() + return self.parent.newinstance() else: + return None + + def _getobj(self): + instance = self.instance + if instance is not None: + parent_obj = instance + else: + assert self.parent is not None parent_obj = self.parent.obj # type: ignore[attr-defined] return getattr(parent_obj, self.originalname) diff --git a/src/_pytest/unittest.py b/src/_pytest/unittest.py index b0ec02e7d..32eb361c6 100644 --- a/src/_pytest/unittest.py +++ b/src/_pytest/unittest.py @@ -177,16 +177,15 @@ class TestCaseFunction(Function): nofuncargs = True _excinfo: Optional[List[_pytest._code.ExceptionInfo[BaseException]]] = None - def _getobj(self): + def _getinstance(self): assert isinstance(self.parent, UnitTestCase) - testcase = self.parent.obj(self.name) - return getattr(testcase, self.name) + return self.parent.obj(self.name) # Backward compat for pytest-django; can be removed after pytest-django # updates + some slack. @property def _testcase(self): - return self._obj.__self__ + return self.instance def setup(self) -> None: # A bound method to be called during teardown() if set (see 'runtest()'). @@ -296,7 +295,8 @@ class TestCaseFunction(Function): def runtest(self) -> None: from _pytest.debugging import maybe_wrap_pytest_function_for_tracing - testcase = self.obj.__self__ + testcase = self.instance + assert testcase is not None maybe_wrap_pytest_function_for_tracing(self) diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 8d59b36d3..2e277626c 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -4577,3 +4577,48 @@ def test_deduplicate_names() -> None: assert items == ("a", "b", "c", "d") items = deduplicate_names((*items, "g", "f", "g", "e", "b")) assert items == ("a", "b", "c", "d", "g", "f", "e") + + +def test_staticmethod_classmethod_fixture_instance(pytester: Pytester) -> None: + """Ensure that static and class methods get and have access to a fresh + instance. + + This also ensures `setup_method` works well with static and class methods. + + Regression test for #12065. + """ + pytester.makepyfile( + """ + import pytest + + class Test: + ran_setup_method = False + ran_fixture = False + + def setup_method(self): + assert not self.ran_setup_method + self.ran_setup_method = True + + @pytest.fixture(autouse=True) + def fixture(self): + assert not self.ran_fixture + self.ran_fixture = True + + def test_method(self): + assert self.ran_setup_method + assert self.ran_fixture + + @staticmethod + def test_1(request): + assert request.instance.ran_setup_method + assert request.instance.ran_fixture + + @classmethod + def test_2(cls, request): + assert request.instance.ran_setup_method + assert request.instance.ran_fixture + """ + ) + result = pytester.runpytest() + assert result.ret == ExitCode.OK + result.assert_outcomes(passed=3) diff --git a/testing/python/integration.py b/testing/python/integration.py index a6c14ece4..219ebf9ce 100644 --- a/testing/python/integration.py +++ b/testing/python/integration.py @@ -410,22 +410,37 @@ def test_function_instance(pytester: Pytester) -> None: items = pytester.getitems( """ def test_func(): pass + class TestIt: def test_method(self): pass + @classmethod def test_class(cls): pass + @staticmethod def test_static(): pass """ ) assert len(items) == 4 + assert isinstance(items[0], Function) assert items[0].name == "test_func" assert items[0].instance is None + assert isinstance(items[1], Function) assert items[1].name == "test_method" assert items[1].instance is not None assert items[1].instance.__class__.__name__ == "TestIt" + + # Even class and static methods get an instance! + # This is the instance used for bound fixture methods, which + # class/staticmethod tests are perfectly able to request. + assert isinstance(items[2], Function) + assert items[2].name == "test_class" + assert items[2].instance is not None + assert isinstance(items[3], Function) assert items[3].name == "test_static" - assert items[3].instance is None + assert items[3].instance is not None + + assert items[1].instance is not items[2].instance is not items[3].instance