Rework Session and Package collection #11646

bluetech · 2023-11-27T21:47:19Z

This PR implements the proposals in #7777.

RonnyPfannschmidt

its amazing to see this massive undertaking take shape

there are some detail devils and i noted 2 nitpicks

i will try to do some deeper digging into the python collector changes

i'll avoid bikesheding the Dir/Directory naming - naming rights for the implementor !

testing/test_config.py

RonnyPfannschmidt · 2023-11-29T22:03:25Z

testing/test_collection.py

@@ -489,7 +489,7 @@ def test_collect_topdir(self, pytester: Pytester) -> None:
        # assert root2 == rcol, rootid
        colitems = rcol.perform_collect([rcol.nodeid], genitems=False)
        assert len(colitems) == 1
-        assert colitems[0].path == p
+        assert colitems[0].path == topdir


this one seems off - the path there should match the nodeid

The old tree was

<Session> <Module test_collect_topdir.py>

Now it's

<Session> <Dir test_collect_topdir0> <Module test_collect_topdir.py>

So now the collection (with genitems=False) collects the Dir not the Module.

I'm not sure what was the original purpose of this test (it goes back to e2683f4) but the change here is intentional. Do you think there is something that relies on this?

Perform collect towards nodes ids yielded the concrete files, my understanding is that it ought to keep doing that

Do you mean that you think it should not emit Session -> Dir -> Module here, but Session -> Module like before?

I'll try to explain the rationale. Consider for example

repo/ pytest.ini tests/ test_it.py

Before this PR, the tree is

<Module tests/test_it.py> <Function test_it>

but if I add a file tests/__init__.py (i.e. make tests a package), it becomes

<Package tests> <Module test_it.py> <Function test_it>

The idea in this PR is to remove the inconsistency with Package. In this PR, without __init__.py the tree is

<Dir repo> <Dir tests> <Module test_it.py> <Function test_it>

and with __init__.py

<Dir repo> <Package tests> <Module test_it.py> <Function test_it>

Now Package is the python-plugin-ified Directory.

The purpose of that function is to go from a list of nodeids/paths to the initial collectors

Aka the concrete "top-level" fs collectors that are files

The alternative is to ensure all directory collection is limited to filtering of the initial args

OK I understand what you're saying now. perform_collect still does this in this PR.

The test is a bit weird. Basically it runs session.perform_collect([""]). What is the initial collector for this? From a quick test, pytest treats this like pytest . i.e. current directory. So the new behavior seems right to me. Previously it picked out the test file simply because it was the first and there was no intermediary node.

For illustration, if I add an __init__.py file in main (not this PR),

diff --git a/testing/test_collection.py b/testing/test_collection.py index ca2e2b731..424bb9933 100644 --- a/testing/test_collection.py +++ b/testing/test_collection.py @@ -478,6 +478,7 @@ class TestCustomConftests: class TestSession: def test_collect_topdir(self, pytester: Pytester) -> None: p = pytester.makepyfile("def test_func(): pass") + pytester.makepyfile(__init__="") id = "::".join([p.name, "test_func"]) # XXX migrate to collectonly? (see below) config = pytester.parseconfig(id)

then colitems[0] is the Package.

testing/acceptance_test.py

nicoddemus

Awesome work @bluetech! Thanks a lot for working on this. 👍

Left a bunch of minor suggestions, please take a look.

nicoddemus · 2023-12-01T12:56:48Z

src/_pytest/main.py

+            if direntry.is_dir():
+                if direntry.name == "__pycache__":
+                    continue
+                ihook = self.ihook


Given self.ihook is a property which calls self.session.gethookproxy, would it be better to move this outside the loop?

ihook returns a FSHookProxy with a static remove_mods, and is thus very sensitive to its placement. It's something I'm hoping to improve in the future (though it's not easy to make something fast for this in pluggy), but with how pytest is now ihook cannot be safely hoisted.

i believe within the loop it is safe as the remove_mods depends on self.fspath

IIRC the exact problem was with conftest plugins that are loaded after the ihook and are then not filtered out by remove_mods

my rough understanding is, with the usage if directories, conftests don't get added until after the collect phase,
as such it presents as safe from my perspective

do we consider conftests in packages at their construction or at their collection?

After taking a second and third look at this, I've made a change which removes an ugly hack from this PR, allows the hoisting, and removes conftest loading from gethookproxy << this has been a real pain in my neck.

Recap (a little complicated, sorry):

Current main

In current main, 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

changelog/7777.breaking.rst

doc/en/example/customdirectory.rst

src/_pytest/main.py

src/_pytest/python.py

nicoddemus · 2023-12-01T14:15:57Z

src/_pytest/python.py

+        def sort_key(entry: "os.DirEntry[str]") -> object:
+            return (entry.name != "__init__.py", entry.name)
+
+        config = self.config


From this point onward Dir.collect and Package.collect are essentially identical: can we move this to a common function somewhere and reuse it in both places? They are non-trivial and worth avoiding the duplication IMHO.

I live by the Rule of three 😀

But more seriously, I started out with this, with some common private function on Directory, but this stuff shouldn't be on Directory so I relented.

My goal is to simplify this loop such that it doesn't need deduplication because it's simple enough (which will also help plugins to implement it correctly). This things that pop out are:

The __pycache__ check can move to pytest_ignore_collect -- slight performance hit but probably minor.

If we handle the ihook issue we can hoist it out.

If Path(direntry.path) is not super slow we can hoist it out

Maybe the isinitpath check can move to pytest_ignore_collect as well?

Sounds good!

bluetech · 2023-12-02T22:33:06Z

Thanks for the review @nicoddemus, I pushed a commit addressing some your comments, and replied to the others (let me know if I missed any).

nicoddemus

LGTM! There's one minor comment to address, but is minor and up to you. 👍

Great work on this, this clears up a lot of the code and makes the collection tree much more sane and predictable. 👍

src/_pytest/nodes.py

Will be used in upcoming commit.

Fix pytest-dev#7777.

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

bluetech · 2023-12-10T15:04:31Z

Had to rebase due to some conflicts. I left the "Different fix for conftest loading" separate, I think it will be useful to have the diff in the history.

bluetech · 2023-12-30T10:31:39Z

@RonnyPfannschmidt do you intend to review this or should I just merge it? It's currently holding the pytest 8.0 release so it would be good to resolve it one way or the other.

RonnyPfannschmidt · 2023-12-30T10:45:38Z

Im Still down with a Cold, so don't block on me

bluetech · 2023-12-30T10:51:26Z

OK, hope you'll feel better soon!

BeyondEvil · 2024-01-06T12:01:33Z

This PR made some of my tests start to fail with PermissionError: [Errno 13] Permission denied: '/tmp/snap-private-tmp/__init__.py'

https://github.com/pytest-dev/pytest-metadata/actions/runs/7365057317/job/20046008077

The tests pass against c7ee5599, but fail on this merge.

Running it locally I also get some warnings:

warnings

=============================== warnings summary ===============================
../../../../../../../../../Users/jimbrannlund/dev/pytest/pytest/src/_pytest/config/__init__.py:1211
  /Users/jimbrannlund/dev/pytest/pytest/src/_pytest/config/__init__.py:1211: PytestAssertRewriteWarning: Module already imported so cannot be rewritten: pytest_metadata
    self._mark_plugins_for_rewrite(hook)

../../../../../../../../../Users/jimbrannlund/dev/pytest/pytest/src/_pytest/config/__init__.py:1211
  /Users/jimbrannlund/dev/pytest/pytest/src/_pytest/config/__init__.py:1211: PytestAssertRewriteWarning: Module already imported so cannot be rewritten: tests
    self._mark_plugins_for_rewrite(hook)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

I tried to figure out exactly where it goes wrong, but failed. It's entirely possible there's something wrong in my plugin that was just surfaced by the changes in this PR.

Please let me know what information I can contribute with.

Ping @bluetech

bluetech · 2024-01-06T12:04:26Z

@BeyondEvil thanks for testing, I'll check it. Can you report it as a separate issue please?

BeyondEvil · 2024-01-06T12:05:16Z

@BeyondEvil thanks for testing, I'll check it. Can you report it as a separate issue please?

Sure, I wasn't sure if I should do that given it's not a release, just a commit off main branch. 😅

BeyondEvil · 2024-01-06T12:12:44Z

#11781

bluetech mentioned this pull request Nov 27, 2023

How should package collection work? #7777

Closed

bluetech force-pushed the pkg-collect branch from 10832d7 to 0e62302 Compare November 27, 2023 21:59

bluetech added this to the 8.0 milestone Nov 27, 2023

bluetech force-pushed the pkg-collect branch 5 times, most recently from 6de3469 to 39f8ae6 Compare November 29, 2023 11:45

RonnyPfannschmidt requested changes Nov 29, 2023

View reviewed changes

nicoddemus requested changes Dec 1, 2023

View reviewed changes

nicoddemus approved these changes Dec 5, 2023

View reviewed changes

src/_pytest/nodes.py Show resolved Hide resolved

bluetech added 3 commits December 10, 2023 16:32

main: add with_parents parameter to isinitpath

f411c8d

Will be used in upcoming commit.

Rework Session and Package collection

385796b

Fix pytest-dev#7777.

bluetech force-pushed the pkg-collect branch from 36b7ebe to e1c66ab Compare December 10, 2023 15:01

bluetech requested a review from RonnyPfannschmidt December 10, 2023 15:06

bluetech merged commit acd445a into pytest-dev:main Dec 30, 2023
23 of 24 checks passed

bluetech deleted the pkg-collect branch December 30, 2023 10:51

bluetech mentioned this pull request Jan 6, 2024

rootdir for pytester identified as /tmp when passing a tmpfile in /tmp as part of a argument #11781

Open

4 tasks

bluetech mentioned this pull request Jan 14, 2024

main,python: move __pycache__ ignore to pytest_ignore_collect #11814

Merged

The-Compiler mentioned this pull request Feb 6, 2024

8.0.0 reverses order of parametrize when calling individual tests #11937

Closed

4 tasks

bluetech mentioned this pull request Feb 9, 2024

main: fix reversed collection order in Session #11957

Merged

bluetech mentioned this pull request Feb 17, 2024

Package scoped fixture will not execute teardown #8189

Open

nonatomiclabs mentioned this pull request Mar 6, 2024

Issue with duplicates handling in Pytest 8 #12083

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Session and Package collection #11646

Rework Session and Package collection #11646

bluetech commented Nov 27, 2023

RonnyPfannschmidt left a comment

RonnyPfannschmidt Nov 29, 2023

bluetech Nov 30, 2023

RonnyPfannschmidt Nov 30, 2023

bluetech Nov 30, 2023

RonnyPfannschmidt Dec 1, 2023

bluetech Dec 2, 2023

nicoddemus left a comment

nicoddemus Dec 1, 2023

bluetech Dec 2, 2023

RonnyPfannschmidt Dec 2, 2023

bluetech Dec 2, 2023

RonnyPfannschmidt Dec 3, 2023

bluetech Dec 5, 2023

nicoddemus Dec 1, 2023

bluetech Dec 2, 2023

nicoddemus Dec 5, 2023

bluetech commented Dec 2, 2023

nicoddemus left a comment

bluetech commented Dec 10, 2023

bluetech commented Dec 30, 2023

RonnyPfannschmidt commented Dec 30, 2023

bluetech commented Dec 30, 2023

BeyondEvil commented Jan 6, 2024

bluetech commented Jan 6, 2024

BeyondEvil commented Jan 6, 2024 •

edited

Loading

BeyondEvil commented Jan 6, 2024

Rework Session and Package collection #11646

Rework Session and Package collection #11646

Conversation

bluetech commented Nov 27, 2023

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Current main

PR before change

PR after change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluetech commented Dec 2, 2023

nicoddemus left a comment

Choose a reason for hiding this comment

bluetech commented Dec 10, 2023

bluetech commented Dec 30, 2023

RonnyPfannschmidt commented Dec 30, 2023

bluetech commented Dec 30, 2023

BeyondEvil commented Jan 6, 2024

bluetech commented Jan 6, 2024

BeyondEvil commented Jan 6, 2024 • edited Loading

BeyondEvil commented Jan 6, 2024

BeyondEvil commented Jan 6, 2024 •

edited

Loading