Merge pull request #11138 from bluetech/pkg-mod

python: change `Package` to no longer be a `Module`/`File`

Author: bluetech

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).

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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

src/_pytest/cacheprovider.py

@@ -228,12 +228,7 @@ def pytest_make_collect_report(
 
             # 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 @@ def __init__(self, lfplugin: "LFPlugin") -> None:
     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
 

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:

src/_pytest/main.py

@@ -17,9 +17,9 @@
 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 SetupState
 
 
+if TYPE_CHECKING:
+    from _pytest.python import Package
+
+
 def pytest_addoption(parser: Parser) -> None:
     parser.addini(
         "norecursedirs",
@@ -572,6 +576,17 @@ def _recurse(self, direntry: "os.DirEntry[str]") -> bool:
             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 @@ def perform_collect(  # noqa: F811
         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,63 +704,57 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]:
         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))
             self.trace.root.indent += 1
 
             # 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
-
-                    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]]
+            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 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()
-                for direntry in visit(argpath, self._recurse):
-                    if not direntry.is_file():
-                        continue
+                if argpath in pkg_roots:
+                    yield pkg_roots[argpath]
 
+                for direntry in visit(argpath, self._recurse):
                     path = Path(direntry.path)
-                    dirpath = path.parent
-
-                    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):
+                    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
+
+                    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 @@ def collect(self) -> Iterator[Union[nodes.Item, nodes.Collector]]:
                     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