From a21fb87a90974189c1b8b26189959507189bb3a1 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 2 Jun 2023 16:02:09 +0300 Subject: [PATCH] python: change `Package` to no longer be a `Module`/`File` Fix #11137. --- changelog/11137.breaking.rst | 11 +++ doc/en/deprecations.rst | 16 +++++ src/_pytest/cacheprovider.py | 11 +-- src/_pytest/fixtures.py | 5 +- src/_pytest/main.py | 106 +++++++++++++-------------- src/_pytest/python.py | 134 +++++++++++++++++------------------ 6 files changed, 146 insertions(+), 137 deletions(-) create mode 100644 changelog/11137.breaking.rst diff --git a/changelog/11137.breaking.rst b/changelog/11137.breaking.rst new file mode 100644 index 000000000..a92df326a --- /dev/null +++ b/changelog/11137.breaking.rst @@ -0,0 +1,11 @@ +:class:`pytest.Package` is no longer a :class:`pytest.Module` or :class:`pytest.File`. + +The ``Package`` collector node designates a Python package, that is, a directory with an `__init__.py` file. +Previously ``Package`` was a subtype of ``pytest.Module`` (which represents a single Python module), +the module being the `__init__.py` file. +This has been deemed a design mistake (see :issue:`11137` and :issue:`7777` for details). + +The ``path`` property of ``Package`` nodes now points to the package directory instead of the ``__init__.py`` file. + +Note that a ``Module`` node for ``__init__.py`` (which is not a ``Package``) may still exist, +if it is picked up during collection (e.g. if you configured :confval:`python_files` to include ``__init__.py`` files). diff --git a/doc/en/deprecations.rst b/doc/en/deprecations.rst index e73c1a18e..7b4e99965 100644 --- a/doc/en/deprecations.rst +++ b/doc/en/deprecations.rst @@ -495,6 +495,22 @@ an appropriate period of deprecation has passed. Some breaking changes which could not be deprecated are also listed. +:class:`pytest.Package` is no longer a :class:`pytest.Module` or :class:`pytest.File` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +.. versionchanged:: 8.0 + +The ``Package`` collector node designates a Python package, that is, a directory with an `__init__.py` file. +Previously ``Package`` was a subtype of ``pytest.Module`` (which represents a single Python module), +the module being the `__init__.py` file. +This has been deemed a design mistake (see :issue:`11137` and :issue:`7777` for details). + +The ``path`` property of ``Package`` nodes now points to the package directory instead of the ``__init__.py`` file. + +Note that a ``Module`` node for ``__init__.py`` (which is not a ``Package``) may still exist, +if it is picked up during collection (e.g. if you configured :confval:`python_files` to include ``__init__.py`` files). + + Collecting ``__init__.py`` files no longer collects package ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/_pytest/cacheprovider.py b/src/_pytest/cacheprovider.py index f519c974b..67dee6add 100755 --- a/src/_pytest/cacheprovider.py +++ b/src/_pytest/cacheprovider.py @@ -228,12 +228,7 @@ class LFPluginCollWrapper: # Use stable sort to priorize last failed. def sort_key(node: Union[nodes.Item, nodes.Collector]) -> bool: - # Package.path is the __init__.py file, we need the directory. - if isinstance(node, Package): - path = node.path.parent - else: - path = node.path - return path in lf_paths + return node.path in lf_paths res.result = sorted( res.result, @@ -277,9 +272,7 @@ class LFPluginCollSkipfiles: def pytest_make_collect_report( self, collector: nodes.Collector ) -> Optional[CollectReport]: - # Packages are Files, but we only want to skip test-bearing Files, - # so don't filter Packages. - if isinstance(collector, File) and not isinstance(collector, Package): + if isinstance(collector, File): if collector.path not in self.lfplugin._last_failed_paths: self.lfplugin._skipped_files += 1 diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index c35f842db..f2bf5320c 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -119,9 +119,8 @@ def get_scope_package( from _pytest.python import Package current: Optional[Union[nodes.Item, nodes.Collector]] = node - fixture_package_name = "{}/{}".format(fixturedef.baseid, "__init__.py") while current and ( - not isinstance(current, Package) or fixture_package_name != current.nodeid + not isinstance(current, Package) or current.nodeid != fixturedef.baseid ): current = current.parent # type: ignore[assignment] if current is None: @@ -263,7 +262,7 @@ def get_parametrized_fixture_keys(item: nodes.Item, scope: Scope) -> Iterator[_K if scope is Scope.Session: key: _Key = (argname, param_index) elif scope is Scope.Package: - key = (argname, param_index, item.path.parent) + key = (argname, param_index, item.path) elif scope is Scope.Module: key = (argname, param_index, item.path) elif scope is Scope.Class: diff --git a/src/_pytest/main.py b/src/_pytest/main.py index f27767cf1..4c3c9aed4 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -17,9 +17,9 @@ from typing import Literal from typing import Optional from typing import overload 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 import _pytest._code @@ -43,6 +43,10 @@ from _pytest.runner import collect_one_node from _pytest.runner import SetupState +if TYPE_CHECKING: + from _pytest.python import Package + + def pytest_addoption(parser: Parser) -> None: parser.addini( "norecursedirs", @@ -572,6 +576,17 @@ class Session(nodes.FSCollector): return False return True + def _collectpackage(self, fspath: Path) -> Optional["Package"]: + from _pytest.python import Package + + ihook = self.gethookproxy(fspath) + if not self.isinitpath(fspath): + if ihook.pytest_ignore_collect(collection_path=fspath, config=self.config): + return None + + pkg: Package = Package.from_parent(self, path=fspath) + return pkg + def _collectfile( self, fspath: Path, handle_dupes: bool = True ) -> Sequence[nodes.Collector]: @@ -680,8 +695,6 @@ class Session(nodes.FSCollector): return items def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]: - from _pytest.python import Package - # Keep track of any collected nodes in here, so we don't duplicate fixtures. node_cache1: Dict[Path, Sequence[nodes.Collector]] = {} node_cache2: Dict[Tuple[Type[nodes.Collector], Path], nodes.Collector] = {} @@ -691,7 +704,9 @@ class Session(nodes.FSCollector): matchnodes_cache: Dict[Tuple[Type[nodes.Collector], str], CollectReport] = {} # Directories of pkgs with dunder-init files. - pkg_roots: Dict[Path, Package] = {} + pkg_roots: Dict[Path, "Package"] = {} + + pm = self.config.pluginmanager for argpath, names in self._initial_parts: self.trace("processing argument", (argpath, names)) @@ -699,55 +714,47 @@ class Session(nodes.FSCollector): # Start with a Session root, and delve to argpath item (dir or file) # and stack all Packages found on the way. - # No point in finding packages when collecting doctests. - if not self.config.getoption("doctestmodules", False): - pm = self.config.pluginmanager - for parent in (argpath, *argpath.parents): - if not pm._is_in_confcutdir(argpath): - break + for parent in (argpath, *argpath.parents): + if not pm._is_in_confcutdir(argpath): + break - if parent.is_dir(): - pkginit = parent / "__init__.py" - if pkginit.is_file() and pkginit not in node_cache1: - col = self._collectfile(pkginit, handle_dupes=False) - if col: - if isinstance(col[0], Package): - pkg_roots[parent] = col[0] - node_cache1[col[0].path] = [col[0]] + if parent.is_dir(): + pkginit = parent / "__init__.py" + if pkginit.is_file() and parent not in node_cache1: + pkg = self._collectpackage(parent) + if pkg is not None: + pkg_roots[parent] = pkg + node_cache1[pkg.path] = [pkg] # If it's a directory argument, recurse and look for any Subpackages. # Let the Package collector deal with subnodes, don't collect here. if argpath.is_dir(): assert not names, f"invalid arg {(argpath, names)!r}" - seen_dirs: Set[Path] = set() + if argpath in pkg_roots: + yield pkg_roots[argpath] + for direntry in visit(argpath, self._recurse): - if not direntry.is_file(): - continue - path = Path(direntry.path) - dirpath = path.parent + if direntry.is_dir() and self._recurse(direntry): + pkginit = path / "__init__.py" + if pkginit.is_file(): + pkg = self._collectpackage(path) + if pkg is not None: + yield pkg + pkg_roots[path] = pkg - if dirpath not in seen_dirs: - # Collect packages first. - seen_dirs.add(dirpath) - pkginit = dirpath / "__init__.py" - if pkginit.exists(): - for x in self._collectfile(pkginit): + elif direntry.is_file(): + if path.parent in pkg_roots: + # Package handles this file. + continue + for x in self._collectfile(path): + key2 = (type(x), x.path) + if key2 in node_cache2: + yield node_cache2[key2] + else: + node_cache2[key2] = x yield x - if isinstance(x, Package): - pkg_roots[dirpath] = x - if dirpath in pkg_roots: - # Do not collect packages here. - continue - - for x in self._collectfile(path): - key2 = (type(x), x.path) - if key2 in node_cache2: - yield node_cache2[key2] - else: - node_cache2[key2] = x - yield x else: assert argpath.is_file() @@ -806,21 +813,6 @@ class Session(nodes.FSCollector): self._notfound.append((report_arg, col)) continue - # If __init__.py was the only file requested, then the matched - # node will be the corresponding Package (by default), and the - # first yielded item will be the __init__ Module itself, so - # just use that. If this special case isn't taken, then all the - # files in the package will be yielded. - if argpath.name == "__init__.py" and isinstance(matching[0], Package): - try: - yield next(iter(matching[0].collect())) - except StopIteration: - # The package collects nothing with only an __init__.py - # file in it, which gets ignored by the default - # "python_files" option. - pass - continue - yield from matching self.trace.root.indent -= 1 diff --git a/src/_pytest/python.py b/src/_pytest/python.py index cf7575336..c0c16d4d0 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -204,7 +204,7 @@ def pytest_collect_file(file_path: Path, parent: nodes.Collector) -> Optional["M if file_path.suffix == ".py": if not parent.session.isinitpath(file_path): if not path_matches_patterns( - file_path, parent.config.getini("python_files") + ["__init__.py"] + file_path, parent.config.getini("python_files") ): return None ihook = parent.session.gethookproxy(file_path) @@ -221,9 +221,6 @@ def path_matches_patterns(path: Path, patterns: Iterable[str]) -> bool: def pytest_pycollect_makemodule(module_path: Path, parent) -> "Module": - if module_path.name == "__init__.py": - pkg: Package = Package.from_parent(parent, path=module_path) - return pkg mod: Module = Module.from_parent(parent, path=module_path) return mod @@ -517,11 +514,62 @@ class PyCollector(PyobjMixin, nodes.Collector): ) +def importtestmodule( + path: Path, + config: Config, +): + # We assume we are only called once per module. + importmode = config.getoption("--import-mode") + try: + mod = import_path(path, mode=importmode, root=config.rootpath) + except SyntaxError as e: + raise nodes.Collector.CollectError( + ExceptionInfo.from_current().getrepr(style="short") + ) from e + except ImportPathMismatchError as e: + raise nodes.Collector.CollectError( + "import file mismatch:\n" + "imported module %r has this __file__ attribute:\n" + " %s\n" + "which is not the same as the test file we want to collect:\n" + " %s\n" + "HINT: remove __pycache__ / .pyc files and/or use a " + "unique basename for your test file modules" % e.args + ) from e + except ImportError as e: + exc_info = ExceptionInfo.from_current() + if config.getoption("verbose") < 2: + exc_info.traceback = exc_info.traceback.filter(filter_traceback) + exc_repr = ( + exc_info.getrepr(style="short") + if exc_info.traceback + else exc_info.exconly() + ) + formatted_tb = str(exc_repr) + raise nodes.Collector.CollectError( + "ImportError while importing test module '{path}'.\n" + "Hint: make sure your test modules/packages have valid Python names.\n" + "Traceback:\n" + "{traceback}".format(path=path, traceback=formatted_tb) + ) from e + except skip.Exception as e: + if e.allow_module_level: + raise + raise nodes.Collector.CollectError( + "Using pytest.skip outside of a test will skip the entire module. " + "If that's your intention, pass `allow_module_level=True`. " + "If you want to skip a specific test or an entire class, " + "use the @pytest.mark.skip or @pytest.mark.skipif decorators." + ) from e + config.pluginmanager.consider_module(mod) + return mod + + class Module(nodes.File, PyCollector): """Collector for test classes and functions in a Python module.""" def _getobj(self): - return self._importtestmodule() + return importtestmodule(self.path, self.config) def collect(self) -> Iterable[Union[nodes.Item, nodes.Collector]]: self._inject_setup_module_fixture() @@ -606,55 +654,8 @@ class Module(nodes.File, PyCollector): self.obj.__pytest_setup_function = xunit_setup_function_fixture - def _importtestmodule(self): - # We assume we are only called once per module. - importmode = self.config.getoption("--import-mode") - try: - mod = import_path(self.path, mode=importmode, root=self.config.rootpath) - except SyntaxError as e: - raise self.CollectError( - ExceptionInfo.from_current().getrepr(style="short") - ) from e - except ImportPathMismatchError as e: - raise self.CollectError( - "import file mismatch:\n" - "imported module %r has this __file__ attribute:\n" - " %s\n" - "which is not the same as the test file we want to collect:\n" - " %s\n" - "HINT: remove __pycache__ / .pyc files and/or use a " - "unique basename for your test file modules" % e.args - ) from e - except ImportError as e: - exc_info = ExceptionInfo.from_current() - if self.config.getoption("verbose") < 2: - exc_info.traceback = exc_info.traceback.filter(filter_traceback) - exc_repr = ( - exc_info.getrepr(style="short") - if exc_info.traceback - else exc_info.exconly() - ) - formatted_tb = str(exc_repr) - raise self.CollectError( - "ImportError while importing test module '{path}'.\n" - "Hint: make sure your test modules/packages have valid Python names.\n" - "Traceback:\n" - "{traceback}".format(path=self.path, traceback=formatted_tb) - ) from e - except skip.Exception as e: - if e.allow_module_level: - raise - raise self.CollectError( - "Using pytest.skip outside of a test will skip the entire module. " - "If that's your intention, pass `allow_module_level=True`. " - "If you want to skip a specific test or an entire class, " - "use the @pytest.mark.skip or @pytest.mark.skipif decorators." - ) from e - self.config.pluginmanager.consider_module(mod) - return mod - -class Package(Module): +class Package(nodes.FSCollector): """Collector for files and directories in a Python packages -- directories with an `__init__.py` file.""" @@ -680,22 +681,24 @@ class Package(Module): session=session, nodeid=nodeid, ) - self.name = self.path.parent.name + self.name = self.path.name def setup(self) -> None: + init_mod = importtestmodule(self.path / "__init__.py", self.config) + # Not using fixtures to call setup_module here because autouse fixtures # from packages are not called automatically (#4085). setup_module = _get_first_non_fixture_func( - self.obj, ("setUpModule", "setup_module") + init_mod, ("setUpModule", "setup_module") ) if setup_module is not None: - _call_with_optional_argument(setup_module, self.obj) + _call_with_optional_argument(setup_module, init_mod) teardown_module = _get_first_non_fixture_func( - self.obj, ("tearDownModule", "teardown_module") + init_mod, ("tearDownModule", "teardown_module") ) if teardown_module is not None: - func = partial(_call_with_optional_argument, teardown_module, self.obj) + func = partial(_call_with_optional_argument, teardown_module, init_mod) self.addfinalizer(func) def _recurse(self, direntry: "os.DirEntry[str]") -> bool: @@ -732,21 +735,16 @@ class Package(Module): return ihook.pytest_collect_file(file_path=fspath, parent=self) # type: ignore[no-any-return] def collect(self) -> Iterable[Union[nodes.Item, nodes.Collector]]: - this_path = self.path.parent - # Always collect the __init__ first. - if self.session.isinitpath(self.path) or path_matches_patterns( - self.path, self.config.getini("python_files") - ): - yield Module.from_parent(self, path=self.path) + yield from self._collectfile(self.path / "__init__.py") pkg_prefixes: Set[Path] = set() - for direntry in visit(str(this_path), recurse=self._recurse): + for direntry in visit(self.path, recurse=self._recurse): path = Path(direntry.path) - # We will visit our own __init__.py file, in which case we skip it. + # Already handled above. if direntry.is_file(): - if direntry.name == "__init__.py" and path.parent == this_path: + if direntry.name == "__init__.py" and path.parent == self.path: continue parts_ = parts(direntry.path) @@ -761,7 +759,7 @@ class Package(Module): elif not direntry.is_dir(): # Broken symlink or invalid/missing file. continue - elif path.joinpath("__init__.py").is_file(): + elif self._recurse(direntry) and path.joinpath("__init__.py").is_file(): pkg_prefixes.add(path)