Skip to content

Commit

Permalink
python: fix instance handling in static and class method tests
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bluetech committed Mar 9, 2024
1 parent 774f0c4 commit 0dc0360
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 15 deletions.
4 changes: 4 additions & 0 deletions changelog/12065.bugfix.rst
Original file line number Diff line number Diff line change
@@ -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 <pytest.FixtureRequest.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``.
5 changes: 3 additions & 2 deletions src/_pytest/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,8 +470,9 @@ def cls(self):
@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):
Expand Down
34 changes: 27 additions & 7 deletions src/_pytest/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -302,10 +302,10 @@ def instance(self):
"""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):
Expand Down Expand Up @@ -1702,7 +1702,8 @@ def __init__(
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
Expand Down Expand Up @@ -1752,12 +1753,31 @@ def function(self):
"""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

Check warning on line 1773 in src/_pytest/python.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/python.py#L1773

Added line #L1773 was not covered by tests

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)

Expand Down
10 changes: 5 additions & 5 deletions src/_pytest/unittest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Check warning on line 188 in src/_pytest/unittest.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/unittest.py#L188

Added line #L188 was not covered by tests

def setup(self) -> None:
# A bound method to be called during teardown() if set (see 'runtest()').
Expand Down Expand Up @@ -296,7 +295,8 @@ def addDuration(self, testcase: "unittest.TestCase", elapsed: float) -> None:
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)

Expand Down
45 changes: 45 additions & 0 deletions testing/python/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
17 changes: 16 additions & 1 deletion testing/python/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 0dc0360

Please sign in to comment.