Don't reregister subfixture finalizer in parent fixture if value is cached #12136

jakkdl · 2024-03-18T14:18:30Z

When getting the value of a fixture, it will always add its finalizer to parent fixtures, regardless of if it's cached or not. When we have several subfixtures, this leads to unpredictable ordering between them when they're torn down (if triggered by the parent fixture being torn down). It is also an obvious inefficiency, where a long-lived parent fixture could rack up tons of irrelevant fixtures.

Adds two regression tests: one that checks the ordering failure, and one that checks the number of finalizers.

The implementation would be slightly cleaner if parent fixture parametrization was included in the cache key, as discussed in #4871 (comment)

…ached.

WarrenTheRabbit · 2024-03-19T11:23:33Z

@jakkdl: I like how you add context by including the related issue in the source code of your regression test. I think I'll adopt that practice myself! Is it a common policy?

Other than a point of praise, I don't really have anything to say. pytest is still quite opaque to me. But I'll read through the issues you cite (thank you for including them) when I have time and see if I can follow along with your approach.

jakkdl · 2024-03-19T11:37:48Z

@jakkdl: I like how you add context by including the related issue in the source code of your regression test. I think I'll adopt that practice myself! Is it a common policy?

I have seen different versions of it in different repos, so I'd say it's relatively common. This repo also has a couple files dedicated to being regression tests for issues, e.g.

testing/example_scripts/issue88_initial_file_multinodes
testing/example_scripts/issue_519.py
testing/examples/test_issue519.py

There's also stuff like https://docs.astral.sh/ruff/rules/missing-todo-link/

Other than a point of praise, I don't really have anything to say. pytest is still quite opaque to me. But I'll read through the issues you cite (thank you for including them) when I have time and see if I can follow along with your approach.

Oh yeah it took me a while to grok the fixture infrastructure, not for the faint of heart!

WarrenTheRabbit · 2024-03-19T12:17:25Z

This repo also has a couple files dedicated to being regression tests for issues

There's also stuff like https://docs.astral.sh/ruff/rules/missing-todo-link/

Thanks for the thoughtful response. ruff looks cool too.

Oh yeah it took me a while to grok the fixture infrastructure, not for the faint of heart!

Ha! I'm at the point where if I say anything at all about the fixture infrastructure I invariably say nonsensical things and embarrass myself horribly. But am definitely trying to be a citizen of this place - one fainting spell at a time. In fact, wrote some things about it the other day in an Issue and likely face-planted myself all over the pavement. Such is the journey I suppose. Onwards and upwards. But also: Eeep!

I won't yatter on and make it harder for someone to get up to speed on your PR. Thanks, @jakkdl!

bluetech

Thanks @jakkdl! As mentioned in the issue, this change LGTM. I left some comments mostly on terminology.

bluetech · 2024-03-21T13:57:33Z

changelog/12135.bugfix.rst

@@ -0,0 +1 @@
+Fix subfixtures adding their finalizer multiple times to parent fixtures, causing incorrect teardown ordering in some instances.


I feel the "parent fixture" terminology a bit confusing, since if fix1 requests fix2, from certain perspectives one could consider fix1 to be the parent of fix2, while you mean the reverse (which also makes sense in certain perspectives).
So I prefer to use "requests" terminology which is usually pretty clear.

I also wouldn't call it "incorrect teardown ordering", because I think we should reserve "incorrect" for cases where there is a violation of dependency order. We don't strictly guarantee stack order. So I suggest "non-intuitive".

Suggested change

Fix subfixtures adding their finalizer multiple times to parent fixtures, causing incorrect teardown ordering in some instances.

Fix fixtures adding their finalizer multiple times to fixture they request, causing non-intuitive teardown ordering in some instances.

"a fixture that this fixture requests" turned out to be very wordy and hard to read, so I swapped to using "dependent fixtures" - which seems to be somewhat established and shouldn't be possible to interpret as the reverse.

Switched to non-intuitive + unreliable (since teardown order will change depending on which fixture was last requested)

bluetech · 2024-03-21T14:07:07Z

src/_pytest/fixtures.py

+        # Ensure arguments (parent fixtures) are loaded.
+        # If their cache has been invalidated it will finish itself and subfixtures,
+        # which will set our self.cached_result = None.
+        # If/when parent fixture parametrization is included in our cache key this
+        # can be moved after checking our cache key and not require saving in a list.


Let's avoid the term "subfixtures" since it's not a term we use and it's not very clear (to me at least), similar to "parent".

I think the "if/when" comment is more of a wishlist item so better to keep it in the issue.

Suggested rewording:

Suggested change

# Ensure arguments (parent fixtures) are loaded.

# If their cache has been invalidated it will finish itself and subfixtures,

# which will set our self.cached_result = None.

# If/when parent fixture parametrization is included in our cache key this

# can be moved after checking our cache key and not require saving in a list.

# Ensure fixtures that this fixture requests are loaded (XXX: figure out

# why this is needed even in the cached case).

#

# Also make it such that whenever a fixture we request is torn down,

# this fixture is torn down first. This is generally handled by

# SetupState, but still currently needed when the requested fixture is

# not parameterized itself but depends on a parameterized fixture (see

# #4871 for more details).

# Only do this in the non-cached case. It's not needed in the cached

# case because the finalizers had already been registered.

XXX: figure out why this is needed even in the cached case

The reason this is needed in the cached case is also because of the requested-fixture-parametrization case (and possible other cases where the requested fixture's cache is invalidated?). This fixture's cache key does not change depending on the cache key of the requested fixture, and the following couple lines only check our cache key before deciding whether to return early.

Also make it such that whenever a fixture we request is torn down, this fixture is torn down first.

I slightly disagree on putting this up top instead of when this is actually ensured, where you wanted to remove the comment, but I'll reword it slightly different to not directly imply that this is a direct consequence of the lines directly following this.

It's slightly confusing to have a huge block comment at the beginning of a function, implying it explains the whole function, but it in fact is just explaining the first 7 lines of it. So I'm adding a short docstring too.

src/_pytest/fixtures.py

bluetech · 2024-03-21T14:09:24Z

src/_pytest/fixtures.py

@@ -1060,6 +1065,11 @@ def execute(self, request: SubRequest) -> FixtureValue:
            self.finish(request)
            assert self.cached_result is None

+        finalizer = functools.partial(self.finish, request=request)
+        # add finalizer to parent fixtures


I think the comment at the top is sufficient

Suggested change

# add finalizer to parent fixtures

I removed it.. and added it back. Feel free to remove it again if you still think it is detracting

testing/python/fixtures.py

bluetech · 2024-03-27T15:15:51Z

src/_pytest/fixtures.py

-        # Get required arguments and register our own finish()
-        # with their finalization.
+        """Return the value of this fixture, executing it if not cached."""
+        # Ensure dependent fixtures that this fixture requests are loaded.


This comment has a grammar issue, can you rephrase it?

I failed to see the problem on my own, but Claude fixed it :)

fix ambigious phrasing in comment

bluetech · 2024-03-31T12:03:45Z

Thanks @jakkdl!

BTW, I'm going to send you an invite to join pytest-dev. This doesn't require anything from you really, but will allow you to edit issues, merge your PRs, etc. if you'd like to contribute more.

jakkdl · 2024-03-31T13:49:08Z

Thanks @jakkdl!

BTW, I'm going to send you an invite to join pytest-dev. This doesn't require anything from you really, but will allow you to edit issues, merge your PRs, etc. if you'd like to contribute more.

Sweet, might come in handy!

…s cached (pytest-dev#12136)

Don't reregister subfixture finalizer in parent fixture if value is c…

358513b

…ached.

bluetech requested changes Mar 21, 2024

View reviewed changes

jakkdl and others added 2 commits March 26, 2024 15:44

update comments, and remove test relying on internal functionality

1eebb78

Merge branch 'main' into parent_fixture_finalization_on_cached

a2a7654

bluetech reviewed Mar 27, 2024

View reviewed changes

Update fixtures.py

6bbd2b2

fix ambigious phrasing in comment

bluetech approved these changes Mar 31, 2024

View reviewed changes

bluetech merged commit e64efd8 into pytest-dev:main Mar 31, 2024
24 checks passed

flying-sheep pushed a commit to flying-sheep/pytest that referenced this pull request Apr 9, 2024

Don't reregister subfixture finalizer in requested fixture if value i…

a709942

…s cached (pytest-dev#12136)

hrfuller pushed a commit to hrfuller/pytest that referenced this pull request Apr 17, 2024

Don't reregister subfixture finalizer in requested fixture if value i…

4ef23c1

…s cached (pytest-dev#12136)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't reregister subfixture finalizer in parent fixture if value is cached #12136

Don't reregister subfixture finalizer in parent fixture if value is cached #12136

jakkdl commented Mar 18, 2024

WarrenTheRabbit commented Mar 19, 2024

jakkdl commented Mar 19, 2024

WarrenTheRabbit commented Mar 19, 2024

bluetech left a comment

bluetech Mar 21, 2024 •

edited

Loading

jakkdl Mar 26, 2024

bluetech Mar 21, 2024

jakkdl Mar 26, 2024

jakkdl Mar 26, 2024

bluetech Mar 21, 2024

jakkdl Mar 26, 2024

bluetech Mar 27, 2024

jakkdl Mar 31, 2024 •

edited

Loading

bluetech commented Mar 31, 2024

jakkdl commented Mar 31, 2024

		@@ -0,0 +1 @@
		Fix subfixtures adding their finalizer multiple times to parent fixtures, causing incorrect teardown ordering in some instances.

	Fix subfixtures adding their finalizer multiple times to parent fixtures, causing incorrect teardown ordering in some instances.
	Fix fixtures adding their finalizer multiple times to fixture they request, causing non-intuitive teardown ordering in some instances.

-        # Ensure arguments (parent fixtures) are loaded.
-        # If their cache has been invalidated it will finish itself and subfixtures,
-        # which will set our self.cached_result = None.
-        # If/when parent fixture parametrization is included in our cache key this
-        # can be moved after checking our cache key and not require saving in a list.
+        # Ensure fixtures that this fixture requests are loaded (XXX: figure out
+        # why this is needed even in the cached case).
+        #
+        # Also make it such that whenever a fixture we request is torn down,
+        # this fixture is torn down first. This is generally handled by
+        # SetupState, but still currently needed when the requested fixture is
+        # not parameterized itself but depends on a parameterized fixture (see
+        # #4871 for more details).
+        # Only do this in the non-cached case. It's not needed in the cached
+        # case because the finalizers had already been registered.

Don't reregister subfixture finalizer in parent fixture if value is cached #12136

Don't reregister subfixture finalizer in parent fixture if value is cached #12136

Conversation

jakkdl commented Mar 18, 2024

WarrenTheRabbit commented Mar 19, 2024

jakkdl commented Mar 19, 2024

WarrenTheRabbit commented Mar 19, 2024

bluetech left a comment

Choose a reason for hiding this comment

bluetech Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

jakkdl Mar 26, 2024

Choose a reason for hiding this comment

bluetech Mar 21, 2024

Choose a reason for hiding this comment

jakkdl Mar 26, 2024

Choose a reason for hiding this comment

jakkdl Mar 26, 2024

Choose a reason for hiding this comment

bluetech Mar 21, 2024

Choose a reason for hiding this comment

jakkdl Mar 26, 2024

Choose a reason for hiding this comment

bluetech Mar 27, 2024

Choose a reason for hiding this comment

jakkdl Mar 31, 2024 • edited Loading

Choose a reason for hiding this comment

bluetech commented Mar 31, 2024

jakkdl commented Mar 31, 2024

bluetech Mar 21, 2024 •

edited

Loading

jakkdl Mar 31, 2024 •

edited

Loading