Possible Bug in package scoped fixture teardown

[s0undt3ch]

Please consider the following diff against PyTest 4.1:

diff --git a/testing/python/fixture.py b/testing/python/fixture.py
index b6692ac9..7b59cef0 100644
--- a/testing/python/fixture.py
+++ b/testing/python/fixture.py
@@ -3792,7 +3792,11 @@ class TestScopeOrdering(object):
             ├── sub1
             │   ├── __init__.py
             │   ├── conftest.py
-            │   └── test_1.py
+            │   ├── test_1.py
+            │   └── sub1_2
+            │       ├── __init__.py
+            │       ├── conftest.py
+            │       └── test_1_2.py
             └── sub2
                 ├── __init__.py
                 ├── conftest.py
@@ -3824,6 +3828,30 @@ class TestScopeOrdering(object):
         """
             )
         )
+        sub1_2 = sub1.mkdir("sub1_2")
+        sub1_2.ensure("__init__.py")
+        sub1_2.join("conftest.py").write(
+            textwrap.dedent(
+                """\
+            import pytest
+            from ... import values
+            @pytest.fixture(scope="package")
+            def fix():
+                values.append("pre-sub1_2")
+                yield values
+                assert values.pop() == "pre-sub1_2"
+        """
+            )
+        )
+        sub1_2.join("test_1_2.py").write(
+            textwrap.dedent(
+                """\
+            from ... import values
+            def test_1_2(fix):
+                assert values == ["pre-sub1_2"]
+        """
+            )
+        )
         sub2 = root.mkdir("sub2")
         sub2.ensure("__init__.py")
         sub2.join("conftest.py").write(
@@ -3849,7 +3877,103 @@ class TestScopeOrdering(object):
             )
         )
         reprec = testdir.inline_run()
-        reprec.assertoutcome(passed=2)
+        reprec.assertoutcome(passed=3)
+
+    def test_multiple_packages_inherit(self, testdir):
+        """Complex test involving multiple package fixtures. Make sure teardowns
+        are executed in order.
+        .
+        └── root
+            ├── __init__.py
+            ├── sub1
+            │   ├── __init__.py
+            │   ├── conftest.py
+            │   ├── test_1.py
+            │   └── sub1_2
+            │       ├── __init__.py
+            │       ├── conftest.py
+            │       └── test_1_2.py
+            └── sub2
+                ├── __init__.py
+                ├── conftest.py
+                └── test_2.py
+        """
+        root = testdir.mkdir("root")
+        root.join("__init__.py").write("values = []")
+        sub1 = root.mkdir("sub1")
+        sub1.ensure("__init__.py")
+        sub1.join("conftest.py").write(
+            textwrap.dedent(
+                """\
+            import pytest
+            from .. import values
+            @pytest.fixture(scope="package")
+            def fix():
+                values.append("pre-sub1")
+                yield values
+                assert values.pop() == "pre-sub1"
+        """
+            )
+        )
+        sub1.join("test_1.py").write(
+            textwrap.dedent(
+                """\
+            from .. import values
+            def test_1(fix):
+                assert values == ["pre-sub1"]
+        """
+            )
+        )
+        sub1_2 = sub1.mkdir("sub1_2")
+        sub1_2.ensure("__init__.py")
+        sub1_2.join("conftest.py").write(
+            textwrap.dedent(
+                """\
+            import pytest
+            from ... import values
+            @pytest.fixture(scope="package")
+            def fix(fix):
+                values.append("pre-sub1_2")
+                yield values
+                assert values.pop() == "pre-sub1_2"
+        """
+            )
+        )
+        sub1_2.join("test_1_2.py").write(
+            textwrap.dedent(
+                """\
+            from ... import values
+            def test_1_2(fix):
+                assert values == ["pre-sub1", "pre-sub1_2"]
+        """
+            )
+        )
+        sub2 = root.mkdir("sub2")
+        sub2.ensure("__init__.py")
+        sub2.join("conftest.py").write(
+            textwrap.dedent(
+                """\
+            import pytest
+            from .. import values
+            @pytest.fixture(scope="package")
+            def fix():
+                values.append("pre-sub2")
+                yield values
+                assert values.pop() == "pre-sub2"
+        """
+            )
+        )
+        sub2.join("test_2.py").write(
+            textwrap.dedent(
+                """\
+            from .. import values
+            def test_2(fix):
+                assert values == ["pre-sub2"]
+        """
+            )
+        )
+        reprec = testdir.inline_run()
+        reprec.assertoutcome(passed=3)

 def test_call_fixture_function_error():

The difference in the tests is that on test_multiple_packages, there's no fixture reuse/extend, while on test_multiple_packages_inherit I "extend" the fix fixture.

On test_multiple_packages_inherit, at teardown, I expect sub_1_2 to cleanup first, then sub_1, but the test fails at sub_2 because it still contains the item added in sub_1(which didn't ran it's teardown code yet?

[s0undt3ch]

Unless, of course, my ordering assumptions are wrong.

[Zac-HD]

I don't know enough about fixtures to comment on the details, but you'd probably have better luck opening a pull request to propose changes.

[s0undt3ch]

I shouldn't propose changes if the behaviour is what's expected, hence this pre-PR issue. Closed and unanswered...

[s0undt3ch]

Also, the above is just proposing a test shiv will fail, the fix is yet unknown. Don't even know if its desirable, if it is, this shouldn't have been closed.

[nicoddemus]

@s0undt3ch indeed looks like a bug, thanks for reporting it.

[RomanPylypenkoDevPro]

I have the similar issue with "scope=package" when I want to group my test in a few sub-folders using one common conftest. In the case below, the teardown of package_1/conftest runs at the end of the session just after teardown of package_2/conftest:

        .
        └── root
            ├── __init__.py
            ├── package_1
            │   ├── __init__.py
            │   ├── conftest.py
            │   └── sub_package1
            │       ├── __init__.py
            │       └── test_1.py
            └── package_2
                 ├── __init__.py
                 ├── conftest.py
                 └── test_2.py

But at the same time the issue is not reproduced if there are no any nested tests in sub-packages like:

        .
        └── root
            ├── __init__.py
            ├── package_1
            │   ├── __init__.py
            │   ├── conftest.py
            │   └── test_1.py
            └── package_2
                 ├── __init__.py
                 ├── conftest.py
                 └── test_2.py

Now the order of fixtures execution is correct. Full code of example with issue reproduced: https://github.com/RomanPylypenkoDevPro/Sandbox

I hope this info will be useful.

[s0undt3ch]

I've been trying to address this issue. My current idea is that each FixtureDef will store each nodeid where it's used. Then instead of relying on the schedule finalization routine, we check if there aren't any more node IDs for said FixtureDef and in said case, we run the fixture finalize method immediately.

Which at least suggests how fixture teardown should be, ie, if not going to be used anymore, finalize it.

Now, it breaks a lot of tests, even an acceptance test which just passes a tempdir to pytest.main() and is supposed to rerun an exit code indicating no tests were collected...

Would you agree that this fixture termination is the desired one but we've just been relying on deferred termination routines?

Is this the wrong approach?

[shughes-uk]

Hi there I ran into this issue today. Thanks @RomanPylypenkoDevPro for explaining it. I guess the workaround for now is to avoid sub packages?

[s0undt3ch]

This is a hack but this and this is what we're doing to force fixture termination once no test is using it.

[s0undt3ch]

Is it fair to say that if we are in package scope A and we enter package B which is a sub-package of A, any fixtures in the package A scope should not be teardown? I want to work on this but guidelines on where PyTest would like this to go is preferable in order not to waste time.

I still think it's useful to teardown fixtures when they are no longer going to be used, but this "usage tracking" might not be preferable.

[s0undt3ch]

New diff for tests against current master https://gist.github.com/s0undt3ch/021d86c62ab2c4824abea23b1cc9c5f5#file-fixtures-diff-L16

test_multiple_packages_complex_2 fails of course

[thenx]

Guess the bug is still there.

Env:

─➤  pip list 
Package             Version                   Editable project location
------------------- ------------------------- ----------------------------
attrs               22.2.0
exceptiongroup      1.1.0
iniconfig           2.0.0
packaging           23.0
pip                 22.3.1
pluggy              1.0.0
pytest              7.2.1
pytest-asyncio      0.20.3
setuptools          67.0.0
tomli               2.0.1
wheel               0.38.4

Folder structure:

─➤  tree tests                
tests
├── __init__.py
├── example
│   ├── __init__.py
│   ├── conftest.py
│   └── inner
│       ├── __init__.py
│       └── test_inner.py
└── outer
    ├── __init__.py
    └── test_outer.py

Content of conftest.py:

import asyncio
from collections.abc import AsyncIterator, Iterable

import pytest

@pytest.fixture(scope="package")
def event_loop() -> Iterable[asyncio.AbstractEventLoop]:
    policy = asyncio.get_event_loop_policy()
    loop = policy.new_event_loop()
    yield loop
    loop.close()

@pytest.fixture(scope="package")
async def async_package_fixture() -> AsyncIterator[None]:
    yield

Content of test_inner.py:

async def test_inner(async_package_fixture: None) -> None:
    pass

Content of test_outer.py:

async def test_outer() -> None:
    pass

Output of pytest --setup-show:

─➤  pytest --setup-show
=============================================================================== test session starts ===============================================================================
platform darwin -- Python 3.10.9, pytest-7.2.1, pluggy-1.0.0
rootdir: <rootdir>, configfile: pyproject.toml, testpaths: tests
plugins: asyncio-0.20.3
asyncio: mode=auto
collected 2 items                                                                                                                                                                 

tests/example/inner/test_inner.py 
  SETUP    P event_loop
  SETUP    P async_package_fixture (fixtures used: event_loop)
        tests/example/inner/test_inner.py::test_inner (fixtures used: async_package_fixture, event_loop, request).
tests/outer/test_outer.py 
        SETUP    F event_loop
        tests/outer/test_outer.py::test_outer (fixtures used: event_loop).
        TEARDOWN F event_loop
  TEARDOWN P async_package_fixture
  TEARDOWN P event_loopE

===================================================================================== ERRORS ======================================================================================
_________________________________________________________________________ ERROR at teardown of test_outer _________________________________________________________________________

    def finalizer() -> None:
        """Yield again, to finalize."""

        async def async_finalizer() -> None:
            try:
                await gen_obj.__anext__()
            except StopAsyncIteration:
                pass
            else:
                msg = "Async generator fixture didn't stop."
                msg += "Yield only once."
                raise ValueError(msg)

>       event_loop.run_until_complete(async_finalizer())

.venv/lib/python3.10/site-packages/pytest_asyncio/plugin.py:297: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../.pyenv/versions/3.10.9/lib/python3.10/asyncio/base_events.py:624: in run_until_complete
    self._check_closed()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <_UnixSelectorEventLoop running=False closed=True debug=False>

    def _check_closed(self):
        if self._closed:
>           raise RuntimeError('Event loop is closed')
E           RuntimeError: Event loop is closed

../../.pyenv/versions/3.10.9/lib/python3.10/asyncio/base_events.py:515: RuntimeError
============================================================================= short test summary info =============================================================================
ERROR tests/outer/test_outer.py::test_outer - RuntimeError: Event loop is closed
=========================================================================== 2 passed, 1 error in 0.06s ============================================================================
sys:1: RuntimeWarning: coroutine '_wrap_asyncgen_fixture.<locals>._asyncgen_fixture_wrapper.<locals>.finalizer.<locals>.async_finalizer' was never awaited
RuntimeWarning: Enable tracemalloc to get the object allocation traceback

Guess the main info is the following:

tests/example/inner/test_inner.py 
  SETUP    P event_loop
  SETUP    P async_package_fixture (fixtures used: event_loop)
        tests/example/inner/test_inner.py::test_inner (fixtures used: async_package_fixture, event_loop, request).
tests/outer/test_outer.py 
        SETUP    F event_loop
        tests/outer/test_outer.py::test_outer (fixtures used: event_loop).
        TEARDOWN F event_loop
  TEARDOWN P async_package_fixture
  TEARDOWN P event_loopE

There's no teardown for package-scoped event_loop before setup/teardown of function-scoped event_loop. The consequence of it is an attempt to close closed event_loop.

[RonnyPfannschmidt]

Currently we have the issue that multi level package scope is practically undefined and not safely fixable without major reworks

[bluetech]

I haven't tested the exact scenarios, but I believe this should be fixed with #11646. Please test pip install pytest==8.0.0rc1 and let us know if not!

[thenx]

Confirm that my example is resolved in 8.0.0rc1