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.