Prevent INTERNALERROR in pytest_collectstart when accessing module with pytest.skip

[seifertm]

What's the problem this feature will solve?

Accessing pytest.Module.obj triggers a module import. If this happens during pytest_collectstart and the module that is about to be collected contains a pytest.skip statement, the result will be an INTERNALERROR. This is either caused by the Skipped exception or by Collector.CollectError if the allow_module_level keyword is missing in the pytest.skip call.

There's no good way to deal with this in the hook, because test outcomes, such as the Skipped exception are not part of the public API.

Describe the solution you'd like

I would like that no INTERNALERROR is triggered when accessing Module.obj in pytest_collectstart

Real world use case: Dynamically attaching a fixture to a module, similar to what's done in Module._inject_setup_module_fixture:

pytest/src/_pytest/python.py

Lines 564 to 577 in 2390610

	@fixtures.fixture(
	autouse=True,
	scope="module",
	# Use a unique name to speed up lookup.
	name=f"_xunit_setup_module_fixture_{self.obj.__name__}",
	)
	def xunit_setup_module_fixture(request) -> Generator[None, None, None]:
	if setup_module is not None:
	_call_with_optional_argument(setup_module, request.module)
	yield
	if teardown_module is not None:
	_call_with_optional_argument(teardown_module, request.module)
	self.obj.__pytest_setup_module = xunit_setup_module_fixture

Alternative Solutions

A workaround is to surround the access to Module.obj with a try-except. This forces the import of a non public symbol:

from pytest import Collector
from _pytest.outcomes import OutcomeException

def pytest_collectstart(collector):
    try:
        collector.obj
    except (OutcomeException, Collector.CollectError):
        pass

https://github.com/pytest-dev/pytest-asyncio/blob/a214c3e77149608d427ccab69140edb509c67697/pytest_asyncio/plugin.py#L592-L619

[RonnyPfannschmidt]

based on the purpose of the hooks, they are indeed the wrong place to do the work

i believe the best way to handle this would be to introduce a way for a node to collect its own factories

then both the pytest internal ones as well as pytest-asyncio could add factories in a sensible manner without using the wrong hook

CC @bluetech @nicoddemus

[seifertm]

i believe the best way to handle this would be to introduce a way for a node to collect its own factories

then both the pytest internal ones as well as pytest-asyncio could add factories in a sensible manner without using the wrong hook

This sounds like a great idea to me! At least it would solve my original problem. Do you have any thoughts on how the design should look like?

Here's my current understanding of your proposal:
The FixtureManager is currently in charge for introspecting a Node (or underlying object) and creating the appropriate FixtureDefs. This logic should largely move to the Node class. The upside is that there'll be a way to dynamically add fixtures, [0] both in pytest internally and in plugins. One possible downside is that the concept of fixtures is currently implemented as a plugin. Moving the parsefactories logic to Node will couple fixtures more tightly to the "base" functionality of pytest.

Side question as I'm not familiar with the naming: Are factories and fixtures synonymous? Or are there factories that aren't fixtures?`

[0] see also #6101 which proposes a different approach but a similar goal

[bluetech]

Experiment with a "fixture registration" API: https://github.com/bluetech/pytest/commits/register-fixture/
Currently it's a private _register_fixture method on FixtureManager, so doubly private. I do not mean for the public API to be this, but just to get the discussion going.

I converted the internal usages of the "fixture injection hack" to use that instead, which seems to work well.

Regarding pytest-asyncio

I then tried to convert pytest-asyncio to use _register_fixture to see how it works out.

See diff

diff --git a/pytest_asyncio/plugin.py b/pytest_asyncio/plugin.py
index eb013f4..87e91f5 100644
--- a/pytest_asyncio/plugin.py
+++ b/pytest_asyncio/plugin.py
@@ -588,10 +588,6 @@ def pytest_collectstart(collector: pytest.Collector):
     event_loop_fixture_id = f"{collector.nodeid}::<event_loop>"
     collector.stash[_event_loop_fixture_id] = event_loop_fixture_id
 
-    @pytest.fixture(
-        scope=collector_scope,
-        name=event_loop_fixture_id,
-    )
     def scoped_event_loop(
         *args,  # Function needs to accept "cls" when collected by pytest.Class
         event_loop_policy,
@@ -604,41 +600,12 @@ def pytest_collectstart(collector: pytest.Collector):
             yield loop
             loop.close()
 
-    # @pytest.fixture does not register the fixture anywhere, so pytest doesn't
-    # know it exists. We work around this by attaching the fixture function to the
-    # collected Python object, where it will be picked up by pytest.Class.collect()
-    # or pytest.Module.collect(), respectively
-    if type(collector) is Module:
-        # Accessing Module.obj triggers a module import executing module-level
-        # statements. A module-level pytest.skip statement raises the "Skipped"
-        # OutcomeException or a Collector.CollectError, if the "allow_module_level"
-        # kwargs is missing. These cases are handled correctly when they happen inside
-        # Collector.collect(), but this hook runs before the actual collect call.
-        # Therefore, we monkey patch Module.collect to add the scoped fixture to the
-        # module before it runs the actual collection.
-        def _patched_collect():
-            collector.obj.__pytest_asyncio_scoped_event_loop = scoped_event_loop
-            return collector.__original_collect()
-
-        collector.__original_collect = collector.collect
-        collector.collect = _patched_collect
-    else:
-        pyobject = collector.obj
-        # If the collected module is a DoctestTextfile, collector.obj is None
-        if pyobject is None:
-            return
-        pyobject.__pytest_asyncio_scoped_event_loop = scoped_event_loop
-    # When collector is a package, collector.obj is the package's __init__.py.
-    # pytest doesn't seem to collect fixtures in __init__.py.
-    # Using parsefactories to collect fixtures in __init__.py their baseid will end
-    # with "__init__.py", thus limiting the scope of the fixture to the init module.
-    # Therefore, we tell the pluginmanager explicitly to collect the fixtures
-    # in the init module, but strip "__init__.py" from the baseid
-    # Possibly related to https://github.com/pytest-dev/pytest/issues/4085
-    if isinstance(collector, Package):
-        fixturemanager = collector.config.pluginmanager.get_plugin("funcmanage")
-        package_node_id = _removesuffix(collector.nodeid, "__init__.py")
-        fixturemanager.parsefactories(collector.obj, nodeid=package_node_id)
+    collector.session._fixturemanager._register_fixture(
+        name=event_loop_fixture_id,
+        func=scoped_event_loop,
+        scope=collector_scope,
+        nodeid=collector.nodeid,
+    )
 
 
 def _removesuffix(s: str, suffix: str) -> str:

however ran into two issues with cause some package-scoped event loop fixture tests to fail:

• In pytest 8, the nodeid of a Package is just its path. Some tests have the top-level dir as the package, and in this case the Package nodeid is ., while the nodeid of the Module under this Package is e.g. test_it.py. The problem is that pytest's "is this fixture visible to this node" check (see matchfactories function) is pretty stupid -- it basically just checks if the fixture's baseid is a prefix of the nodeid, and . is not a prefix of test_it.py. But this is a pytest issue that presumably can be fixed (e.g. like conftests handle it, though this is slightly broken as well -- becomes too visible).

  It should be said that pytest currently basically doesn't provide any way to register fixtures directly on the Package itself which is why this issue never came up.

• In pytest 8, Packages are now properly nested in the collection tree (see How should package collection work? #7777). And the question is how the scoped event loops should behave for nested/sub packages. Some tests fail due to this IIUC. I don't actually use async and particularly pytest-asyncio myself so I don't know how it should work, but I think whatever the desired behavior is, it can be implemented with some extra checks.

[nicoddemus]

Reviewed the code, and the register_fixture function is a great idea @bluetech!

[bluetech]

Reviewed the code, and the register_fixture function is a great idea @bluetech!

Great, I'll submit it then. While we still need to work out a public API, this provides a basis for discussion and improves things internally so no reason to hold it.

---

Regarding the Package . nodeid fixture matching issue, I've prepared a fix for this here: https://github.com/bluetech/pytest/commits/matchfactories-nodes/

It's still WIP, I need to handle the backward compat and write a changelog, but it's a nice cleanup even regardless of the bug fix.

[seifertm]

@bluetech Regardless of whether there will eventually be a public API for this functionality, I wanted to thank you for your efforts and for going above and beyond by testing your fixture registration code with pytest-asyncio.