diff --git a/changelog/8037.deprecation.rst b/changelog/8037.deprecation.rst new file mode 100644 index 00000000000..a543d00f179 --- /dev/null +++ b/changelog/8037.deprecation.rst @@ -0,0 +1 @@ +Deprecate implying node ctor arguments. diff --git a/changelog/8037.feature.rst b/changelog/8037.feature.rst new file mode 100644 index 00000000000..205ee4ea726 --- /dev/null +++ b/changelog/8037.feature.rst @@ -0,0 +1 @@ +Introduce ``Node.fs_path`` as pathlib replacement for ``Node.fspath`` diff --git a/src/_pytest/deprecated.py b/src/_pytest/deprecated.py index 19b31d66538..75a6c3a7d61 100644 --- a/src/_pytest/deprecated.py +++ b/src/_pytest/deprecated.py @@ -64,6 +64,12 @@ PRIVATE = PytestDeprecationWarning("A private pytest class or function was used.") +NODE_IMPLIED_ARG = UnformattedWarning( + PytestDeprecationWarning, + "implying {type.__name__}.{arg} from parent.{arg} has been deprecated\n" + "please use the Node.from_parent to have them be implied by the superclass named ctors", +) + # You want to make some `__init__` or function "private". # diff --git a/src/_pytest/doctest.py b/src/_pytest/doctest.py index 255ca80b913..18b2b6d5f5d 100644 --- a/src/_pytest/doctest.py +++ b/src/_pytest/doctest.py @@ -254,8 +254,9 @@ def __init__( parent: "Union[DoctestTextfile, DoctestModule]", runner: Optional["doctest.DocTestRunner"] = None, dtest: Optional["doctest.DocTest"] = None, + **kw: Any, ) -> None: - super().__init__(name, parent) + super().__init__(name=name, parent=parent, **kw) self.runner = runner self.dtest = dtest self.obj = None diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 43a40a86449..0d218d9d4e4 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -662,7 +662,7 @@ def _compute_fixture_value(self, fixturedef: "FixtureDef[object]") -> None: "\n\nRequested here:\n{}:{}".format( funcitem.nodeid, fixturedef.argname, - getlocation(fixturedef.func, funcitem.config.rootdir), + getlocation(fixturedef.func, str(funcitem.config.rootpath)), source_path_str, source_lineno, ) diff --git a/src/_pytest/main.py b/src/_pytest/main.py index 5036601f9bb..ef7d468d8e4 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -464,7 +464,12 @@ class Session(nodes.FSCollector): def __init__(self, config: Config) -> None: super().__init__( - config.rootdir, parent=None, config=config, session=self, nodeid="" + fs_path=config.rootpath, + name="", + parent=None, + config=config, + session=self, + nodeid="", ) self.testsfailed = 0 self.testscollected = 0 @@ -688,7 +693,7 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]: if col: if isinstance(col[0], Package): pkg_roots[str(parent)] = col[0] - node_cache1[Path(col[0].fspath)] = [col[0]] + node_cache1[col[0].fs_path] = [col[0]] # If it's a directory argument, recurse and look for any Subpackages. # Let the Package collector deal with subnodes, don't collect here. @@ -717,7 +722,7 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]: continue for x in self._collectfile(path): - key2 = (type(x), Path(x.fspath)) + key2 = (type(x), x.fs_path) if key2 in node_cache2: yield node_cache2[key2] else: @@ -749,12 +754,12 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]: continue if not isinstance(node, nodes.Collector): continue - key = (type(node), node.nodeid) - if key in matchnodes_cache: - rep = matchnodes_cache[key] + key_matchnodes = (type(node), node.nodeid) + if key_matchnodes in matchnodes_cache: + rep = matchnodes_cache[key_matchnodes] else: rep = collect_one_node(node) - matchnodes_cache[key] = rep + matchnodes_cache[key_matchnodes] = rep if rep.passed: submatchnodes = [] for r in rep.result: diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py index 2a96d55ad05..21fb3e090d2 100644 --- a/src/_pytest/nodes.py +++ b/src/_pytest/nodes.py @@ -26,6 +26,7 @@ from _pytest.config import Config from _pytest.config import ConftestImportFailure from _pytest.deprecated import FSCOLLECTOR_GETHOOKPROXY_ISINITPATH +from _pytest.deprecated import NODE_IMPLIED_ARG from _pytest.mark.structures import Mark from _pytest.mark.structures import MarkDecorator from _pytest.mark.structures import NodeKeywords @@ -110,45 +111,67 @@ class Node(metaclass=NodeMeta): "parent", "config", "session", - "fspath", + "fs_path", "_nodeid", "_store", "__dict__", ) + name: str + config: Config + session: "Session" + fs_path: Path + _nodeid: str + def __init__( self, + *, name: str, - parent: "Optional[Node]" = None, + parent: Optional["Node"], config: Optional[Config] = None, - session: "Optional[Session]" = None, - fspath: Optional[py.path.local] = None, + session: Optional["Session"] = None, + fs_path: Optional[Path] = None, nodeid: Optional[str] = None, ) -> None: #: A unique name within the scope of the parent node. self.name = name + if nodeid is not None: + assert "::()" not in nodeid + else: + assert parent is not None + nodeid = parent.nodeid + if name != "()": + nodeid = f"{nodeid}::{name}" + warnings.warn(NODE_IMPLIED_ARG.format(type=type(self), arg="nodeid")) + self._nodeid = nodeid #: The parent collector node. self.parent = parent #: The pytest config object. - if config: - self.config: Config = config - else: - if not parent: - raise TypeError("config or parent must be provided") + if config is None: + assert parent is not None self.config = parent.config + warnings.warn(NODE_IMPLIED_ARG.format(type=type(self), arg="config")) + else: + self.config = config #: The pytest session this node is part of. - if session: - self.session = session - else: - if not parent: - raise TypeError("session or parent must be provided") + if session is None: + assert parent is not None self.session = parent.session + warnings.warn(NODE_IMPLIED_ARG.format(type=type(self), arg="session")) + else: + self.session = session + #: Filesystem path where this node was collected from (can be None). - self.fspath = fspath or getattr(parent, "fspath", None) + if fs_path is None: + assert parent is not None + self.fs_path = parent.fs_path + warnings.warn(NODE_IMPLIED_ARG.format(type=type(self), arg="fs_path")) + else: + self.fs_path = fs_path # The explicit annotation is to avoid publicly exposing NodeKeywords. #: Keywords/markers collected from all scopes. @@ -160,22 +183,29 @@ def __init__( #: Allow adding of extra keywords to use for matching. self.extra_keyword_matches: Set[str] = set() - if nodeid is not None: - assert "::()" not in nodeid - self._nodeid = nodeid - else: - if not self.parent: - raise TypeError("nodeid or parent must be provided") - self._nodeid = self.parent.nodeid - if self.name != "()": - self._nodeid += "::" + self.name - # A place where plugins can store information on the node for their # own use. Currently only intended for internal plugins. self._store = Store() + @property + def fspath(self) -> py.path.local: + """Filesystem path where this node was collected from (can be None). + + Since pytest 6.2, prefer to use `fs_path` instead which returns a `pathlib.Path` + instead of a `py.path.local`. + """ + return py.path.local(self.fs_path) + @classmethod - def from_parent(cls, parent: "Node", **kw): + def from_parent( + cls, + parent: "Node", + *, + name: str, + fs_path: Optional[Path] = None, + nodeid: Optional[str] = None, + **kw: Any, + ): """Public constructor for Nodes. This indirection got introduced in order to enable removing @@ -190,7 +220,26 @@ def from_parent(cls, parent: "Node", **kw): 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) + if nodeid is not None: + assert "::()" not in nodeid + else: + nodeid = parent.nodeid + if name != "()": + nodeid = f"{nodeid}::{name}" + + config = parent.config + session = parent.session + if fs_path is None: + fs_path = parent.fs_path + return cls._create( + parent=parent, + config=config, + session=session, + nodeid=nodeid, + name=name, + fs_path=fs_path, + **kw, + ) @property def ihook(self): @@ -495,38 +544,60 @@ def _check_initialpaths_for_relpath( class FSCollector(Collector): - def __init__( - self, - fspath: py.path.local, - parent=None, - config: Optional[Config] = None, - session: Optional["Session"] = None, + def __init__(self, **kw): + + fs_path: Optional[Path] = kw.pop("fs_path", None) + fspath: Optional[py.path.local] = kw.pop("fspath", None) + + if fspath is not None: + assert fs_path is None + fs_path = Path(fspath) + kw["fs_path"] = fs_path + super().__init__(**kw) + + @classmethod + def from_parent( + cls, + parent: Node, + *, + fspath: Optional[py.path.local] = None, + fs_path: Optional[Path] = None, nodeid: Optional[str] = None, - ) -> None: - name = fspath.basename - if parent is not None: + name: Optional[str] = None, + **kw, + ): + """The public constructor.""" + if fspath is not None: + assert ( + fs_path is None + ), "fs_path and fspath cannot both be passed at the same time" + known_path = Path(fspath) + else: + assert fs_path is not None, "fs_path or fspath must be given" + known_path = fs_path + fspath = py.path.local(known_path) + + if name is None: + name = known_path.name rel = fspath.relto(parent.fspath) if rel: - name = rel - name = name.replace(os.sep, SEP) - self.fspath = fspath - - session = session or parent.session + name = str(rel) if nodeid is None: - nodeid = self.fspath.relto(session.config.rootdir) + assert parent is not None + session = parent.session + relp = session._bestrelpathcache[known_path] + if not relp.startswith(".." + os.sep): + nodeid = str(relp) if not nodeid: nodeid = _check_initialpaths_for_relpath(session, fspath) if nodeid and os.sep != SEP: nodeid = nodeid.replace(os.sep, SEP) - super().__init__(name, parent, config, session, nodeid=nodeid, fspath=fspath) - - @classmethod - def from_parent(cls, parent, *, fspath, **kw): - """The public constructor.""" - return super().from_parent(parent=parent, fspath=fspath, **kw) + return super().from_parent( + parent=parent, name=name, fs_path=known_path, nodeid=nodeid, **kw + ) def gethookproxy(self, fspath: "os.PathLike[str]"): warnings.warn(FSCOLLECTOR_GETHOOKPROXY_ISINITPATH, stacklevel=2) @@ -555,12 +626,20 @@ class Item(Node): def __init__( self, name, - parent=None, - config: Optional[Config] = None, - session: Optional["Session"] = None, - nodeid: Optional[str] = None, + parent: Node, + config: Config, + session: "Session", + nodeid: str, + fs_path: Path, ) -> None: - super().__init__(name, parent, config, session, nodeid=nodeid) + super().__init__( + name=name, + parent=parent, + config=config, + session=session, + nodeid=nodeid, + fs_path=fs_path, + ) self._report_sections: List[Tuple[str, str, str]] = [] #: A list of tuples (name, value) that holds user defined properties diff --git a/src/_pytest/pytester.py b/src/_pytest/pytester.py index 95b22b3b23e..e1c3f22dc16 100644 --- a/src/_pytest/pytester.py +++ b/src/_pytest/pytester.py @@ -900,7 +900,7 @@ def copy_example(self, name: Optional[str] = None) -> Path: example_dir = self._request.config.getini("pytester_example_dir") if example_dir is None: raise ValueError("pytester_example_dir is unset, can't copy examples") - example_dir = Path(str(self._request.config.rootdir)) / example_dir + example_dir = self._request.config.rootpath / example_dir for extra_element in self._request.node.iter_markers("pytester_example_path"): assert extra_element.args diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 29ebd176bbb..073dd765e76 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -23,7 +23,6 @@ from typing import Sequence from typing import Set from typing import Tuple -from typing import Type from typing import TYPE_CHECKING from typing import Union @@ -214,7 +213,7 @@ def path_matches_patterns(path: Path, patterns: Iterable[str]) -> bool: def pytest_pycollect_makemodule(fspath: Path, path: py.path.local, parent) -> "Module": if fspath.name == "__init__.py": - pkg: Package = Package.from_parent(parent, fspath=path) + pkg: Package = Package.from_parent(parent, fs_path=fspath) return pkg mod: Module = Module.from_parent(parent, fspath=path) return mod @@ -255,21 +254,9 @@ def pytest_pycollect_makeitem(collector: "PyCollector", name: str, obj: object): return res -class PyobjMixin: +class PyobjMixin(nodes.Node): _ALLOW_MARKERS = True - # Function and attributes that the mixin needs (for type-checking only). - if TYPE_CHECKING: - name: str = "" - parent: Optional[nodes.Node] = None - own_markers: List[Mark] = [] - - def getparent(self, cls: Type[nodes._NodeType]) -> Optional[nodes._NodeType]: - ... - - def listchain(self) -> List[nodes.Node]: - ... - @property def module(self): """Python module object this node was collected from (can be None).""" @@ -497,7 +484,7 @@ def _genfunctions(self, name: str, funcobj) -> Iterator["Function"]: for callspec in metafunc._calls: subname = f"{name}[{callspec.id}]" yield Function.from_parent( - self, + parent=self, name=subname, callspec=callspec, callobj=funcobj, @@ -634,22 +621,26 @@ def _importtestmodule(self): class Package(Module): - def __init__( - self, - fspath: py.path.local, - parent: nodes.Collector, - # NOTE: following args are unused: - config=None, - session=None, - nodeid=None, - ) -> None: - # NOTE: Could be just the following, but kept as-is for compat. - # nodes.FSCollector.__init__(self, fspath, parent=parent) - session = parent.session - nodes.FSCollector.__init__( - self, fspath, parent=parent, config=config, session=session, nodeid=nodeid - ) - self.name = os.path.basename(str(fspath.dirname)) + @classmethod + def from_parent( + cls, + parent, + *, + name=None, + fs_path: Optional[Path] = None, + fspath: Optional[py.path.local] = None, + **kw, + ): + if name is None: + if fs_path is not None: + name = fs_path.parent.name + elif fspath is not None: + fs_path = Path(fspath) + name = fs_path.parent.name + else: + raise TypeError("name required") + + return super().from_parent(parent=parent, name=name, fs_path=fs_path, **kw) def setup(self) -> None: # Not using fixtures to call setup_module here because autouse fixtures @@ -721,7 +712,7 @@ def collect(self) -> Iterable[Union[nodes.Item, nodes.Collector]]: if init_module.is_file() and path_matches_patterns( init_module, self.config.getini("python_files") ): - yield Module.from_parent(self, fspath=py.path.local(init_module)) + yield Module.from_parent(self, fs_path=init_module) pkg_prefixes: Set[Path] = set() for direntry in visit(str(this_path), recurse=self._recurse): path = Path(direntry.path) @@ -1577,16 +1568,21 @@ class Function(PyobjMixin, nodes.Item): def __init__( self, name: str, - parent, - config: Optional[Config] = None, + parent: nodes.Node, + config: Config, callspec: Optional[CallSpec2] = None, callobj=NOTSET, keywords=None, - session: Optional[Session] = None, fixtureinfo: Optional[FuncFixtureInfo] = None, originalname: Optional[str] = None, + *, + session: Session, + nodeid: str, + fs_path: Path, ) -> None: - super().__init__(name, parent, config=config, session=session) + super().__init__( + name, parent, config=config, session=session, nodeid=nodeid, fs_path=fs_path + ) if callobj is not NOTSET: self.obj = callobj @@ -1636,11 +1632,6 @@ def __init__( self.fixturenames = fixtureinfo.names_closure self._initrequest() - @classmethod - def from_parent(cls, parent, **kw): # todo: determine sound type limitations - """The public constructor.""" - return super().from_parent(parent=parent, **kw) - def _initrequest(self) -> None: self.funcargs: Dict[str, object] = {} self._request = fixtures.FixtureRequest(self, _ispytest=True) diff --git a/testing/test_collection.py b/testing/test_collection.py index 3dd9283eced..17aa6540e2c 100644 --- a/testing/test_collection.py +++ b/testing/test_collection.py @@ -1346,14 +1346,10 @@ def test_fscollector_from_parent(pytester: Pytester, request: FixtureRequest) -> """ class MyCollector(pytest.File): - def __init__(self, fspath, parent, x): - super().__init__(fspath, parent) + def __init__(self, *, x, **kw): + super().__init__(**kw) self.x = x - @classmethod - def from_parent(cls, parent, *, fspath, x): - return super().from_parent(parent=parent, fspath=fspath, x=x) - collector = MyCollector.from_parent( parent=request.session, fspath=py.path.local(pytester.path) / "foo", x=10 ) diff --git a/testing/test_nodes.py b/testing/test_nodes.py index 59d9f409eac..8633b592612 100644 --- a/testing/test_nodes.py +++ b/testing/test_nodes.py @@ -34,9 +34,37 @@ def test_iterparentnodeids(nodeid: str, expected: List[str]) -> None: def test_node_from_parent_disallowed_arguments() -> None: with pytest.raises(TypeError, match="session is"): - nodes.Node.from_parent(None, session=None) # type: ignore[arg-type] + nodes.Node.from_parent(None, name="none", session=None) # type: ignore[arg-type] with pytest.raises(TypeError, match="config is"): - nodes.Node.from_parent(None, config=None) # type: ignore[arg-type] + nodes.Node.from_parent(None, name="none", config=None) # type: ignore[arg-type] + + +@pytest.mark.parametrize("absent_arg", ["nodeid", "config", "session", "fs_path"]) +def test_node_warns_implied_ctor_args(pytester, absent_arg): + config = pytester.parseconfigure() + from _pytest.main import Session + + session = Session.from_config(config) + + args = dict( + name="test", + nodeid="test", + session=session, + config=session.config, + fs_path=session.fs_path, + ) + del args[absent_arg] + + with pytest.warns( + PytestWarning, + match=fr"implying Node.{absent_arg} from parent.{absent_arg} has been deprecated\n" + "please use the Node.from_parent to have them be implied by the superclass named ctors", + ): + nodes.Node._create(parent=session, **args) + + +def test_fspath_warns(): + pass @pytest.mark.parametrize(