Different fix for conftest loading

--- Current main

In current main (before pervious commit), calls to gethookproxy/ihook
are the trigger for loading non-initial conftests. This basically means
that conftest loading is done almost as a random side-effect,
uncontrolled and very non-obvious. And it also dashes any hope of making
gethookproxy faster (gethookproxy shows up prominently in pytest
profiles).

I've wanted to improve this for a while, #11268 was the latest step
towards that.

--- PR before change

In this PR, I ran into a problem.

Previously, Session and Package did all of the directory traversals
inside of their collect, which loaded the conftests as a side effect. If
the conftest loading failed, it will occur inside of the collect() and
cause it to be reported as a failure.

Now I've changed things such that Session.collect and Package.collect no
longer recurse, but just collect their immediate descendants, and
genitems does the recursive expansion work.

The problem though is that genitems() doesn't run inside of specific
collector's collect context. So when it loads a conftest, and the
conftest loading fails, the exception isn't handled by any CollectReport
and causes an internal error instead.

The way I've fixed this problem is by loading the conftests eagerly in a
pytest_collect_directory post-wrapper, but only during genitems to make
sure the directory is actually selected.

This solution in turn caused the conftests to be collected too early;
specifically, the plugins are loaded during the parent's collect(), one
after the other as the directory entries are collected. So when the
ihook is hoisted out of the loop, new plugins are loaded inside the
loop, and due to the way the hook proxy works, they are added to the
ihook even though they're supposed to be scoped to the child collectors.
So no hoisting.

--- PR after change

Now I've come up with a better solution: since now the collection tree
actually reflects the filesystem tree, what we really want is to load
the conftest of a directory right before we run its collect(). A
conftest can affect a directory's collect() (e.g. with a
pytest_ignore_collect hookimpl), but it cannot affect how the directory
node itself is collected. So I just moved the conftest loading to be
done right before calling collect, but still inside the CollectReport
context.

This allows the hoisting, and also removes conftest loading from
gethookproxy since it's no longer necessary. And it will probably enable
further cleanups. So I'm happy with it.

Author: bluetech

src/_pytest/main.py

@@ -12,7 +12,6 @@
 from typing import Dict
 from typing import final
 from typing import FrozenSet
-from typing import Generator
 from typing import Iterable
 from typing import Iterator
 from typing import List
@@ -502,11 +501,11 @@ def collect(self) -> Iterable[Union[nodes.Item, nodes.Collector]]:
         config = self.config
         col: Optional[nodes.Collector]
         cols: Sequence[nodes.Collector]
+        ihook = self.ihook
         for direntry in scandir(self.path):
             if direntry.is_dir():
                 if direntry.name == "__pycache__":
                     continue
-                ihook = self.ihook
                 path = Path(direntry.path)
                 if not self.session.isinitpath(path, with_parents=True):
                     if ihook.pytest_ignore_collect(collection_path=path, config=config):
@@ -516,7 +515,6 @@ def collect(self) -> Iterable[Union[nodes.Item, nodes.Collector]]:
                     yield col
 
             elif direntry.is_file():
-                ihook = self.ihook
                 path = Path(direntry.path)
                 if not self.session.isinitpath(path):
                     if ihook.pytest_ignore_collect(collection_path=path, config=config):
@@ -559,7 +557,6 @@ def __init__(self, config: Config) -> None:
         self._initialpaths_with_parents: FrozenSet[Path] = frozenset()
         self._notfound: List[Tuple[str, Sequence[nodes.Collector]]] = []
         self._initial_parts: List[Tuple[Path, List[str]]] = []
-        self._in_genitems = False
         self._collection_cache: Dict[nodes.Collector, CollectReport] = {}
         self.items: List[nodes.Item] = []
 
@@ -612,29 +609,6 @@ def pytest_runtest_logreport(
 
     pytest_collectreport = pytest_runtest_logreport
 
-    @hookimpl(wrapper=True)
-    def pytest_collect_directory(
-        self,
-    ) -> Generator[None, Optional[nodes.Collector], Optional[nodes.Collector]]:
-        col = yield
-
-        # Eagerly load conftests for the directory.
-        # This is needed because a conftest error needs to happen while
-        # collecting a collector, so it is caught by its CollectReport.
-        # Without this, the conftests are loaded inside of genitems itself
-        # which leads to an internal error.
-        # This should only be done for genitems; if done unconditionally, it
-        # will load conftests for non-selected directories which is to be
-        # avoided.
-        if self._in_genitems and col is not None:
-            self.config.pluginmanager._loadconftestmodules(
-                col.path,
-                self.config.getoption("importmode"),
-                rootpath=self.config.rootpath,
-            )
-
-        return col
-
     def isinitpath(
         self,
         path: Union[str, "os.PathLike[str]"],
@@ -665,15 +639,6 @@ def gethookproxy(self, fspath: "os.PathLike[str]") -> pluggy.HookRelay:
         pm = self.config.pluginmanager
         # Check if we have the common case of running
         # hooks with all conftest.py files.
-        #
-        # TODO: pytest relies on this call to load non-initial conftests. This
-        # is incidental. It will be better to load conftests at a more
-        # well-defined place.
-        pm._loadconftestmodules(
-            path,
-            self.config.getoption("importmode"),
-            rootpath=self.config.rootpath,
-        )
         my_conftestmodules = pm._getconftestmodules(path)
         remove_mods = pm._conftest_plugins.difference(my_conftestmodules)
         proxy: pluggy.HookRelay
@@ -754,7 +719,6 @@ def perform_collect(  # noqa: F811
 
         self._notfound = []
         self._initial_parts = []
-        self._in_genitems = False
         self._collection_cache = {}
         self.items = []
         items: Sequence[Union[nodes.Item, nodes.Collector]] = self.items
@@ -789,7 +753,6 @@ def perform_collect(  # noqa: F811
 
                 raise UsageError(*errors)
 
-            self._in_genitems = True
             if not genitems:
                 items = rep.result
             else:
@@ -804,7 +767,6 @@ def perform_collect(  # noqa: F811
         finally:
             self._notfound = []
             self._initial_parts = []
-            self._in_genitems = False
             self._collection_cache = {}
             hook.pytest_collection_finish(session=self)
 

src/_pytest/python.py

@@ -731,12 +731,12 @@ def sort_key(entry: "os.DirEntry[str]") -> object:
         config = self.config
         col: Optional[nodes.Collector]
         cols: Sequence[nodes.Collector]
+        ihook = self.ihook
         for direntry in scandir(self.path, sort_key):
             if direntry.is_dir():
                 if direntry.name == "__pycache__":
                     continue
                 path = Path(direntry.path)
-                ihook = self.ihook
                 if not self.session.isinitpath(path, with_parents=True):
                     if ihook.pytest_ignore_collect(collection_path=path, config=config):
                         continue
@@ -746,7 +746,6 @@ def sort_key(entry: "os.DirEntry[str]") -> object:
 
             elif direntry.is_file():
                 path = Path(direntry.path)
-                ihook = self.ihook
                 if not self.session.isinitpath(path):
                     if ihook.pytest_ignore_collect(collection_path=path, config=config):
                         continue

src/_pytest/runner.py

@@ -28,6 +28,7 @@
 from _pytest.config.argparsing import Parser
 from _pytest.deprecated import check_ispytest
 from _pytest.nodes import Collector
+from _pytest.nodes import Directory
 from _pytest.nodes import Item
 from _pytest.nodes import Node
 from _pytest.outcomes import Exit
@@ -368,7 +369,23 @@ def pytest_runtest_makereport(item: Item, call: CallInfo[None]) -> TestReport:
 
 
 def pytest_make_collect_report(collector: Collector) -> CollectReport:
-    call = CallInfo.from_call(lambda: list(collector.collect()), "collect")
+    def collect() -> List[Union[Item, Collector]]:
+        # Before collecting, if this is a Directory, load the conftests.
+        # If a conftest import fails to load, it is considered a collection
+        # error of the Directory collector. This is why it's done inside of the
+        # CallInfo wrapper.
+        #
+        # Note: initial conftests are loaded early, not here.
+        if isinstance(collector, Directory):
+            collector.config.pluginmanager._loadconftestmodules(
+                collector.path,
+                collector.config.getoption("importmode"),
+                rootpath=collector.config.rootpath,
+            )
+
+        return list(collector.collect())
+
+    call = CallInfo.from_call(collect, "collect")
     longrepr: Union[None, Tuple[str, int, str], str, TerminalRepr] = None
     if not call.excinfo:
         outcome: Literal["passed", "skipped", "failed"] = "passed"