Skip to content

Move fixture cache from FixtureDef to SetupState - #14770

Open
seifertm wants to merge 26 commits into
pytest-dev:mainfrom
seifertm:move_fixture_cache_from_fixturedef_to_setupstate
Open

Move fixture cache from FixtureDef to SetupState#14770
seifertm wants to merge 26 commits into
pytest-dev:mainfrom
seifertm:move_fixture_cache_from_fixturedef_to_setupstate

Conversation

@seifertm

@seifertm seifertm commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

This PR removes FixtureDef.cached_result in favor of a cache that is attached to SetupState. FixtureRequest received methods to manipulate the fixture cache.

The PR is easiest reviewed commit by commit.

Open issues

Previously, fixture values were cached as part of FixtureDef objects and the cache entries contained a cache_key. The cache key is either None for non-parametrized fixtures or SubRequest.param. This allows invalidating cache entries when fixture parameters change.

Hoisting up the fixture cache to SetupState technically makes FixtureDef part of the cache key. This means we should use a two-dimensional index (e.g. tuple[FixtureDef, param]) to store entries in the fixture cache. Unfortunately, this is tricky, because TopRequest doesn't know about the current fixture param and therefore doesn't know the cache key.

This work has been done as part of the pytest sprint 2026, sponsored by Omicron.

@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jul 23, 2026
@seifertm
seifertm force-pushed the move_fixture_cache_from_fixturedef_to_setupstate branch from c54967f to 6678e5d Compare July 23, 2026 10:19
@seifertm
seifertm marked this pull request as ready for review July 23, 2026 10:19
Comment thread src/_pytest/setuponly.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed

@seifertm seifertm Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, thanks @RonnyPfannschmidt !

I pushed 5 additional commits. The tracking of the currently active request parameter for --setup-show is no longer attached to FixtureDef. Instead, --setup-show uses the existing SetupState.fixture_cache, which already contains the active fixture param as a cache key. The variable naming was changed from "cache_key" to "param" or "active_param".

In order to distinguish between "no param" and "param=None", I introduced a sentinel value. This prevents --setup-show from printing my_fixture[None] for unparametrized fixtures.

@bluetech

Copy link
Copy Markdown
Member

Previous stalled PR you might want to use as a reference/comparison: #14104

@seifertm

Copy link
Copy Markdown
Contributor Author

Thanks @bluetech! This topic was raised during the pytest sprint and I wasn't aware that there have been previous attempts at this. I'll check out the PR.

@seifertm
seifertm force-pushed the move_fixture_cache_from_fixturedef_to_setupstate branch from cf3854d to 61286bd Compare July 24, 2026 09:34
@seifertm

Copy link
Copy Markdown
Contributor Author

Previous stalled PR you might want to use as a reference/comparison: #14104

The linked PR appears to rework fixture evaluation logic and move that to runner.py. This PR reduces the mutability of FixtureDef by moving the fixture cache from FixtureDef to SetupState. On first sight, both PRs seem to be complementary rather than competing with each other.

Comment thread src/_pytest/fixtures.py Outdated
Comment thread changelog/14770.misc.rst Outdated
Comment thread src/_pytest/fixtures.py Outdated
Comment thread src/_pytest/fixtures.py
# If the fixture was executed, the current value of the fixture.
# Can change if the fixture is executed with different parameters.
self.cached_result: _FixtureCachedResult[FixtureValue] | None = None
self._finalizers: Final[list[Callable[[], object]]] = []

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the PR, more of a question (@RonnyPfannschmidt): what about the finalizers? We likely would also want finalizers to be hosted per-thread?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finlizers should also move to setupstate

Comment thread src/_pytest/runner.py Outdated
Comment on lines +514 to +516
# Importing the appropriate types from the fixtures module lead to circular
# imports, so we leave the cache untyped for now
self.fixture_cache = {} # type: ignore[var-annotated]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having the type checking here is important, let's move the type definitions we need to a new fixtures_types module, containing only the type definitions, which allows us to import them here without cycles.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be important to allow type-safe use of the fixture cache. I noticed that we cannot simply type the fixture cache as dict[FixtureDef[FixtureValue], FixtureValue], because FixtureValue is only bound once to the whole dict, but it can be different for each entry.

Therefore, I ended up creating a FixtureCache class that provides a type-safe API for its entries. The class lives in a dedicated fixture_cache module. I could simply move the cache accessors from FixtureRequest to the new class, so the change doesn't even introduce an extra layer of indirection.

see commit 9e2a891

Let me know what you think of that.

Comment thread testing/test_setuponly.py
)


def test_suppress_capturing(pytester: Pytester) -> None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test defined here?

@seifertm seifertm Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added to make the CI pass. The codecov job seems to fail, if the coverage of the PR is less than 100%. Apparently this also applies to files that are touched by the PR, but didn't have full coverage. This test specifically covers the negative branch of this if statement.

@seifertm
seifertm force-pushed the move_fixture_cache_from_fixturedef_to_setupstate branch from 1c6a7d2 to 9720a75 Compare July 24, 2026 14:39
@INTODAN

INTODAN commented Jul 28, 2026

Copy link
Copy Markdown

Hello!
Can you add which existing issues this PR might solve? For example the stalled PR #14104 specifies multiple existing issues it solves, such as #9287

Thanks!

@seifertm

seifertm commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

I haven't forgotten about this PR and will try to address the remaining comments this week.

seifertm added 16 commits August 9, 2026 17:54
…hing fixture results to distinguish between *no param* and *None*.
seifertm and others added 10 commits August 9, 2026 17:54
Co-authored-by: Bruno Oliveira <bruno@soliv.dev>
This allows typed use of SetupState.fixture_cache without import cycles. All of the methods of the new FixtureCache class were already present in FixtureRequest, so this refactoring makes FixtureRequest more manageable without introducing and extra layer of indirection.
@seifertm
seifertm force-pushed the move_fixture_cache_from_fixturedef_to_setupstate branch from 9720a75 to 9e2a891 Compare August 9, 2026 17:44
@seifertm

seifertm commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Hello! Can you add which existing issues this PR might solve? For example the stalled PR #14104 specifies multiple existing issues it solves, such as #9287

Thanks!

@INTODAN This PR doesn't resolve an existing issue. It is a refactoring towards enabling multi-threaded runs in pytest. The scope of this PR has been limited deliberately to allow a proper review. I guess the best issue to reference is #13768
The approach has been discussed with some of the core devs at this year's pytest sprint. The larger idea is to move mutable state into SetupState and give each thread their own SetupState.

@seifertm

seifertm commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Added the FixtureCache class and rebased to latest main.

@RonnyPfannschmidt

Copy link
Copy Markdown
Member

Looking at this in the context of the long-standing invocation-scoped fixtures request (#1681, POC in #12816), since that feature is blocked on exactly the state this PR touches. Two notes.

On the "Open issues" section: key by owner, not by param.

You want tuple[FixtureDef, param] and are blocked because TopRequest doesn't know the current param. Consider making the second dimension the owner of the instance instead:

  • for the node-anchored scopes the owner is the scope node, which SubRequest already computes in __init__,
  • and TopRequest knows its own node, so the lookup is always answerable.

param then stays inside the entry for miss detection, exactly as you have it now. That gets you the two-dimensional index without the thing that blocked you, and it happens to be the dimension a future "invocation" scope needs as well — there the owner is the requesting request rather than a collection node, because such an instance is not anchored on the tree at all.

Complementarity with #14104.

#14104 moves the other half of the per-instance state: FixtureDef._finalizers plus the teardown ordering graph, into SetupState. This PR drops cached_result and keeps _finalizers; #14104 drops _finalizers and keeps cached_result. Together they would empty FixtureDef of instance state entirely, which is the actual precondition for any scope that can have more than one live instance per definition. They will conflict textually, so we should pick an order rather than let them race. Note also that #14104's SetupState._active_fixtures is keyed by FixtureDef alone and asserts on re-entry, so the same owner dimension applies there.

For the record: this PR on its own does not yet unblock #12816. I ported the invocation-scope patch onto this branch and it still hits assert not self._finalizers in FixtureDef.execute, because the finalizer list stays on FixtureDef and FixtureCache remains one entry per fixturedef. That is not a complaint about the scope of this PR — just a note that the owner-keyed index above is what turns it from a relocation into the foundation.

Thanks for picking this up at the sprint, the direction is right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants