Merge pull request #13774 from bluetech/fixture-closure-overrides

fixtures: fix fixture closure calculation to properly consider overridden fixtures

Author: bluetech

changelog/13773.bugfix.rst

@@ -0,0 +1 @@
+Fixed the static fixture closure calculation to properly consider transitive dependencies requested by overridden fixtures.

src/_pytest/fixtures.py

@@ -1645,9 +1645,18 @@ def getfixtureclosure(
                 fixturedefs = self.getfixturedefs(argname, parentnode)
                 if fixturedefs:
                     arg2fixturedefs[argname] = fixturedefs
-                    for arg in fixturedefs[-1].argnames:
-                        if arg not in fixturenames_closure:
-                            fixturenames_closure.append(arg)
+
+                    # Add dependencies from this fixture.
+                    # If it overrides a fixture with the same name and requests
+                    # it, also add dependencies from the overridden fixtures in
+                    # the chain. See also similar dealing in _get_active_fixturedef().
+                    for fixturedef in reversed(fixturedefs):  # pragma: no cover
+                        for arg in fixturedef.argnames:
+                            if arg not in fixturenames_closure:
+                                fixturenames_closure.append(arg)
+                        if argname not in fixturedef.argnames:
+                            # Overrides, but doesn't request super.
+                            break
 
         def sort_by_scope(arg_name: str) -> Scope:
             try:

testing/python/fixtures.py

@@ -5069,3 +5069,132 @@ def test_method(self, /, fix):
     )
     result = pytester.runpytest()
     result.assert_outcomes(passed=1)
+
+
+def test_fixture_closure_with_overrides(pytester: Pytester) -> None:
+    """Test that an item's static fixture closure properly includes transitive
+    dependencies through overridden fixtures (#13773)."""
+    pytester.makeconftest(
+        """
+        import pytest
+
+        @pytest.fixture
+        def db(): pass
+
+        @pytest.fixture
+        def app(db): pass
+        """
+    )
+    pytester.makepyfile(
+        """
+        import pytest
+
+        # Overrides conftest-level `app` and requests it.
+        @pytest.fixture
+        def app(app): pass
+
+        class TestClass:
+            # Overrides module-level `app` and requests it.
+            @pytest.fixture
+            def app(self, app): pass
+
+            def test_something(self, request, app):
+                # Both dynamic and static fixture closures should include 'db'.
+                assert 'db' in request.fixturenames
+                assert 'db' in request.node.fixturenames
+                # No dynamic dependencies, should be equal.
+                assert set(request.fixturenames) == set(request.node.fixturenames)
+        """
+    )
+    result = pytester.runpytest("-v")
+    result.assert_outcomes(passed=1)
+
+
+@pytest.mark.xfail(reason="not currently handled correctly")
+def test_fixture_closure_with_overrides_and_intermediary(pytester: Pytester) -> None:
+    """Test that an item's static fixture closure properly includes transitive
+    dependencies through overridden fixtures (#13773).
+
+    A more complicated case than test_fixture_closure_with_overrides, adds an
+    intermediary so the override chain is not direct.
+    """
+    pytester.makeconftest(
+        """
+        import pytest
+
+        @pytest.fixture
+        def db(): pass
+
+        @pytest.fixture
+        def app(db): pass
+
+        @pytest.fixture
+        def intermediate(app): pass
+        """
+    )
+    pytester.makepyfile(
+        """
+        import pytest
+
+        # Overrides conftest-level `app` and requests it.
+        @pytest.fixture
+        def app(intermediate): pass
+
+        class TestClass:
+            # Overrides module-level `app` and requests it.
+            @pytest.fixture
+            def app(self, app): pass
+
+            def test_something(self, request, app):
+                # Both dynamic and static fixture closures should include 'db'.
+                assert 'db' in request.fixturenames
+                assert 'db' in request.node.fixturenames
+                # No dynamic dependencies, should be equal.
+                assert set(request.fixturenames) == set(request.node.fixturenames)
+        """
+    )
+    result = pytester.runpytest("-v")
+    result.assert_outcomes(passed=1)
+
+
+def test_fixture_closure_with_broken_override_chain(pytester: Pytester) -> None:
+    """Test that an item's static fixture closure properly includes transitive
+    dependencies through overridden fixtures (#13773).
+
+    A more complicated case than test_fixture_closure_with_overrides, one of the
+    fixtures in the chain doesn't call its super, so it shouldn't be included.
+    """
+    pytester.makeconftest(
+        """
+        import pytest
+
+        @pytest.fixture
+        def db(): pass
+
+        @pytest.fixture
+        def app(db): pass
+        """
+    )
+    pytester.makepyfile(
+        """
+        import pytest
+
+        # Overrides conftest-level `app` and *doesn't* request it.
+        @pytest.fixture
+        def app(): pass
+
+        class TestClass:
+            # Overrides module-level `app` and requests it.
+            @pytest.fixture
+            def app(self, app): pass
+
+            def test_something(self, request, app):
+                # Both dynamic and static fixture closures should include 'db'.
+                assert 'db' not in request.fixturenames
+                assert 'db' not in request.node.fixturenames
+                # No dynamic dependencies, should be equal.
+                assert set(request.fixturenames) == set(request.node.fixturenames)
+        """
+    )
+    result = pytester.runpytest("-v")
+    result.assert_outcomes(passed=1)