Repository: pytest-dev/pytest
Pull request: Don't add fixture finalizer if the value is cached
Fixes #1489 - see https://github.com/pytest-dev/pytest/issues/1489#issuecomment-1894057766 for explanation of current behavior.

It's possible this might break test setups that (inadvertently) relies on the teardown order being as it is, but I think it's an improvement on the status quo.

The code for setup & teardown of fixtures was very hard to parse, and I added a couple comments and fixed what I think are outdated and now incorrect docstrings. But feels like a more thorough cleanup could be warranted. I have not looked into `git blame` to see if/when/how stuff got changed.

Remaining questions:
* Should finalizer be scheduled if the cached result is an exception? It previously was, I probably lean no. The change does not affect any current tests, but I could maybe write a test that asserts the behavior.
* I never managed to figure out why fixtures adds its finalizer to all fixtures it depends on, nor if this could cause setup/teardown ordering failures (I tried writing tests that would trigger it, but didn't manage to). There's definitely reasons for it though, lots of stuff breaks if I remove that code.
  * I tested moving the loop of adding the finalizer to fixtures after getting the cached value, which could plausibly remove some extraneous finalizers getting added, and that affected the result of `test_issue_519` #519 (teardown of `('test_one[arg1v2-arg2v1]', 'fix2', 'arg2v1'),` got moved one step earlier) and a similar thing in `test_parametrization_setup_teardown_ordering` (`step1-2` happening before `setup 2`), but I haven't dug into why or if/how it's bad.
  * saving the `functools.partial` call is mostly to make it more visible that it's done in several places in the function, but am down for skipping it.
* should I just make `execute` return `None`?

Problem statement:
fixture finalizer dependency incorrect when using autouse or getfuncargvalue
Calling fixture finalizers in the correct order works by dependent fixtures registering their finalizer in their dependency (see python.FixtureDef.execute+10).
However, this only happens for fixtures explicitly requested in args, not for autouse or getfuncargvalue fixtures.
This code:

``` python
import pytest

@pytest.fixture(scope="session")
def global_dict():
    return {}

@pytest.fixture(scope="session", autouse=True)
def add_1(global_dict):
    global_dict[1] = "a"
    print("add_1 setup", global_dict)
    yield global_dict
    print("add_1 teardown", global_dict)
    del global_dict[1]

@pytest.fixture(scope="session")
def change_1_to_2(global_dict):
    global_dict[2] = global_dict.pop(1)
    print("change_1_to_2 setup", global_dict)
    yield global_dict
    print("change_1_to_2 teardown", global_dict)
    global_dict[1] = global_dict.pop(2)

def test1(change_1_to_2):
    assert change_1_to_2[2] == "a"

def test2(global_dict):
    assert global_dict[2] == "a"
```

gives with `py.test --capture=no test.py`:

```
============================= test session starts ==============================
platform linux2 -- Python 2.7.11, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/amos/git/test_py, inifile: 
collected 2 items 

test.py add_1 setup {1: 'a'}
change_1_to_2 setup {2: 'a'}
..add_1 teardown {2: 'a'}
change_1_to_2 teardown {2: 'a'}
E

==================================== ERRORS ====================================
__________________________ ERROR at teardown of test2 __________________________

global_dict = {1: 'a'}

    @pytest.yield_fixture(scope="session", autouse=True)
    def add_1(global_dict):
        global_dict[1] = "a"
        print "add_1 setup", global_dict
        yield global_dict
        print "add_1 teardown", global_dict
>       del global_dict[1]
E       KeyError: 1

test.py:14: KeyError
====================== 2 passed, 1 error in 0.01 seconds =======================
```

as does this code:

``` python
import pytest

@pytest.fixture(scope="session")
def global_dict():
    return {}

@pytest.yield_fixture(scope="session")
def add_1(global_dict):
    global_dict[1] = "a"
    print "add_1 setup", global_dict
    yield global_dict
    print "add_1 teardown", global_dict
    del global_dict[1]

@pytest.yield_fixture(scope="session")
def change_1_to_2(request):
    add_1 = request.getfuncargvalue("add_1")
    add_1[2] = add_1.pop(1)
    print "change_1_to_2 setup", add_1
    yield add_1
    print "change_1_to_2 teardown", add_1
    add_1[1] = add_1.pop(2)

def test1(change_1_to_2):
    assert change_1_to_2[2] == "a"

def test2(global_dict):
    assert global_dict[2] == "a"
```

Hints from issue discussion:
Hi @amosonn,

Thanks for taking the time to write down this report. 

(I edited your post to add syntax highlight to your examples to make them easier to read, hope you don't mind).

I thought I could use autouse fixtures to do some setup and teardown, but since the teardown part is not garanteed to come after the teardown of non-autouse fixtures, it's not straightforward. Here is a more compact and modern test case that shows the problem (the scope could also be set to package or session):
```python
import pytest

@pytest.fixture(scope='module', autouse=True)
def fixture_autouse():
    print('\nSETUP FIXTURE AUTOUSE')
    yield
    print('\nTEARDOWN FIXTURE AUTOUSE')

@pytest.fixture(scope='module')
def fixture_test():
    print('\nSETUP FIXTURE TEST')
    yield
    print('\nTEARDOWN FIXTURE TEST')

def test_1(fixture_test):
    pass

def test_2():
    pass
```
running the test gives:
```
============================== 2 passed in 0.00s ===============================
(venv) (base) 22) lilla:~/work/bug_pytest> pytest -s tests/
============================= test session starts ==============================
platform linux -- Python 3.8.3, pytest-6.3.0.dev454+gb8f1b7cdc, py-1.10.0, pluggy-0.13.1
rootdir: /home/chanial/work/bug_pytest
collected 2 items                                                              

tests/test_fixture.py 
SETUP FIXTURE AUTOUSE

SETUP FIXTURE TEST
..
TEARDOWN FIXTURE AUTOUSE

TEARDOWN FIXTURE TEST


============================== 2 passed in 0.01s ===============================
```
which shows that the autouse fixture teardown does not come after that of the non-autouse fixture. Since the autouse fixture setup comes first wrt non-autouse setups, it is expected that their teardowns come last.
+1 ran into this bug. This behavior is different from the documented expected behavior that finalizers will run in reverse order to the order the fixtures were initialized. From https://docs.pytest.org/en/6.2.x/fixture.html#yield-fixtures-recommended :

> Once the test is finished, pytest will go back down the list of fixtures, but in the reverse order, taking each one that yielded, and running the code inside it that was after the yield statement.

Perhaps adding a caveat to the documentation would be good until this is fixed?
Started looking into this, and it's actually not `autouse` that is the problem. We can modify pchanial's example to not have autouse and get the same behaviour:

```python
import pytest

@pytest.fixture(scope='module')
def fixture_1():
    print('\nSETUP FIXTURE 1')
    yield
    print('\nTEARDOWN FIXTURE 1')

@pytest.fixture(scope='module')
def fixture_2():
    print('\nSETUP FIXTURE 2')
    yield
    print('\nTEARDOWN FIXTURE 2')

def test_1(fixture_1, fixture_2):
    pass

def test_2(fixture_1):
    pass
```

```
SETUP FIXTURE 1

SETUP FIXTURE 2
..
TEARDOWN FIXTURE 1

TEARDOWN FIXTURE 2
```

if the test order is reversed, or `test_2` is removed, it does not occur. What seems to be happening is that the teardown for fixtures are queued each time they're called, so we get the following finalizer stack:
```python
[<bound method Node.teardown of <Module mytest2.py>>,
 functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='fixture_1' scope='module' baseid='testing/python/mytest2.py'>>, request=<SubRequest 'fixture_1' for <Function test_1>>),
 functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='fixture_2' scope='module' baseid='testing/python/mytest2.py'>>, request=<SubRequest 'fixture_2' for <Function test_1>>),
 functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='fixture_1' scope='module' baseid='testing/python/mytest2.py'>>, request=<SubRequest 'fixture_1' for <Function test_2>>)]
```
it processes it from the end, tearing down `fixture_1`, then `fixture_2`, and then `fixture_1` again - but `FixtureDef.finish` ignores the second teardown of `fixture_1` since it's already been finalized.


My current best-guess on how to fix it is to add some logic to runner.py:SetupState.addfinalizer so we only add a finalizer for a given fixture once.
edit: probably better to add it to fixtures.py:FixtureRequest._compute_fixture_value

Commit message: improve comment getfixturevalue, move test_scoped_fixture_teardown_order to testing/python/fixtures.py

Patch to review:
diff --git a/changelog/1489.bugfix.rst b/changelog/1489.bugfix.rst
new file mode 100644
index 00000000000..70c5dd1252e
--- /dev/null
+++ b/changelog/1489.bugfix.rst
@@ -0,0 +1 @@
+Fix some instances where teardown of higher-scoped fixtures was not happening in the reverse order they were initialized in.
diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py
index 0a505d65ad0..fa727513c13 100644
--- a/src/_pytest/fixtures.py
+++ b/src/_pytest/fixtures.py
@@ -531,6 +531,11 @@ def getfixturevalue(self, argname: str) -> Any:
         :raises pytest.FixtureLookupError:
             If the given fixture could not be found.
         """
+        # Note that in addition to the use case described in the docstring,
+        # getfixturevalue() is also called by pytest itself during item setup to
+        # evaluate the fixtures that are requested statically
+        # (using function parameters, autouse, etc).
+
         fixturedef = self._get_active_fixturedef(argname)
         assert fixturedef.cached_result is not None, (
             f'The fixture value for "{argname}" is not available.  '
@@ -567,9 +572,8 @@ def _compute_fixture_value(self, fixturedef: "FixtureDef[object]") -> None:
         """Create a SubRequest based on "self" and call the execute method
         of the given FixtureDef object.
 
-        This will force the FixtureDef object to throw away any previous
-        results and compute a new fixture value, which will be stored into
-        the FixtureDef object itself.
+        If the FixtureDef has cached the result it will do nothing, otherwise it will
+        setup and run the fixture, cache the value, and schedule a finalizer for it.
         """
         # prepare a subrequest object before calling fixture function
         # (latter managed by fixturedef)
@@ -629,18 +633,8 @@ def _compute_fixture_value(self, fixturedef: "FixtureDef[object]") -> None:
 
         # Check if a higher-level scoped fixture accesses a lower level one.
         subrequest._check_scope(argname, self._scope, scope)
-        try:
-            # Call the fixture function.
-            fixturedef.execute(request=subrequest)
-        finally:
-            self._schedule_finalizers(fixturedef, subrequest)
-
-    def _schedule_finalizers(
-        self, fixturedef: "FixtureDef[object]", subrequest: "SubRequest"
-    ) -> None:
-        # If fixture function failed it might have registered finalizers.
-        finalizer = functools.partial(fixturedef.finish, request=subrequest)
-        subrequest.node.addfinalizer(finalizer)
+        # Make sure the fixture value is cached, running it if it isn't
+        fixturedef.execute(request=subrequest)
 
 
 @final
@@ -768,21 +762,6 @@ def _factorytraceback(self) -> List[str]:
     def addfinalizer(self, finalizer: Callable[[], object]) -> None:
         self._fixturedef.addfinalizer(finalizer)
 
-    def _schedule_finalizers(
-        self, fixturedef: "FixtureDef[object]", subrequest: "SubRequest"
-    ) -> None:
-        # If the executing fixturedef was not explicitly requested in the argument list (via
-        # getfixturevalue inside the fixture call) then ensure this fixture def will be finished
-        # first.
-        if (
-            fixturedef.argname not in self._fixture_defs
-            and fixturedef.argname not in self._pyfuncitem.fixturenames
-        ):
-            fixturedef.addfinalizer(
-                functools.partial(self._fixturedef.finish, request=self)
-            )
-        super()._schedule_finalizers(fixturedef, subrequest)
-
 
 @final
 class FixtureLookupError(LookupError):
@@ -1040,6 +1019,7 @@ def finish(self, request: SubRequest) -> None:
             self._finalizers.clear()
 
     def execute(self, request: SubRequest) -> FixtureValue:
+        finalizer = functools.partial(self.finish, request=request)
         # Get required arguments and register our own finish()
         # with their finalization.
         for argname in self.argnames:
@@ -1047,7 +1027,7 @@ def execute(self, request: SubRequest) -> FixtureValue:
             if argname != "request":
                 # PseudoFixtureDef is only for "request".
                 assert isinstance(fixturedef, FixtureDef)
-                fixturedef.addfinalizer(functools.partial(self.finish, request=request))
+                fixturedef.addfinalizer(finalizer)
 
         my_cache_key = self.cache_key(request)
         if self.cached_result is not None:
@@ -1057,6 +1037,7 @@ def execute(self, request: SubRequest) -> FixtureValue:
             if my_cache_key is cache_key:
                 if self.cached_result[2] is not None:
                     exc = self.cached_result[2]
+                    # this would previously trigger adding a finalizer. Should it?
                     raise exc
                 else:
                     result = self.cached_result[0]
@@ -1067,7 +1048,14 @@ def execute(self, request: SubRequest) -> FixtureValue:
             assert self.cached_result is None
 
         ihook = request.node.ihook
-        result = ihook.pytest_fixture_setup(fixturedef=self, request=request)
+        try:
+            # Setup the fixture, run the code in it, and cache the value
+            # in self.cached_result
+            result = ihook.pytest_fixture_setup(fixturedef=self, request=request)
+        finally:
+            # schedule our finalizer, even if the setup failed
+            request.node.addfinalizer(finalizer)
+
         return result
 
     def cache_key(self, request: SubRequest) -> object:
diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py
index 299e411a695..0758b590ca0 100644
--- a/testing/python/fixtures.py
+++ b/testing/python/fixtures.py
@@ -4559,3 +4559,134 @@ def test_deduplicate_names() -> None:
     assert items == ("a", "b", "c", "d")
     items = deduplicate_names((*items, "g", "f", "g", "e", "b"))
     assert items == ("a", "b", "c", "d", "g", "f", "e")
+
+
+def test_scoped_fixture_teardown_order(pytester: Pytester) -> None:
+    """
+    Make sure teardowns happen in reverse order of setup with scoped fixtures, when
+    a later test only depends on a subset of scoped fixtures.
+    Regression test for https://github.com/pytest-dev/pytest/issues/1489
+    """
+    pytester.makepyfile(
+        """
+        from typing import Generator
+
+        import pytest
+
+
+        last_executed = ""
+
+
+        @pytest.fixture(scope="module")
+        def fixture_1() -> Generator[None, None, None]:
+            global last_executed
+            assert last_executed == ""
+            last_executed = "autouse_setup"
+            yield
+            assert last_executed == "noautouse_teardown"
+            last_executed = "autouse_teardown"
+
+
+        @pytest.fixture(scope="module")
+        def fixture_2() -> Generator[None, None, None]:
+            global last_executed
+            assert last_executed == "autouse_setup"
+            last_executed = "noautouse_setup"
+            yield
+            assert last_executed == "run_test"
+            last_executed = "noautouse_teardown"
+
+
+        def test_autouse_fixture_teardown_order(fixture_1: None, fixture_2: None) -> None:
+            global last_executed
+            assert last_executed == "noautouse_setup"
+            last_executed = "run_test"
+
+
+        def test_2(fixture_1: None) -> None:
+            # this would previously queue an additional teardown of fixture_1,
+            # despite fixture_1's value being cached, which caused fixture_1 to be
+            # torn down before fixture_2 - violating the rule that teardowns should
+            # happen in reverse order of setup.
+            pass
+        """
+    )
+    result = pytester.runpytest()
+    assert result.ret == 0
+
+
+def test_scope_fixture_caching_1(pytester: Pytester) -> None:
+    """
+    Make sure setup and finalization is only run once when using fixture
+    multiple times. This might be a duplicate of another test."""
+    pytester.makepyfile(
+        """
+        from __future__ import annotations
+
+        from typing import Generator
+
+        import pytest
+        executed: list[str] = []
+        @pytest.fixture(scope="class")
+        def fixture_1() -> Generator[None, None, None]:
+            executed.append("fix setup")
+            yield
+            executed.append("fix teardown")
+
+
+        class TestFixtureCaching:
+            def test_1(self, fixture_1: None) -> None:
+                assert executed == ["fix setup"]
+
+            def test_2(self, fixture_1: None) -> None:
+                assert executed == ["fix setup"]
+
+
+        def test_expected_setup_and_teardown() -> None:
+            assert executed == ["fix setup", "fix teardown"]
+        """
+    )
+    result = pytester.runpytest()
+    assert result.ret == 0
+
+
+def test_scope_fixture_caching_2(pytester: Pytester) -> None:
+    """Make sure setup & finalization is only run once, with a cached exception."""
+    pytester.makepyfile(
+        """
+        from __future__ import annotations
+
+        from typing import Generator
+
+        import pytest
+        executed_crash: list[str] = []
+
+
+        @pytest.fixture(scope="class")
+        def fixture_crash(request: pytest.FixtureRequest) -> None:
+            executed_crash.append("fix_crash setup")
+
+            def my_finalizer() -> None:
+                executed_crash.append("fix_crash teardown")
+
+            request.addfinalizer(my_finalizer)
+
+            raise Exception("foo")
+
+
+        class TestFixtureCachingException:
+            @pytest.mark.xfail
+            def test_crash_1(self, fixture_crash: None) -> None:
+                ...
+
+            @pytest.mark.xfail
+            def test_crash_2(self, fixture_crash: None) -> None:
+                ...
+
+
+        def test_crash_expected_setup_and_teardown() -> None:
+            assert executed_crash == ["fix_crash setup", "fix_crash teardown"]
+        """
+    )
+    result = pytester.runpytest()
+    assert result.ret == 0


Code review comments:
diff_hunk: @@ -4559,3 +4559,134 @@ def test_deduplicate_names() -> None:
     assert items == ("a", "b", "c", "d")
     items = deduplicate_names((*items, "g", "f", "g", "e", "b"))
     assert items == ("a", "b", "c", "d", "g", "f", "e")
+
+
+def test_scoped_fixture_teardown_order(pytester: Pytester) -> None:
+    """
+    Make sure teardowns happen in reverse order of setup with scoped fixtures, when
+    a later test only depends on a subset of scoped fixtures.
+    Regression test for https://github.com/pytest-dev/pytest/issues/1489
+    """
+    pytester.makepyfile(
+        """
+        from typing import Generator
+
+        import pytest
+
+
+        last_executed = ""
+
+
+        @pytest.fixture(scope="module")
+        def fixture_1() -> Generator[None, None, None]:
+            global last_executed
+            assert last_executed == ""
+            last_executed = "autouse_setup"
+            yield
+            assert last_executed == "noautouse_teardown"
+            last_executed = "autouse_teardown"
+
+
+        @pytest.fixture(scope="module")
+        def fixture_2() -> Generator[None, None, None]:
+            global last_executed
+            assert last_executed == "autouse_setup"
+            last_executed = "noautouse_setup"
+            yield
+            assert last_executed == "run_test"
+            last_executed = "noautouse_teardown"
+
+
+        def test_autouse_fixture_teardown_order(fixture_1: None, fixture_2: None) -> None:
+            global last_executed
+            assert last_executed == "noautouse_setup"
+            last_executed = "run_test"
+
+
+        def test_2(fixture_1: None) -> None:
+            # this would previously queue an additional teardown of fixture_1,
+            # despite fixture_1's value being cached, which caused fixture_1 to be
+            # torn down before fixture_2 - violating the rule that teardowns should
+            # happen in reverse order of setup.
+            pass
+        """
+    )
+    result = pytester.runpytest()
+    assert result.ret == 0
+
+
+def test_scope_fixture_caching_1(pytester: Pytester) -> None:
+    """
+    Make sure setup and finalization is only run once when using fixture
+    multiple times. This might be a duplicate of another test."""
path: testing/python/fixtures.py
text: @user1:
```suggestion
    """Make sure setup and finalization is only run once when using a fixture
    multiple times."""
```

diff_hunk: @@ -4559,3 +4559,134 @@ def test_deduplicate_names() -> None:
     assert items == ("a", "b", "c", "d")
     items = deduplicate_names((*items, "g", "f", "g", "e", "b"))
     assert items == ("a", "b", "c", "d", "g", "f", "e")
+
+
+def test_scoped_fixture_teardown_order(pytester: Pytester) -> None:
+    """
+    Make sure teardowns happen in reverse order of setup with scoped fixtures, when
+    a later test only depends on a subset of scoped fixtures.
+    Regression test for https://github.com/pytest-dev/pytest/issues/1489
+    """
+    pytester.makepyfile(
+        """
+        from typing import Generator
+
+        import pytest
+
+
+        last_executed = ""
+
+
+        @pytest.fixture(scope="module")
+        def fixture_1() -> Generator[None, None, None]:
+            global last_executed
+            assert last_executed == ""
+            last_executed = "autouse_setup"
path: testing/python/fixtures.py
text: @user1:
The test mentions autouse but there is no autouse fixture, perhaps you changed it and forgot to update?

@author:
good catch, yep. It's only after I worked on this PR for a while I figured out that `autouse` wasn't required to surface the issue.