From e1c66ab0ad8eda13e5552dfc939e07d7290ecd39 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Tue, 5 Dec 2023 23:07:06 +0200 Subject: [PATCH] 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. --- src/_pytest/main.py | 40 +--------------------------------------- src/_pytest/python.py | 3 +-- src/_pytest/runner.py | 19 ++++++++++++++++++- 3 files changed, 20 insertions(+), 42 deletions(-) diff --git a/src/_pytest/main.py b/src/_pytest/main.py index e4ca05aac..3672df05a 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -12,7 +12,6 @@ from typing import Callable 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 @@ class Dir(nodes.Directory): 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 @@ class Dir(nodes.Directory): 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 @@ class Session(nodes.Collector): 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 @@ class Session(nodes.Collector): 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 @@ class Session(nodes.Collector): 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 @@ class Session(nodes.Collector): 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 @@ class Session(nodes.Collector): raise UsageError(*errors) - self._in_genitems = True if not genitems: items = rep.result else: @@ -804,7 +767,6 @@ class Session(nodes.Collector): finally: self._notfound = [] self._initial_parts = [] - self._in_genitems = False self._collection_cache = {} hook.pytest_collection_finish(session=self) diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 09adb2b9c..e0f7a447a 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -731,12 +731,12 @@ class Package(nodes.Directory): 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 @@ class Package(nodes.Directory): 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 diff --git a/src/_pytest/runner.py b/src/_pytest/runner.py index 1b39f93cf..dcfc6b7d0 100644 --- a/src/_pytest/runner.py +++ b/src/_pytest/runner.py @@ -28,6 +28,7 @@ from _pytest._code.code import TerminalRepr 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"