From 8606fdcd3b334780085c52dab5480c6eab53c64f Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Mon, 6 Oct 2025 10:10:51 +0300 Subject: [PATCH 1/2] testing: add some more tests for fixture closure Explicitly test a couple more scenarios. One of the tests doesn't currently pass so marked xfail. --- testing/python/fixtures.py | 107 +++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index d673eac7742..2f830275108 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -5198,3 +5198,110 @@ 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", "session", "db"] + assert request.fixturenames == ["request", "app", "user", "session", "db"] + """ + ) + result = pytester.runpytest("-v") + result.assert_outcomes(passed=1) + + +@pytest.mark.xfail(reason="not currently handled correctly") +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) From 3f831e600ad1d4cfa35a928485de4659cdbe5a26 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Mon, 6 Oct 2025 10:11:28 +0300 Subject: [PATCH 2/2] fixtures: fix closure computation with indirect override chains Continuation of 72ae3dbf9608a70df683d081cf53146a8d79f1ea. The previous fix was a minimal change to make the existing code at least consider fixtures overrides, and handles the common case (direct override chains), but still wasn't correct, as it didn't handle override chains involving an intermediary (not the overridden) fixture. To make this work, the algorithm needs to change. Now instead of a simple breadth-first search, we do a depth-first search, which more closely simulates the runtime behavior, and allows us the to check the "stack" of fixtures so we can use the correct fixture index (depth in the override chain). This is more expensive but should be OK. Refs #13773. --- src/_pytest/fixtures.py | 59 ++++++++++++++++++++++++-------------- testing/python/fixtures.py | 32 ++++++++++++++++++--- 2 files changed, 65 insertions(+), 26 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index b0ce620c13a..1f583a610f0 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1634,29 +1634,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: diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 2f830275108..ce0e5f87050 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -5110,7 +5110,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). @@ -5255,15 +5254,14 @@ def session(db): pass def app(user, session): pass def test_diamond_deps(request, app): - assert request.node.fixturenames == ["request", "app", "user", "session", "db"] - assert request.fixturenames == ["request", "app", "user", "session", "db"] + 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) -@pytest.mark.xfail(reason="not currently handled correctly") def test_fixture_closure_with_complex_override_and_shared_deps( pytester: Pytester, ) -> None: @@ -5305,3 +5303,29 @@ def test_shared_deps(self, request, app): ) 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)