diff --git a/changelog/3926.improvement.rst b/changelog/3926.improvement.rst new file mode 100644 index 00000000000..98f5503663d --- /dev/null +++ b/changelog/3926.improvement.rst @@ -0,0 +1,15 @@ +Added the :confval:`collect_function_definition` option, which can promote the +internal ``FunctionDefinition`` to a real node in the collection tree. + +When set to ``pedantic``, a ``FunctionDefinition`` collector node is inserted +between a :class:`~pytest.Module`/:class:`~pytest.Class` and its test functions, +so that the (possibly parametrized) invocations of each test are collected +underneath a shared definition node instead of as flat siblings. Function-level +markers are scoped to the definition node in this mode. + +The default ``hidden`` keeps the previous flat layout, and the transitional +``messy`` value keeps the definition node out of marker resolution for code that +is not yet prepared for the new scope. + +The node ids of the individual test invocations are unchanged in all modes, so +test selection, caching and reporting behave exactly as before. diff --git a/doc/en/reference/reference.rst b/doc/en/reference/reference.rst index 4f2e0b70850..b7235d58a99 100644 --- a/doc/en/reference/reference.rst +++ b/doc/en/reference/reference.rst @@ -1378,6 +1378,79 @@ passed multiple times. The expected format is ``name=value``. For example:: variables, that will be expanded. For more information about cache plugin please refer to :ref:`cache_provider`. +.. confval:: collect_function_definition + :type: ``string`` + :default: ``hidden`` + + .. versionadded:: 9.2 + + Controls whether a ``FunctionDefinition`` collector node is inserted into the + collection tree between a :class:`~pytest.Module`/:class:`~pytest.Class` and + its test functions, grouping the (possibly parametrized) invocations of a test + function *underneath* the definition node instead of as flat siblings. + + It accepts three values: + + ``hidden`` (default) + Keep the flat layout. The definition node is used only internally to drive + parametrization and is kept out of the collection tree. + + ``pedantic`` + Insert the definition node with correct marker scoping: function-level + markers (such as ``@pytest.mark.parametrize`` or custom marks) belong to + the definition node, and each invocation only owns its own parameter-set + markers. This is the intended target layout. + + ``messy`` + A stopgap for migration only. It inserts the definition node but transfers + the function-level markers back down onto each invocation, reproducing the + old (duplicated) marker layout so that code which is not yet prepared for + the definition scope keeps working. It emits a warning in the session + header and exists purely to buy time; fix the offending code and move to + ``pedantic``. + + .. tab:: toml + + .. code-block:: toml + + [pytest] + collect_function_definition = "pedantic" + + .. tab:: ini + + .. code-block:: ini + + [pytest] + collect_function_definition = pedantic + + For example, given a parametrized test: + + .. code-block:: python + + import pytest + + + @pytest.mark.parametrize("x", [1, 2]) + def test_it(x): ... + + the default (``hidden``) layout collects the invocations directly under the + module:: + + + + + + while ``pedantic`` and ``messy`` group them under a definition node:: + + + + + + + This is an opt-in structural change. The node ids of the individual test + invocations are unchanged (for example ``test_it.py::test_it[1]``), so test + selection, caching and reporting behave exactly as before. + .. confval:: collect_imported_tests :type: ``bool`` :default: ``true`` @@ -3677,6 +3750,14 @@ All the command-line flags can also be obtained by running ``pytest --help``:: collect_imported_tests (bool): Whether to collect tests in imported modules outside `testpaths` + collect_function_definition (string): + How the function-definition collector node + participates in the collection tree. - hidden + (default): keep the flat layout, no node in the tree + - pedantic: insert the node and scope function-level + markers to it - messy: insert the node but transfer + markers down to each invocation to preserve the + legacy marker layout (emits a warning) consider_namespace_packages (bool): Consider namespace packages when resolving module names during import diff --git a/src/_pytest/main.py b/src/_pytest/main.py index 1b337e20c7e..364f27b7d2d 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -251,6 +251,17 @@ def pytest_addoption(parser: Parser) -> None: type="bool", default=True, ) + parser.addini( + "collect_function_definition", + "How the function-definition collector node participates in the " + "collection tree.\n" + "- hidden (default): keep the flat layout, no node in the tree\n" + "- pedantic: insert the node and scope function-level markers to it\n" + "- messy: insert the node but transfer markers down to each invocation " + "to preserve the legacy marker layout (emits a warning)", + type="string", + default="hidden", + ) parser.addini( "consider_namespace_packages", type="bool", diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 4f168012c36..7b2d26669ea 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -91,6 +91,29 @@ get_args(LongStrIdStrategy) ) +# Modes for the ``collect_function_definition`` option. +# - "hidden": legacy flat layout; the FunctionDefinition is used transiently to +# drive parametrization and kept out of ("hidden" from) the tree. +# - "pedantic": insert the FunctionDefinition node; function-level markers are +# scoped to it and each invocation only owns its callspec markers. +# - "messy": insert the node, but transfer the function-level markers down onto +# each invocation to preserve the legacy marker layout. Emits a +# warning, as this defeats the purpose of the definition scope. +FunctionDefinitionMode = Literal["hidden", "pedantic", "messy"] +_FUNCTION_DEFINITION_MODES: frozenset[FunctionDefinitionMode] = frozenset( + get_args(FunctionDefinitionMode) +) + + +def _collect_function_definition_mode(config: Config) -> FunctionDefinitionMode: + value = config.getini("collect_function_definition") + if value not in _FUNCTION_DEFINITION_MODES: + raise UsageError( + f"Unknown collect_function_definition: {value!r}. " + f"Valid values: {', '.join(sorted(_FUNCTION_DEFINITION_MODES))}" + ) + return cast(FunctionDefinitionMode, value) + def pytest_addoption(parser: Parser) -> None: parser.addini( @@ -163,6 +186,16 @@ def pytest_configure(config: Config) -> None: ) +def pytest_report_header(config: Config) -> list[str] | None: + if _collect_function_definition_mode(config) == "messy": + return [ + "warning: collect_function_definition=messy transfers markers to the " + "individual invocations to preserve the legacy layout; prefer " + "'pedantic' once your plugins handle the definition scope", + ] + return None + + def async_fail(nodeid: str) -> None: msg = ( "async def functions are not natively supported.\n" @@ -331,6 +364,11 @@ def getmodpath(self, stopatmodule: bool = True, includemodule: bool = False) -> """Return Python path relative to the containing module.""" parts = [] for node in self.iter_parents(): + # A FunctionDefinition parent contributes the same name as the + # Function collected under it, so skip it to avoid duplication + # (but keep it when it is the node itself). + if node is not self and isinstance(node, FunctionDefinition): + continue name = node.name if isinstance(node, Module): name = os.path.splitext(name)[0] @@ -467,54 +505,19 @@ def collect(self) -> Iterable[nodes.Item | nodes.Collector]: result.extend(values) return result - def _genfunctions(self, name: str, funcobj) -> Iterator[Function]: - modulecol = self.getparent(Module) - assert modulecol is not None - module = modulecol.obj - clscol = self.getparent(Class) - cls = (clscol and clscol.obj) or None - + def _genfunctions( + self, name: str, funcobj + ) -> Iterator[Function | FunctionDefinition]: definition = FunctionDefinition.from_parent(self, name=name, callobj=funcobj) - fixtureinfo = definition._fixtureinfo - - # pytest_generate_tests impls call metafunc.parametrize() which fills - # metafunc._calls, the outcome of the hook. - metafunc = Metafunc( - definition=definition, - fixtureinfo=fixtureinfo, - config=self.config, - cls=cls, - module=module, - _ispytest=True, - ) - methods = [] - if hasattr(module, "pytest_generate_tests"): - methods.append(module.pytest_generate_tests) - if cls is not None and hasattr(cls, "pytest_generate_tests"): - methods.append(cls().pytest_generate_tests) - self.ihook.pytest_generate_tests.call_extra(methods, dict(metafunc=metafunc)) - - if not metafunc._calls: - yield Function.from_parent(self, name=name, fixtureinfo=fixtureinfo) + if _collect_function_definition_mode(self.config) == "hidden": + # Legacy flat layout: the definition is used only to drive + # parametrization and is discarded ("hidden"), the invocations are + # collected directly under this collector. + yield from definition._generate_functions(self) else: - metafunc._recompute_direct_params_indices() - # Direct parametrizations taking place in module/class-specific - # `metafunc.parametrize` calls may have shadowed some fixtures, so make sure - # we update what the function really needs a.k.a its fixture closure. Note that - # direct parametrizations using `@pytest.mark.parametrize` have already been considered - # into making the closure using `ignore_args` arg to `getfixtureclosure`. - fixtureinfo.prune_dependency_tree() - - for callspec in metafunc._calls: - subname = f"{name}[{callspec.id}]" if callspec._idlist else name - yield Function.from_parent( - self, - name=subname, - callspec=callspec, - fixtureinfo=fixtureinfo, - keywords={callspec.id: True}, - originalname=name, - ) + # Insert the function definition as a collector node into the tree; + # its ``collect()`` yields the (possibly parametrized) invocations. + yield definition def importtestmodule( @@ -1688,8 +1691,9 @@ def __init__( session: Session | None = None, fixtureinfo: FuncFixtureInfo | None = None, originalname: str | None = None, + nodeid: str | None = None, ) -> None: - super().__init__(name, parent, config=config, session=session) + super().__init__(name, parent, config=config, session=session, nodeid=nodeid) if callobj is not NOTSET: self._obj = callobj @@ -1706,7 +1710,12 @@ def __init__( # Note: when FunctionDefinition is introduced, we should change ``originalname`` # to a readonly property that returns FunctionDefinition.name. - self.own_markers.extend(get_unpacked_marks(self.obj)) + # Function-level markers are owned by the FunctionDefinition scope when + # that node is part of the collection tree; otherwise (flat layout) the + # Function owns them itself. In "messy" mode FunctionDefinition.collect() + # transfers them back onto each invocation for legacy compatibility. + if not isinstance(self.parent, FunctionDefinition): + self.own_markers.extend(get_unpacked_marks(self.obj)) if callspec: self.callspec = callspec self.own_markers.extend(callspec.marks) @@ -1748,17 +1757,16 @@ 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 + self._instance = self._getinstance() return self._instance def _getinstance(self): - if isinstance(self.parent, Class): + # The containing class, if any -- skipping over an interposed + # FunctionDefinition node (see the ``collect_function_definition`` option). + cls = self.getparent(Class) + if cls is not None: # Each Function gets a fresh class instance. - return self.parent.newinstance() + return cls.newinstance() else: return None @@ -1767,8 +1775,13 @@ def _getobj(self): if instance is not None: parent_obj = instance else: - assert self.parent is not None - parent_obj = self.parent.obj # type: ignore[attr-defined] + # The namespace this function was collected from -- a Module (or a + # Class handled above), skipping over an interposed FunctionDefinition. + parent = self.parent + while isinstance(parent, FunctionDefinition): + parent = parent.parent + assert parent is not None + parent_obj = parent.obj # type: ignore[attr-defined] return getattr(parent_obj, self.originalname) @property @@ -1823,14 +1836,130 @@ def repr_failure( # type: ignore[override] return self._repr_failure_py(excinfo, style=style) -class FunctionDefinition(Function): - """This class is a stop gap solution until we evolve to have actual function - definition nodes and manage to get rid of ``metafunc``.""" +class FunctionDefinition(PyCollector): + """Collector node for a single test function definition. - def runtest(self) -> None: - raise RuntimeError("function definitions are not supposed to be run as tests") + Its children are the (possibly parametrized) :class:`Function` invocations + generated from the definition via :hook:`pytest_generate_tests`. + + This node is only inserted into the collection tree when the + :confval:`collect_function_definition` option is enabled. Otherwise it is + created transiently to drive parametrization (backing :class:`Metafunc`) + and then discarded, with the resulting :class:`Function` items collected + directly under the containing :class:`Class`/:class:`Module`. + """ - setup = runtest + # Markers are handled explicitly below, mirroring Function. + _ALLOW_MARKERS = False + + def __init__( + self, + name: str, + parent, + callobj, + config: Config | None = None, + session: Session | None = None, + ) -> None: + super().__init__(name, parent, config=config, session=session) + + # The definition always stands for a concrete function object; unlike + # Function it never looks the object up from its parent lazily. + self._obj = callobj + + self.own_markers.extend(get_unpacked_marks(self.obj)) + self.keywords.update((mark.name, mark) for mark in self.own_markers) + self.keywords.update(self.obj.__dict__) + + def collect(self) -> Iterable[nodes.Item | nodes.Collector]: + children = list(self._generate_functions(self)) + if _collect_function_definition_mode(self.config) == "messy": + # Legacy marker layout: transfer this scope's markers back onto each + # invocation and drop them here, so marker resolution matches the flat + # layout exactly (no duplication when walking parents in iter_markers). + marks = self.own_markers + for child in children: + child.own_markers[:0] = marks + child.keywords.update((mark.name, mark) for mark in marks) + self.own_markers = [] + return children + + def _generate_functions(self, parent: nodes.Collector) -> Iterator[Function]: + """Run :hook:`pytest_generate_tests` and yield the resulting + :class:`Function` invocations under ``parent``. + + ``parent`` is this definition node when it is part of the tree, or the + containing :class:`Class`/:class:`Module` in the legacy flat layout. + """ + name = self.name + modulecol = self.getparent(Module) + assert modulecol is not None + module = modulecol.obj + clscol = self.getparent(Class) + cls = (clscol and clscol.obj) or None + + # Compute the function's fixture closure. This drives parametrization and + # is shared with the generated invocations; it is intentionally *not* + # stored on the definition -- fixtures belong to the executed items, not + # to this collector. + # TODO(#3926): getfixtureinfo() is item-scoped, but here the definition + # (a collector) stands in for the not-yet-created invocations. Resolve by + # giving fixture-closure computation a node-level entry point instead of + # casting a collector to an item. + fixtureinfo = self.session._fixturemanager.getfixtureinfo( + cast(nodes.Item, self), self.obj, cls + ) + + # pytest_generate_tests impls call metafunc.parametrize() which fills + # metafunc._calls, the outcome of the hook. + metafunc = Metafunc( + definition=self, + fixtureinfo=fixtureinfo, + config=self.config, + cls=cls, + module=module, + _ispytest=True, + ) + methods = [] + if hasattr(module, "pytest_generate_tests"): + methods.append(module.pytest_generate_tests) + if cls is not None and hasattr(cls, "pytest_generate_tests"): + methods.append(cls().pytest_generate_tests) + self.ihook.pytest_generate_tests.call_extra(methods, dict(metafunc=metafunc)) + + # The invocations keep a flat nodeid anchored at the collector containing + # the definition, regardless of whether the definition itself is part of + # the tree, so nodeids are stable across the ``collect_function_definition`` + # option. + assert self.parent is not None + base_nodeid = self.parent.nodeid + + if not metafunc._calls: + yield Function.from_parent( + parent, + name=name, + fixtureinfo=fixtureinfo, + nodeid=f"{base_nodeid}::{name}", + ) + else: + metafunc._recompute_direct_params_indices() + # Direct parametrizations taking place in module/class-specific + # `metafunc.parametrize` calls may have shadowed some fixtures, so make sure + # we update what the function really needs a.k.a its fixture closure. Note that + # direct parametrizations using `@pytest.mark.parametrize` have already been considered + # into making the closure using `ignore_args` arg to `getfixtureclosure`. + fixtureinfo.prune_dependency_tree() + + for callspec in metafunc._calls: + subname = f"{name}[{callspec.id}]" if callspec._idlist else name + yield Function.from_parent( + parent, + name=subname, + callspec=callspec, + fixtureinfo=fixtureinfo, + keywords={callspec.id: True}, + originalname=name, + nodeid=f"{base_nodeid}::{subname}", + ) def __getattr__(name: str) -> object: diff --git a/testing/test_collect_function_definition.py b/testing/test_collect_function_definition.py new file mode 100644 index 00000000000..5860bd2c356 --- /dev/null +++ b/testing/test_collect_function_definition.py @@ -0,0 +1,193 @@ +"""Tests for the `collect_function_definition` configuration value.""" + +from __future__ import annotations + +from _pytest.pytester import Pytester +import pytest + + +@pytest.fixture +def sample(pytester: Pytester) -> Pytester: + pytester.makepyfile( + """ + import pytest + + def test_plain(): + pass + + @pytest.mark.parametrize("x", [1, 2]) + def test_param(x): + assert x + + class TestCls: + @pytest.mark.parametrize("y", [3, 4]) + def test_method(self, y): + assert y + + def test_single(self): + pass + """ + ) + return pytester + + +def _set_mode(pytester: Pytester, mode: str) -> None: + pytester.makeini(f"[pytest]\ncollect_function_definition = {mode}\n") + + +FLAT_IDS = [ + "test_plain", + "test_param[1]", + "test_param[2]", + "TestCls::test_method[3]", + "TestCls::test_method[4]", + "TestCls::test_single", +] + +TREE_MODES = ["pedantic", "messy"] +ALL_MODES = ["hidden", *TREE_MODES] + + +def test_hidden_is_default(sample: Pytester) -> None: + """Without the option the definition node is not part of the tree.""" + items, _ = sample.inline_genitems() + for item in items: + assert type(item.parent).__name__ in ("Module", "Class") + + +@pytest.mark.parametrize("mode", TREE_MODES) +def test_definition_node_inserted(sample: Pytester, mode: str) -> None: + """In tree modes a FunctionDefinition collector wraps each test.""" + from _pytest.python import Function + from _pytest.python import FunctionDefinition + + _set_mode(sample, mode) + items, _ = sample.inline_genitems() + assert len(items) == len(FLAT_IDS) + for item in items: + assert isinstance(item, Function) + # Every invocation, including non-parametrized ones, is wrapped. + assert isinstance(item.parent, FunctionDefinition) + + +@pytest.mark.parametrize("mode", ALL_MODES) +def test_nodeids_are_stable(sample: Pytester, mode: str) -> None: + """Invocation nodeids stay flat regardless of the mode.""" + _set_mode(sample, mode) + items, _ = sample.inline_genitems() + suffixes = [item.nodeid.split("::", 1)[1] for item in items] + assert suffixes == FLAT_IDS + + +@pytest.mark.parametrize("mode", ALL_MODES) +def test_tests_run(sample: Pytester, mode: str) -> None: + _set_mode(sample, mode) + result = sample.runpytest() + result.assert_outcomes(passed=6) + + +def test_selection_and_reporting(sample: Pytester) -> None: + """-k selection and failure reporting address the flat nodeid.""" + _set_mode(sample, "pedantic") + result = sample.runpytest("-k", "test_param and 1", "-v") + assert "test_param[1] PASSED" in result.stdout.str() + result.assert_outcomes(passed=1, deselected=5) + + +def test_collect_only_tree(sample: Pytester) -> None: + _set_mode(sample, "pedantic") + result = sample.runpytest("--collect-only") + out = result.stdout.str() + for expected in [ + "", + "", + "", + "", + "", + "", + ]: + assert expected in out + + +@pytest.mark.parametrize("mode", ALL_MODES) +def test_markers_are_not_duplicated(pytester: Pytester, mode: str) -> None: + """A function-level marker resolves exactly once in every mode. + + Regression guard: with the definition node in the tree both it and the + invocation carry the same function-level marks, which must not double up + when walking parents in ``iter_markers``. + """ + pytester.makeini(f"[pytest]\ncollect_function_definition = {mode}\n") + pytester.makepyfile( + """ + import pytest + + @pytest.mark.foo + @pytest.mark.parametrize("x", [1]) + def test_it(x): + pass + """ + ) + items, _ = pytester.inline_genitems() + (item,) = items + assert len(list(item.iter_markers(name="foo"))) == 1 + + +def test_pedantic_scopes_markers_to_definition(pytester: Pytester) -> None: + """In pedantic mode function-level markers live on the definition node.""" + from _pytest.python import FunctionDefinition + + pytester.makeini("[pytest]\ncollect_function_definition = pedantic\n") + pytester.makepyfile( + """ + import pytest + + @pytest.mark.foo + def test_it(): + pass + """ + ) + (item,) = pytester.inline_genitems()[0] + assert isinstance(item.parent, FunctionDefinition) + assert [m.name for m in item.own_markers] == [] + assert [m.name for m in item.parent.own_markers] == ["foo"] + + +def test_messy_transfers_markers_to_invocation(pytester: Pytester) -> None: + """In messy mode markers are transferred back onto the invocation.""" + from _pytest.python import FunctionDefinition + + pytester.makeini("[pytest]\ncollect_function_definition = messy\n") + pytester.makepyfile( + """ + import pytest + + @pytest.mark.foo + def test_it(): + pass + """ + ) + (item,) = pytester.inline_genitems()[0] + assert isinstance(item.parent, FunctionDefinition) + assert [m.name for m in item.own_markers] == ["foo"] + # Transferred away from the definition scope to avoid duplication. + assert [m.name for m in item.parent.own_markers] == [] + + +def test_messy_emits_header_warning(sample: Pytester) -> None: + _set_mode(sample, "messy") + result = sample.runpytest() + result.stdout.fnmatch_lines(["*collect_function_definition=messy*"]) + + +def test_pedantic_has_no_header_warning(sample: Pytester) -> None: + _set_mode(sample, "pedantic") + result = sample.runpytest() + result.stdout.no_fnmatch_line("*collect_function_definition=messy*") + + +def test_invalid_value_errors(sample: Pytester) -> None: + _set_mode(sample, "bogus") + result = sample.runpytest() + result.stderr.fnmatch_lines(["*Unknown collect_function_definition: 'bogus'*"]) + assert result.ret != 0