From 964b5f98d050feeacbc22e4cac8b4e6426bd86af Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Thu, 5 Sep 2024 11:56:23 -0400 Subject: [PATCH 1/3] do not discover properties when iterating fixtures --- changelog/12446.bugfix.rst | 1 + src/_pytest/fixtures.py | 16 +++++++++++++--- testing/python/collect.py | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 changelog/12446.bugfix.rst diff --git a/changelog/12446.bugfix.rst b/changelog/12446.bugfix.rst new file mode 100644 index 00000000000..2f591c48eed --- /dev/null +++ b/changelog/12446.bugfix.rst @@ -0,0 +1 @@ +Avoid calling ``@property`` (and other instance descriptors) during fixture discovery -- by :user:`asottile` diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index aaa92c63725..5314f88a956 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -53,6 +53,7 @@ from _pytest.compat import NOTSET from _pytest.compat import NotSetType from _pytest.compat import safe_getattr +from _pytest.compat import safe_isclass from _pytest.config import _PluggyPlugin from _pytest.config import Config from _pytest.config import ExitCode @@ -1724,17 +1725,26 @@ def parsefactories( if holderobj in self._holderobjseen: return + # avoid accessing `@property` (and other descriptors) when iterating fixtures + if not safe_isclass(holderobj) and not isinstance(holderobj, types.ModuleType): + holderobj_tp: object = type(holderobj) + else: + holderobj_tp = holderobj + self._holderobjseen.add(holderobj) for name in dir(holderobj): # The attribute can be an arbitrary descriptor, so the attribute - # access below can raise. safe_getatt() ignores such exceptions. - obj = safe_getattr(holderobj, name, None) - marker = getfixturemarker(obj) + # access below can raise. safe_getattr() ignores such exceptions. + obj_ub = safe_getattr(holderobj_tp, name, None) + marker = getfixturemarker(obj_ub) if not isinstance(marker, FixtureFunctionMarker): # Magic globals with __getattr__ might have got us a wrong # fixture attribute. continue + # ok we know it is a fixture -- now safe to look up on the _instance_ + obj = getattr(holderobj, name) + if marker.name: name = marker.name diff --git a/testing/python/collect.py b/testing/python/collect.py index 06386611279..2edab2affe3 100644 --- a/testing/python/collect.py +++ b/testing/python/collect.py @@ -263,6 +263,41 @@ def prop(self): result = pytester.runpytest() assert result.ret == ExitCode.NO_TESTS_COLLECTED + def test_does_not_discover_properties(self, pytester: Pytester) -> None: + pytester.makepyfile( + """\ + class TestCase: + @property + def oops(self): + raise SystemExit('do not call me!') + """ + ) + result = pytester.runpytest() + assert result.ret == ExitCode.NO_TESTS_COLLECTED + + def test_does_not_discover_instance_descriptors(self, pytester: Pytester) -> None: + pytester.makepyfile( + """\ + # not `@property`, but it acts like one + # this should cover the case of things like `@cached_property` / etc. + class MyProperty: + def __init__(self, func): + self._func = func + def __get__(self, inst, owner): + if inst is None: + return self + else: + return self._func.__get__(inst, owner)() + + class TestCase: + @MyProperty + def oops(self): + raise SystemExit('do not call me!') + """ + ) + result = pytester.runpytest() + assert result.ret == ExitCode.NO_TESTS_COLLECTED + def test_abstract_class_is_not_collected(self, pytester: Pytester) -> None: """Regression test for #12275 (non-unittest version).""" pytester.makepyfile( From f6ef057104a8ede8019e315fdd2b7baf1a3bcf84 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sun, 8 Sep 2024 11:30:03 -0300 Subject: [PATCH 2/3] Apply suggestions from code review --- src/_pytest/fixtures.py | 4 ++-- testing/python/collect.py | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 5314f88a956..6b882fa3515 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1725,7 +1725,7 @@ def parsefactories( if holderobj in self._holderobjseen: return - # avoid accessing `@property` (and other descriptors) when iterating fixtures + # Avoid accessing `@property` (and other descriptors) when iterating fixtures. if not safe_isclass(holderobj) and not isinstance(holderobj, types.ModuleType): holderobj_tp: object = type(holderobj) else: @@ -1742,7 +1742,7 @@ def parsefactories( # fixture attribute. continue - # ok we know it is a fixture -- now safe to look up on the _instance_ + # OK we know it is a fixture -- now safe to look up on the _instance_. obj = getattr(holderobj, name) if marker.name: diff --git a/testing/python/collect.py b/testing/python/collect.py index 2edab2affe3..c360eab234f 100644 --- a/testing/python/collect.py +++ b/testing/python/collect.py @@ -264,6 +264,7 @@ def prop(self): assert result.ret == ExitCode.NO_TESTS_COLLECTED def test_does_not_discover_properties(self, pytester: Pytester) -> None: + """Regression test for #12446.""" pytester.makepyfile( """\ class TestCase: @@ -276,6 +277,7 @@ def oops(self): assert result.ret == ExitCode.NO_TESTS_COLLECTED def test_does_not_discover_instance_descriptors(self, pytester: Pytester) -> None: + """Regression test for #12446.""" pytester.makepyfile( """\ # not `@property`, but it acts like one From b9641b46092337df49a7dddddc19d677fc0774ef Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 8 Sep 2024 14:30:21 +0000 Subject: [PATCH 3/3] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- testing/python/collect.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/python/collect.py b/testing/python/collect.py index c360eab234f..530f1c340ff 100644 --- a/testing/python/collect.py +++ b/testing/python/collect.py @@ -277,7 +277,7 @@ def oops(self): assert result.ret == ExitCode.NO_TESTS_COLLECTED def test_does_not_discover_instance_descriptors(self, pytester: Pytester) -> None: - """Regression test for #12446.""" + """Regression test for #12446.""" pytester.makepyfile( """\ # not `@property`, but it acts like one