From d7b0e172052d855afe444c599330c907cdc53d93 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Thu, 24 Jun 2021 11:45:32 +0200 Subject: [PATCH] issue a warning when Item and Collector are used in diamond inheritance (#8447) * issue a warning when Items and Collector form a diamond addresses #8435 * Apply suggestions from code review Co-authored-by: Ran Benita * Return support for the broken File/Item hybrids * adds deprecation * ads necessary support code in node construction * fix incorrect mypy based assertions * add docs for deprecation of Item/File inheritance * warn when a non-cooperative ctor is encountered * use getattr instead of cast to get the class __init__ for legacy ctors * update documentation references for node inheritance * clean up file+item inheritance test enhance docs move import upwards Co-authored-by: Ran Benita --- changelog/8447.deprecation.rst | 4 ++ doc/en/deprecations.rst | 14 ++++++ src/_pytest/main.py | 2 +- src/_pytest/nodes.py | 88 +++++++++++++++++++++++++++------- testing/test_nodes.py | 31 ++++++++++++ 5 files changed, 122 insertions(+), 17 deletions(-) create mode 100644 changelog/8447.deprecation.rst diff --git a/changelog/8447.deprecation.rst b/changelog/8447.deprecation.rst new file mode 100644 index 00000000000..d1011f8c849 --- /dev/null +++ b/changelog/8447.deprecation.rst @@ -0,0 +1,4 @@ +Defining a custom pytest node type which is both an item and a collector now issues a warning. +It was never sanely supported and triggers hard to debug errors. + +Instead, a separate collector node should be used, which collects the item. See :ref:`non-python tests` for an example. diff --git a/doc/en/deprecations.rst b/doc/en/deprecations.rst index c3020fb9498..cf501d50921 100644 --- a/doc/en/deprecations.rst +++ b/doc/en/deprecations.rst @@ -42,6 +42,20 @@ As pytest tries to move off `py.path.local None: @classmethod def from_config(cls, config: Config) -> "Session": - session: Session = cls._create(config) + session: Session = cls._create(config=config) return session def __repr__(self) -> str: diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py index 40bc4d4965b..4d12f07a27a 100644 --- a/src/_pytest/nodes.py +++ b/src/_pytest/nodes.py @@ -1,8 +1,10 @@ import os import warnings +from inspect import signature from pathlib import Path from typing import Any from typing import Callable +from typing import cast from typing import Iterable from typing import Iterator from typing import List @@ -34,6 +36,7 @@ from _pytest.pathlib import absolutepath from _pytest.pathlib import commonpath from _pytest.store import Store +from _pytest.warning_types import PytestWarning if TYPE_CHECKING: # Imported here due to circular import. @@ -125,7 +128,20 @@ def __call__(self, *k, **kw): fail(msg, pytrace=False) def _create(self, *k, **kw): - return super().__call__(*k, **kw) + try: + return super().__call__(*k, **kw) + except TypeError: + sig = signature(getattr(self, "__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"{self} is not using a cooperative constructor and only takes {set(known_kw)}" + ) + ) + + return super().__call__(*k, **known_kw) class Node(metaclass=NodeMeta): @@ -539,26 +555,39 @@ def _check_initialpaths_for_relpath(session: "Session", path: Path) -> Optional[ class FSCollector(Collector): def __init__( self, - fspath: Optional[LEGACY_PATH], - path: Optional[Path], - parent=None, + fspath: Optional[LEGACY_PATH] = None, + path_or_parent: Optional[Union[Path, Node]] = None, + path: Optional[Path] = None, + name: Optional[str] = None, + parent: Optional[Node] = None, config: Optional[Config] = None, session: Optional["Session"] = None, nodeid: Optional[str] = None, ) -> 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, fspath = _imply_path(path, fspath=fspath) - 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) + 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 - session = session or parent.session + if session is None: + assert parent is not None + session = parent.session if nodeid is None: try: @@ -570,7 +599,12 @@ def __init__( nodeid = nodeid.replace(os.sep, SEP) super().__init__( - name, parent, config, session, nodeid=nodeid, fspath=fspath, path=path + name=name, + parent=parent, + config=config, + session=session, + nodeid=nodeid, + path=path, ) @classmethod @@ -610,6 +644,20 @@ class Item(Node): nextitem = None + def __init_subclass__(cls) -> None: + problems = ", ".join( + base.__name__ for base in cls.__bases__ if issubclass(base, Collector) + ) + if problems: + warnings.warn( + f"{cls.__name__} is an Item subclass and should not be a collector, " + f"however its bases {problems} are collectors.\n" + "Please split the Collectors and the Item into separate node types.\n" + "Pytest Doc example: https://docs.pytest.org/en/latest/example/nonpython.html\n" + "example pull request on a plugin: https://github.com/asmeurer/pytest-flakes/pull/40/", + PytestWarning, + ) + def __init__( self, name, @@ -617,8 +665,16 @@ def __init__( config: Optional[Config] = None, session: Optional["Session"] = None, nodeid: Optional[str] = None, + **kw, ) -> None: - super().__init__(name, parent, config, session, nodeid=nodeid) + super().__init__( + name=name, + parent=parent, + config=config, + session=session, + nodeid=nodeid, + **kw, + ) self._report_sections: List[Tuple[str, str, str]] = [] #: A list of tuples (name, value) that holds user defined properties diff --git a/testing/test_nodes.py b/testing/test_nodes.py index 9104c1c0588..52cd0173c7d 100644 --- a/testing/test_nodes.py +++ b/testing/test_nodes.py @@ -5,6 +5,7 @@ import pytest from _pytest import nodes +from _pytest.compat import legacy_path from _pytest.pytester import Pytester from _pytest.warning_types import PytestWarning @@ -39,6 +40,36 @@ def test_node_from_parent_disallowed_arguments() -> None: nodes.Node.from_parent(None, config=None) # type: ignore[arg-type] +def test_subclassing_both_item_and_collector_deprecated( + request, tmp_path: Path +) -> None: + """ + Verifies we warn on diamond inheritance + as well as correctly managing legacy inheritance ctors with missing args + as found in plugins + """ + + with pytest.warns( + PytestWarning, + match=( + "(?m)SoWrong is an Item subclass and should not be a collector, however its bases File are collectors.\n" + "Please split the Collectors and the Item into separate node types.\n.*" + ), + ): + + class SoWrong(nodes.File, nodes.Item): + def __init__(self, fspath, parent): + """Legacy ctor with legacy call # don't wana see""" + super().__init__(fspath, parent) + + with pytest.warns( + PytestWarning, match=".*SoWrong.* not using a cooperative constructor.*" + ): + SoWrong.from_parent( + request.session, fspath=legacy_path(tmp_path / "broken.txt") + ) + + @pytest.mark.parametrize( "warn_type, msg", [(DeprecationWarning, "deprecated"), (PytestWarning, "pytest")] )