From fa97dca0eced535e6b23663f8b2b7b8b61d47fac Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Sat, 17 Jan 2026 15:54:40 +0100 Subject: [PATCH] WIP: Migrate Node __init__ logic to from_parent factory methods This refactoring moves derivation logic from Node __init__ methods to from_parent factory methods, making constructors simpler (assignment-only). Changes: - Node.from_parent now computes config/session from parent - FSCollector.from_parent now computes name/nodeid from path - Item.from_parent performs diamond inheritance check before construction - Session.from_config handles plugin registration post-construction - Package, Function, DoctestItem made cooperative (accept **kwargs) - Removed legacy fspath parameter support (was PytestRemovedIn9Warning) - Removed non-cooperative constructor fallback in NodeMeta._create - Updated tests to use cooperative constructors Note: Diamond inheritance (Item + Collector) warning registration needs to be addressed in a separate PR before this can be finalized. Co-authored-by: Cursor AI Co-authored-by: Anthropic Claude --- src/_pytest/doctest.py | 4 +- src/_pytest/main.py | 6 +- src/_pytest/nodes.py | 175 ++++++++++++++----------------------- src/_pytest/python.py | 26 +++--- testing/test_collection.py | 4 +- testing/test_nodes.py | 31 ++----- 6 files changed, 97 insertions(+), 149 deletions(-) diff --git a/src/_pytest/doctest.py b/src/_pytest/doctest.py index cd255f5eeb6..8b2fa1b0f78 100644 --- a/src/_pytest/doctest.py +++ b/src/_pytest/doctest.py @@ -253,10 +253,12 @@ def __init__( self, name: str, parent: DoctestTextfile | DoctestModule, + *, runner: doctest.DocTestRunner, dtest: doctest.DocTest, + **kw, ) -> None: - super().__init__(name, parent) + super().__init__(name, parent, **kw) self.runner = runner self.dtest = dtest diff --git a/src/_pytest/main.py b/src/_pytest/main.py index 02c7fb373fd..6debafc1e00 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -591,7 +591,6 @@ def __init__(self, config: Config) -> None: super().__init__( name="", path=config.rootpath, - fspath=None, parent=None, config=config, session=self, @@ -611,11 +610,12 @@ def __init__(self, config: Config) -> None: self._bestrelpathcache: dict[Path, str] = _bestrelpath_cache(config.rootpath) - self.config.pluginmanager.register(self, name="session") - @classmethod def from_config(cls, config: Config) -> Session: + """The public constructor for Session.""" session: Session = cls._create(config=config) + # Register session as a plugin after construction. + config.pluginmanager.register(session, name="session") return session def __repr__(self) -> str: diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py index 6690f6ab1f8..472e08e63c7 100644 --- a/src/_pytest/nodes.py +++ b/src/_pytest/nodes.py @@ -12,7 +12,6 @@ import pathlib from pathlib import Path from typing import Any -from typing import cast from typing import NoReturn from typing import overload from typing import TYPE_CHECKING @@ -28,11 +27,8 @@ from _pytest._code.code import Traceback from _pytest._code.code import TracebackStyle from _pytest.compat import LEGACY_PATH -from _pytest.compat import signature from _pytest.config import Config from _pytest.config import ConftestImportFailure -from _pytest.config.compat import _check_path -from _pytest.deprecated import NODE_CTOR_FSPATH_ARG from _pytest.mark.structures import Mark from _pytest.mark.structures import MarkDecorator from _pytest.mark.structures import NodeKeywords @@ -56,28 +52,6 @@ _T = TypeVar("_T") - -def _imply_path( - node_type: type[Node], - path: Path | None, - fspath: LEGACY_PATH | None, -) -> Path: - if fspath is not None: - warnings.warn( - NODE_CTOR_FSPATH_ARG.format( - node_type_name=node_type.__name__, - ), - stacklevel=6, - ) - if path is not None: - if fspath is not None: - _check_path(path, fspath) - return path - else: - assert fspath is not None - return Path(fspath) - - _NodeType = TypeVar("_NodeType", bound="Node") @@ -106,23 +80,7 @@ def __call__(cls, *k, **kw) -> NoReturn: fail(msg, pytrace=False) def _create(cls: type[_T], *k, **kw) -> _T: - try: - return super().__call__(*k, **kw) # type: ignore[no-any-return,misc] - except TypeError: - sig = signature(getattr(cls, "__init__")) - known_kw = {k: v for k, v in kw.items() if k in sig.parameters} - from .warning_types import PytestDeprecationWarning - - warnings.warn( - PytestDeprecationWarning( - f"{cls} is not using a cooperative constructor and only takes {set(known_kw)}.\n" - "See https://docs.pytest.org/en/stable/deprecations.html" - "#constructors-of-custom-pytest-node-subclasses-should-take-kwargs " - "for more details." - ) - ) - - return super().__call__(*k, **known_kw) # type: ignore[no-any-return,misc] + return super().__call__(*k, **kw) # type: ignore[no-any-return,misc] class Node(abc.ABC, metaclass=NodeMeta): @@ -159,7 +117,6 @@ def __init__( parent: Node | None = None, config: Config | None = None, session: Session | None = None, - fspath: LEGACY_PATH | None = None, path: Path | None = None, nodeid: str | None = None, ) -> None: @@ -169,26 +126,23 @@ def __init__( #: The parent collector node. self.parent = parent - if config: - #: The pytest config object. - self.config: Config = config - else: - if not parent: - raise TypeError("config or parent must be provided") - self.config = parent.config + if config is None: + raise TypeError("config must be provided") + #: The pytest config object. + self.config: Config = config - if session: - #: The pytest session this node is part of. - self.session: Session = session - else: - if not parent: - raise TypeError("session or parent must be provided") - self.session = parent.session + if session is None: + raise TypeError("session must be provided") + #: The pytest session this node is part of. + self.session: Session = session - if path is None and fspath is None: + # Path - for Item nodes, inherit from parent if not provided. + if path is None and parent is not None: path = getattr(parent, "path", None) - #: Filesystem path where this node was collected from (can be None). - self.path: pathlib.Path = _imply_path(type(self), path, fspath=fspath) + if path is None: + raise TypeError("path must be provided") + #: Filesystem path where this node was collected from. + self.path: pathlib.Path = path # The explicit annotation is to avoid publicly exposing NodeKeywords. #: Keywords/markers collected from all scopes. @@ -200,13 +154,14 @@ def __init__( #: Allow adding of extra keywords to use for matching. self.extra_keyword_matches: set[str] = set() + # Nodeid - for Item nodes, compute from parent if not provided. if nodeid is not None: assert "::()" not in nodeid self._nodeid = nodeid + elif parent is not None: + self._nodeid = parent.nodeid + "::" + self.name else: - if not self.parent: - raise TypeError("nodeid or parent must be provided") - self._nodeid = self.parent.nodeid + "::" + self.name + raise TypeError("nodeid or parent must be provided") #: A place where plugins can store information on the node for their #: own use. @@ -230,7 +185,12 @@ def from_parent(cls, parent: Node, **kw) -> Self: raise TypeError("config is not a valid argument for from_parent") if "session" in kw: raise TypeError("session is not a valid argument for from_parent") - return cls._create(parent=parent, **kw) + + # Derive config and session from parent. + config = parent.config + session = parent.session + + return cls._create(parent=parent, config=config, session=session, **kw) @property def ihook(self) -> pluggy.HookRelay: @@ -562,49 +522,17 @@ class FSCollector(Collector, abc.ABC): def __init__( self, - fspath: LEGACY_PATH | None = None, - path_or_parent: Path | Node | None = None, - path: Path | None = None, - name: str | None = None, + *, + path: Path, + name: str, parent: Node | None = None, config: Config | None = None, session: Session | None = None, nodeid: str | None = None, + **kw, ) -> None: - if path_or_parent: - if isinstance(path_or_parent, Node): - assert parent is None - parent = cast(FSCollector, path_or_parent) - elif isinstance(path_or_parent, Path): - assert path is None - path = path_or_parent - - path = _imply_path(type(self), path, fspath=fspath) - if name is None: - name = path.name - if parent is not None and parent.path != path: - try: - rel = path.relative_to(parent.path) - except ValueError: - pass - else: - name = str(rel) - name = name.replace(os.sep, SEP) self.path = path - if session is None: - assert parent is not None - session = parent.session - - if nodeid is None: - try: - nodeid = str(self.path.relative_to(session.config.rootpath)) - except ValueError: - nodeid = _check_initialpaths_for_relpath(session._initialpaths, path) - - if nodeid and os.sep != SEP: - nodeid = nodeid.replace(os.sep, SEP) - super().__init__( name=name, parent=parent, @@ -612,6 +540,7 @@ def __init__( session=session, nodeid=nodeid, path=path, + **kw, ) @classmethod @@ -619,12 +548,40 @@ def from_parent( cls, parent, *, - fspath: LEGACY_PATH | None = None, path: Path | None = None, **kw, ) -> Self: """The public constructor.""" - return super().from_parent(parent=parent, fspath=fspath, path=path, **kw) + if path is None: + raise TypeError("path argument is required") + # Compute name from path if not provided. + if "name" not in kw: + name = path.name + if parent.path != path: + try: + rel = path.relative_to(parent.path) + except ValueError: + pass + else: + name = str(rel) + name = name.replace(os.sep, SEP) + kw["name"] = name + + # Compute nodeid from path if not provided. + if "nodeid" not in kw: + try: + nodeid: str | None = str( + path.relative_to(parent.session.config.rootpath) + ) + except ValueError: + nodeid = _check_initialpaths_for_relpath( + parent.session._initialpaths, path + ) + if nodeid and os.sep != SEP: + nodeid = nodeid.replace(os.sep, SEP) + kw["nodeid"] = nodeid + + return super().from_parent(parent=parent, path=path, **kw) class File(FSCollector, abc.ABC): @@ -687,15 +644,19 @@ def __init__( #: for this test. self.user_properties: list[tuple[str, object]] = [] - self._check_item_and_collector_diamond_inheritance() + @classmethod + def from_parent(cls, parent: Node, **kw) -> Self: + """The public constructor.""" + # Check for diamond inheritance before construction. + cls._check_item_and_collector_diamond_inheritance() + return super().from_parent(parent=parent, **kw) - def _check_item_and_collector_diamond_inheritance(self) -> None: + @classmethod + def _check_item_and_collector_diamond_inheritance(cls) -> None: """ Check if the current type inherits from both File and Collector at the same time, emitting a warning accordingly (#8447). """ - cls = type(self) - # We inject an attribute in the type to avoid issuing this warning # for the same class more than once, which is not helpful. # It is a hack, but was deemed acceptable in order to avoid diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 7374fa3cee0..c79a04b2283 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -45,7 +45,6 @@ from _pytest.compat import get_real_func from _pytest.compat import getimfunc from _pytest.compat import is_async_function -from _pytest.compat import LEGACY_PATH from _pytest.compat import NOTSET from _pytest.compat import safe_getattr from _pytest.compat import safe_isclass @@ -654,24 +653,21 @@ class Package(nodes.Directory): def __init__( self, - fspath: LEGACY_PATH | None, - parent: nodes.Collector, - # NOTE: following args are unused: - config=None, - session=None, - nodeid=None, - path: Path | None = None, + *, + path: Path, + parent: nodes.Collector | None = None, + config: Config | None = None, + session: Session | None = None, + nodeid: str | None = None, + **kw, ) -> None: - # NOTE: Could be just the following, but kept as-is for compat. - # super().__init__(self, fspath, parent=parent) - session = parent.session super().__init__( - fspath=fspath, path=path, parent=parent, config=config, session=session, nodeid=nodeid, + **kw, ) def setup(self) -> None: @@ -1592,8 +1588,9 @@ def __init__( session: Session | None = None, fixtureinfo: FuncFixtureInfo | None = None, originalname: str | None = None, + **kw, ) -> None: - super().__init__(name, parent, config=config, session=session) + super().__init__(name, parent, config=config, session=session, **kw) if callobj is not NOTSET: self._obj = callobj @@ -1625,6 +1622,8 @@ def __init__( if keywords: self.keywords.update(keywords) + # Fixtureinfo is typically passed by from_parent. + # Fallback to computing for backward compat with non-cooperative callers. if fixtureinfo is None: fm = self.session._fixturemanager fixtureinfo = fm.getfixtureinfo(self, self.obj, self.cls) @@ -1632,7 +1631,6 @@ def __init__( self.fixturenames = fixtureinfo.names_closure self._initrequest() - # todo: determine sound type limitations @classmethod def from_parent(cls, parent, **kw) -> Self: """The public constructor.""" diff --git a/testing/test_collection.py b/testing/test_collection.py index 39753d80cac..18105f7ed6a 100644 --- a/testing/test_collection.py +++ b/testing/test_collection.py @@ -1611,8 +1611,8 @@ def test_class_from_parent(request: FixtureRequest) -> None: """Ensure Class.from_parent can forward custom arguments to the constructor.""" class MyCollector(pytest.Class): - def __init__(self, name, parent, x): - super().__init__(name, parent) + def __init__(self, *, x, **kw): + super().__init__(**kw) self.x = x @classmethod diff --git a/testing/test_nodes.py b/testing/test_nodes.py index de7875ca427..38f4ed10379 100644 --- a/testing/test_nodes.py +++ b/testing/test_nodes.py @@ -2,11 +2,9 @@ from __future__ import annotations from pathlib import Path -import re import warnings from _pytest import nodes -from _pytest.compat import legacy_path from _pytest.outcomes import OutcomeException from _pytest.pytester import Pytester from _pytest.warning_types import PytestWarning @@ -33,21 +31,18 @@ def test_node_direct_construction_deprecated() -> None: nodes.Node(None, session=None) # type: ignore[arg-type] -def test_subclassing_both_item_and_collector_deprecated( - request, tmp_path: Path -) -> None: +def test_subclassing_both_item_and_collector_warns(request, tmp_path: Path) -> None: """ - Verifies we warn on diamond inheritance as well as correctly managing legacy - inheritance constructors with missing args as found in plugins. + Verifies we warn on diamond inheritance (Item + Collector) during from_parent. + Diamond inheritance is not supported and will fail at construction time. """ # We do not expect any warnings messages to issued during class definition. with warnings.catch_warnings(): warnings.simplefilter("error") class SoWrong(nodes.Item, nodes.File): - def __init__(self, fspath, parent): - """Legacy ctor with legacy call # don't wana see""" - super().__init__(fspath, parent) + def __init__(self, **kw): + super().__init__(**kw) def collect(self): raise NotImplementedError() @@ -55,18 +50,10 @@ def collect(self): def runtest(self): raise NotImplementedError() - with pytest.warns(PytestWarning) as rec: - SoWrong.from_parent( - request.session, fspath=legacy_path(tmp_path / "broken.txt") - ) - messages = [str(x.message) for x in rec] - assert any( - re.search(".*SoWrong.* not using a cooperative constructor.*", x) - for x in messages - ) - assert any( - re.search("(?m)SoWrong .* should not be a collector", x) for x in messages - ) + # Diamond inheritance will warn and then fail due to MRO issues. + with pytest.warns(PytestWarning, match="should not be a collector"): + with pytest.raises(TypeError): + SoWrong.from_parent(request.session, path=tmp_path / "broken.txt") @pytest.mark.parametrize(