diff --git a/AUTHORS b/AUTHORS index bd1e27cb37d..794065d1603 100644 --- a/AUTHORS +++ b/AUTHORS @@ -430,6 +430,7 @@ Roberto Polli Roland Puntaier Romain Dorgueil Roman Bolshakov +Rongali Chaitanya Ronny Pfannschmidt Roqia Salah Ross Lawley diff --git a/changelog/2844.bugfix.rst b/changelog/2844.bugfix.rst new file mode 100644 index 00000000000..5bb406e2bbd --- /dev/null +++ b/changelog/2844.bugfix.rst @@ -0,0 +1,2 @@ +Invalidate a cached fixture when an overridden dependency selects a different fixture definition. +Keep finalizers from previous dependency definitions from tearing down the replacement fixture. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 20e43f89132..1965dd9868c 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1186,6 +1186,7 @@ def __init__( # Can change if the fixture is executed with different parameters. self.cached_result: _FixtureCachedResult[FixtureValue] | None = None self._finalizers: Final[list[Callable[[], object]]] = [] + self._cached_dependencies: list[FixtureDef[object]] = [] # only used to emit a deprecationwarning, can be removed in pytest9 self._autouse = _autouse @@ -1222,6 +1223,7 @@ def finish(self, request: SubRequest) -> None: # which will keep instances alive. self.cached_result = None self._finalizers.clear() + self._cached_dependencies.clear() if len(exceptions) == 1: raise exceptions[0] elif len(exceptions) > 1: @@ -1244,7 +1246,10 @@ def execute(self, request: SubRequest) -> FixtureValue: # down first. This is generally handled by SetupState, but still currently # needed when this fixture is not parametrized but depends on a parametrized # fixture. - requested_fixtures_that_should_finalize_us.append(fixturedef) + # The synthetic request fixture is new for every request and has no + # teardown or value shared between fixture executions. + if not isinstance(fixturedef, RequestFixtureDef): + requested_fixtures_that_should_finalize_us.append(fixturedef) # Check for (and return) cached value/exception. if self.cached_result is not None: @@ -1258,13 +1263,18 @@ def execute(self, request: SubRequest) -> FixtureValue: # If the comparison raises, use 'is' as fallback. cache_hit = request_cache_key is cache_key + # An override may select a different dependency definition without + # changing this fixture's own parameter (#2844). + cache_hit = cache_hit and ( + self._cached_dependencies == requested_fixtures_that_should_finalize_us + ) if cache_hit: if self.cached_result[2] is not None: exc, exc_tb = self.cached_result[2] raise exc.with_traceback(exc_tb) else: return self.cached_result[0] - # We have a previous but differently parametrized fixture instance + # We have a differently parametrized fixture or different dependencies # so we need to tear it down before creating a new one. self.finish(request) assert self.cached_result is None @@ -1272,7 +1282,14 @@ def execute(self, request: SubRequest) -> FixtureValue: # Add finalizer to requested fixtures we saved previously. # We make sure to do this after checking for cached value to avoid # adding our finalizer multiple times. (#12135) - finalizer = functools.partial(self.finish, request=request) + self._cached_dependencies = requested_fixtures_that_should_finalize_us + + def finalizer() -> None: + # An old dependency may finish after an override has replaced our + # cached value. Its finalizer belongs only to the earlier execution. + if self._cached_dependencies is requested_fixtures_that_should_finalize_us: + self.finish(request) + for parent_fixture in requested_fixtures_that_should_finalize_us: parent_fixture.addfinalizer(finalizer) diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index cb2f1daab63..fe29421ea1e 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -19,6 +19,189 @@ import pytest +@pytest.mark.parametrize("scope", ["session", "module"]) +@pytest.mark.parametrize("parametrized", [False, True]) +@pytest.mark.parametrize("reverse", [False, True]) +def test_fixture_cache_with_overridden_dependency( + pytester: Pytester, scope: str, parametrized: bool, reverse: bool +) -> None: + """An overridden dependency must invalidate the wrapper cache (#2844).""" + pytester.makepyfile(state="events = []\nactive = set()") + pytester.makeconftest( + f""" + import pytest + from state import active, events + + @pytest.fixture(scope={scope!r}) + def wrapper(request, dependency): + assert not active + active.add(dependency) + events.append(("wrapper setup", dependency)) + yield dependency + active.remove(dependency) + events.append(("wrapper teardown", dependency)) + """ + ) + for name in ("first", "second"): + params = f", params=[{name + ' A'!r}, {name + ' B'!r}]" if parametrized else "" + value = "request.param" if parametrized else repr(name) + pytester.makepyfile( + **{ + f"test_{name}": f""" + import pytest + import state + + @pytest.fixture(scope={scope!r}{params}) + def dependency(request): + value = {value} + state.events.append(("dependency setup", value)) + yield value + assert value not in state.active + state.events.append(("dependency teardown", value)) + + def test_one(wrapper, dependency): + assert wrapper == dependency + + def test_two(wrapper, dependency): + assert wrapper == dependency + """ + } + ) + paths = ["test_first.py", "test_second.py"] + if reverse: + paths.reverse() + result = pytester.inline_run(*paths) + result.assertoutcome(passed=8 if parametrized else 4) + state = result.getcalls("pytest_runtest_call")[0].item.module.state + assert not state.active + values = ( + ["first A", "first B", "second A", "second B"] + if parametrized + else ["first", "second"] + ) + for value in values: + lifecycle = [ + ("dependency setup", value), + ("wrapper setup", value), + ("wrapper teardown", value), + ("dependency teardown", value), + ] + assert [event for event in state.events if event[1] == value] == lifecycle + + +def test_overridden_dependency_old_finalizer_preserves_new_cache( + pytester: Pytester, +) -> None: + """A previous dependency must not tear down the replacement wrapper.""" + pytester.makepyfile(state="setups = []") + order = [ + "test_first.py::test_initial[1]", + "test_second.py::test_override", + "test_first.py::test_dependency_only[2]", + "test_second.py::test_still_cached", + ] + pytester.makeconftest( + f""" + import pytest + import state + + @pytest.hookimpl(wrapper=True) + def pytest_collection_modifyitems(items): + yield + items.sort(key=lambda item: {order!r}.index(item.nodeid)) + + @pytest.fixture(scope="session") + def wrapper(dependency): + state.setups.append(dependency) + yield dependency + """ + ) + pytester.makepyfile( + test_first=""" + import pytest + + @pytest.fixture(scope="session", params=[1, 2]) + def dependency(request): + return request.param + + def test_initial(wrapper): + assert wrapper == 1 + + def test_dependency_only(dependency): + assert dependency == 2 + """, + test_second=""" + import pytest + import state + + @pytest.fixture(scope="session") + def dependency(): + return "override" + + def test_override(wrapper): + assert wrapper == "override" + + def test_still_cached(wrapper): + assert wrapper == "override" + assert state.setups == [1, "override"] + """, + ) + result = pytester.inline_run(*order) + result.assertoutcome(passed=4) + + +def test_fixture_cached_error_with_overridden_dependency(pytester: Pytester) -> None: + """Overrides must also invalidate cached setup errors, without losing reuse.""" + pytester.makepyfile(state="setups = []") + pytester.makeconftest( + """ + import pytest + import state + + @pytest.fixture(scope="session") + def wrapper(dependency): + state.setups.append(dependency) + if dependency == "bad": + raise ValueError("bad dependency") + return dependency + """ + ) + pytester.makepyfile( + test_first=""" + import pytest + + @pytest.fixture(scope="session") + def dependency(): + return "bad" + + @pytest.mark.xfail(raises=ValueError, strict=True) + def test_one(wrapper): + pass + + @pytest.mark.xfail(raises=ValueError, strict=True) + def test_two(wrapper): + pass + """, + test_second=""" + import pytest + import state + + @pytest.fixture(scope="session") + def dependency(): + return "good" + + def test_one(wrapper): + assert wrapper == "good" + + def test_two(wrapper): + assert wrapper == "good" + assert state.setups == ["bad", "good"] + """, + ) + result = pytester.inline_run("test_first.py", "test_second.py") + result.assertoutcome(passed=2, skipped=2) + + def test_getfuncargnames_functions(): """Test getfuncargnames for normal functions"""