Merge pull request #13789 from bluetech/fixtures-closure-visibility-3

 fixtures: fix closure computation with indirect override chains

Author: bluetech

src/_pytest/fixtures.py

@@ -1643,29 +1643,44 @@ def getfixtureclosure(
         fixturenames_closure = list(initialnames)
 
         arg2fixturedefs: dict[str, Sequence[FixtureDef[Any]]] = {}
-        lastlen = -1
-        while lastlen != len(fixturenames_closure):
-            lastlen = len(fixturenames_closure)
-            for argname in fixturenames_closure:
-                if argname in ignore_args:
-                    continue
-                if argname in arg2fixturedefs:
-                    continue
+
+        # Track the index for each fixture name in the simulated stack.
+        # Needed for handling override chains correctly, similar to _get_active_fixturedef.
+        # Using negative indices: -1 is the most specific (last), -2 is second to last, etc.
+        current_indices: dict[str, int] = {}
+
+        def process_argname(argname: str) -> None:
+            # Optimization: already processed this argname.
+            if current_indices.get(argname) == -1:
+                return
+
+            if argname not in fixturenames_closure:
+                fixturenames_closure.append(argname)
+
+            if argname in ignore_args:
+                return
+
+            fixturedefs = arg2fixturedefs.get(argname)
+            if not fixturedefs:
                 fixturedefs = self.getfixturedefs(argname, parentnode)
-                if fixturedefs:
-                    arg2fixturedefs[argname] = fixturedefs
-
-                    # 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
+                if not fixturedefs:
+                    # Fixture not defined or not visible (will error during runtest).
+                    return
+                arg2fixturedefs[argname] = fixturedefs
+
+            index = current_indices.get(argname, -1)
+            if -index > len(fixturedefs):
+                # Exhausted the override chain (will error during runtest).
+                return
+            fixturedef = fixturedefs[index]
+
+            current_indices[argname] = index - 1
+            for dep in fixturedef.argnames:
+                process_argname(dep)
+            current_indices[argname] = index
+
+        for name in initialnames:
+            process_argname(name)
 
         def sort_by_scope(arg_name: str) -> Scope:
             try:

testing/python/fixtures.py

@@ -5146,7 +5146,6 @@ def test_something(self, request, app):
     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).
@@ -5234,3 +5233,135 @@ def test_something(self, request, app):
     )
     result = pytester.runpytest("-v")
     result.assert_outcomes(passed=1)
+
+
+def test_fixture_closure_handles_circular_dependencies(pytester: Pytester) -> None:
+    """Test that getfixtureclosure properly handles circular dependencies.
+
+    The test will error in the runtest phase due to the fixture loop,
+    but the closure computation still completes.
+    """
+    pytester.makepyfile(
+        """
+        import pytest
+
+        # Direct circular dependency.
+        @pytest.fixture
+        def fix_a(fix_b): pass
+
+        @pytest.fixture
+        def fix_b(fix_a): pass
+
+        # Indirect circular dependency through multiple fixtures.
+        @pytest.fixture
+        def fix_x(fix_y): pass
+
+        @pytest.fixture
+        def fix_y(fix_z): pass
+
+        @pytest.fixture
+        def fix_z(fix_x): pass
+
+        def test_circular_deps(fix_a, fix_x):
+            pass
+        """
+    )
+    items, _hookrec = pytester.inline_genitems()
+    assert isinstance(items[0], Function)
+    assert items[0].fixturenames == ["fix_a", "fix_x", "fix_b", "fix_y", "fix_z"]
+
+
+def test_fixture_closure_handles_diamond_dependencies(pytester: Pytester) -> None:
+    """Test that getfixtureclosure properly handles diamond dependencies."""
+    pytester.makepyfile(
+        """
+        import pytest
+
+        @pytest.fixture
+        def db(): pass
+
+        @pytest.fixture
+        def user(db): pass
+
+        @pytest.fixture
+        def session(db): pass
+
+        @pytest.fixture
+        def app(user, session): pass
+
+        def test_diamond_deps(request, app):
+            assert request.node.fixturenames == ["request", "app", "user", "db", "session"]
+            assert request.fixturenames == ["request", "app", "user", "db", "session"]
+        """
+    )
+    result = pytester.runpytest("-v")
+    result.assert_outcomes(passed=1)
+
+
+def test_fixture_closure_with_complex_override_and_shared_deps(
+    pytester: Pytester,
+) -> None:
+    """Test that shared dependencies in override chains are processed only once."""
+    pytester.makeconftest(
+        """
+        import pytest
+
+        @pytest.fixture
+        def db(): pass
+
+        @pytest.fixture
+        def cache(): pass
+
+        @pytest.fixture
+        def settings(): pass
+
+        @pytest.fixture
+        def app(db, cache, settings): pass
+        """
+    )
+    pytester.makepyfile(
+        """
+        import pytest
+
+        # Override app, but also directly use cache and settings.
+        # This creates multiple paths to the same fixtures.
+        @pytest.fixture
+        def app(app, cache, settings): pass
+
+        class TestClass:
+            # Another override that uses both app and cache.
+            @pytest.fixture
+            def app(self, app, cache): pass
+
+            def test_shared_deps(self, request, app):
+                assert request.node.fixturenames == ["request", "app", "db", "cache", "settings"]
+        """
+    )
+    result = pytester.runpytest("-v")
+    result.assert_outcomes(passed=1)
+
+
+def test_fixture_closure_with_parametrize_ignore(pytester: Pytester) -> None:
+    """Test that getfixtureclosure properly handles parametrization argnames
+    which override a fixture."""
+    pytester.makepyfile(
+        """
+        import pytest
+
+        @pytest.fixture
+        def fix1(fix2): pass
+
+        @pytest.fixture
+        def fix2(fix3): pass
+
+        @pytest.fixture
+        def fix3(): pass
+
+        @pytest.mark.parametrize('fix2', ['2'])
+        def test_it(request, fix1):
+            assert request.node.fixturenames == ["request", "fix1", "fix2"]
+            assert request.fixturenames == ["request", "fix1", "fix2"]
+        """
+    )
+    result = pytester.runpytest("-v")
+    result.assert_outcomes(passed=1)