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

Fix #11137.

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 @@ 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.
+                return node.path in lf_paths
-                if isinstance(node, Package):
-                    path = node.path.parent
-                else:
-                    path = node.path
-                return 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,
+        if isinstance(collector, File):
-        # so don't filter Packages.
-        if isinstance(collector, File) and not isinstance(collector, Package):
             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 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,48 +714,40 @@ 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
 
                 if parent.is_dir():
                     pkginit = parent / "__init__.py"
-                        if pkginit.is_file() and pkginit not in node_cache1:
+                    if pkginit.is_file() and parent not in node_cache1:
-                            col = self._collectfile(pkginit, handle_dupes=False)
+                        pkg = self._collectpackage(parent)
-                            if col:
+                        if pkg is not None:
-                                if isinstance(col[0], Package):
+                            pkg_roots[parent] = pkg
-                                    pkg_roots[parent] = col[0]
+                            node_cache1[pkg.path] = [pkg]
-                                node_cache1[col[0].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.
             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:
+                    elif direntry.is_file():
-                        # Collect packages first.
+                        if path.parent in pkg_roots:
-                        seen_dirs.add(dirpath)
+                            # Package handles this file.
-                        pkginit = dirpath / "__init__.py"
-                        if pkginit.exists():
-                            for x in self._collectfile(pkginit):
-                                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:
@@ -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

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(nodes.FSCollector):
-class Package(Module):
     """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(
+        yield from self._collectfile(self.path / "__init__.py")
-            self.path, self.config.getini("python_files")
-        ):
-            yield Module.from_parent(self, path=self.path)
 
         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)