config: split _getconftestmodules and _loadconftestmodules
#11268
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, the `_getconftestmodules` function was used both to load conftest modules for a path (during `pytest_load_initial_conftests`), and to retrieve conftest modules for a path (during hook dispatch and for fetching `collect_ignore`). This made things muddy - it is usually nicer to have clear separation between "command" and "query" functions, when they occur in separate phases. So split into "load" and "get". Currently, `gethookproxy` still loads conftest itself. I hope to change this in the future.
nicoddemus
approved these changes
Aug 7, 2023
|
This broke pytest-doctestplus (https://github.com/scientific-python/pytest-doctestplus) (I haven't yet looked into how and why, whether we misuse a private functionality (as this PR has no changelog mention, etc) or something is wrong in the PR , but noticed the failure as I wanted to tag a new release) |
bluetech
added a commit
to bluetech/pytest
that referenced
this pull request
Dec 10, 2023
--- 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, pytest-dev#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the
_getconftestmodulesfunction was used both to load conftest modules for a path (duringpytest_load_initial_conftests), and to retrieve conftest modules for a path (during hook dispatch and for fetchingcollect_ignore). This made things muddy - it is usually nicer to have clear separation between "command" and "query" functions, when they occur in separate phases.So split into "load" and "get".
Currently,
gethookproxystill loads conftest itself. I hope to change this in the future.