fixtures: some tweaks & improvements #11209

bluetech · 2023-07-14T08:55:18Z

These are some relatively small changes I made while going over the fixture code. Please see the commits.

It makes for a more debuggable repr. Before: <function FixtureRequest._schedule_finalizers.<locals>.<lambda> at 0x7fe4ae32d440> After: functools.partial(<bound method FixtureDef.finish of <FixtureDef argname='hello_package' scope='package' baseid=''>>, request=<SubRequest 'hello_package' for <Function test_hello>>)

I can't imagine why we would want to test for a prefix here.

From understanding the code better I see this is the correct fix. The fixturedefs can be None if `request.getfixturevalue("doesnotexist")` is used. In practice there is no change in behavior because this mapping is used as `self._arg2fixturedefs.get(argname, None)` which ends up the same.

nicoddemus

Thanks @bluetech, great docs as always!

nicoddemus · 2023-07-14T14:13:27Z

src/_pytest/fixtures.py

-    """A container for a fixture definition."""
+    """A container for a fixture definition.
+
+    Note: At this time, only explicitly documented fields and methods are


cache_result is explicitly documented, but we consider it internal according to the changelog... is this definition incomplete, or am I missing something?

I think cached_result is not documented, maybe I'm missing something?

Hmm yeah sorry, I was not clear and I misunderstood: I thought "documented" meant the comments we leave on the attributes and that would appear in the docs, but that's not the case:

https://docs.pytest.org/en/stable/reference/reference.html#fixturedef

Only scope is explicitly documented, sorry for the confusion!

nicoddemus · 2023-07-14T14:15:16Z

src/_pytest/fixtures.py

-        self._arg2fixturedefs: Dict[str, List[FixtureDef[Any]]] = {}
-        self._holderobjseen: Set[object] = set()
+        # Maps a fixture name (argname) to all of the FixtureDefs in the test
+        # suite defined with this name. Populated by parsefactories().


Including external plugins, correct?

Thanks, might be worth clarifying that then:

Suggested change

# suite defined with this name. Populated by parsefactories().

# suite and external plugins defined with this name. Populated by parsefactories().

nicoddemus · 2023-07-14T14:16:43Z

src/_pytest/fixtures.py

@@ -1697,11 +1722,16 @@ def parsefactories(  # noqa: F811
    def getfixturedefs(
        self, argname: str, nodeid: str
    ) -> Optional[Sequence[FixtureDef[Any]]]:


Is getfixturedefs public API? If not, we could (in a future PR of course) change the return value to make the subtle difference between None and empty list more explicit.

It's not public. I'm not sure how to make it better though, interested to hear what you had in mind.

I had not thought specifics when I left the comment, but perhaps something to make it more type-safe and avoid chances for misuse:

@dataclass(frozen=True) class UndefinedFixture: pass @dataclass(frozen=True) class NoFixturesApplicable: pass def getfixturedefs( self, argname: str, nodeid: str ) -> Sequence[FixtureDef[Any]] | NoFixturesApplicable | UndefinedFixture:

This would force the caller to handle each individual case...

However perhaps this is sufficient:

@dataclass(frozen=True) class UndefinedFixture: pass def getfixturedefs( self, argname: str, nodeid: str ) -> Sequence[FixtureDef[Any]] | UndefinedFixture:

The original code feels like a small smell because we need to document the difference between None and [], and it is easy to treat both equally in a bool context.

But not so sure if worth it or not.

… to exception Fix pytest-dev#11208.

FixtureDef is used in the `pytest_fixture_setup` hook so needs to be public. However since its current internals are quite dubious (and not all marked with `_` prefix) I've added an explicit note that only documented fields/methods are considered public. Refs pytest-dev#7469.

Not used since 310b67b.

RonnyPfannschmidt · 2023-07-15T07:11:29Z

src/_pytest/fixtures.py

+        # No fixtures defined with this name.
+        if fixturedefs is None:
+            raise FixtureLookupError(argname, self)
+        # The are no fixtures with this name applicable for the function.
+        if not fixturedefs:
+            raise FixtureLookupError(argname, self)
        index = self._arg2index.get(argname, 0) - 1
-        if fixturedefs is None or (-index > len(fixturedefs)):
+        # The fixture requested its own name, but no remaining to override.
+        if -index > len(fixturedefs):
            raise FixtureLookupError(argname, self)


we should either merge the conditions or give a extra indication of the difference between not found, not applicable and no override avaliable

i'd prefer to add extra inication

I agree, I split the conditions to make the distinct cases clearer. But FixtureLookupError does some complex message formatting, it will require a bit of effort to pass the info along cleanly. I might do it in the future.

how about putting in some helpfull text constants right now as 3rd parameter and then following up with more intentional formatting whenever

I'm happy to attempt this in a follow up, in case @bluetech prefers to move this along.

I'm happy to leave this for a follow-up as well

@nicoddemus I'll be happy if you try it, it will require some changes in FixtureLookupError . (Note that FixtureLookupError if exported and its ctor is not doing the _ispytest check, though I doubt there are plugins with instantiate it directly).

FixtureLookupError already tries to handle the last case directly "recursive dependency involving fixture 'fix' detected", though it will be nice to distinguish between the first ("not found") and second ("not visible to this test") cases.

Gave this a quick try, but the result was not much better and it became a bit more complex I'm afraid.

bluetech added 3 commits July 10, 2023 23:27

fixtures: replace a startswith("conftest.py") with == "conftest.py"

2c80de5

I can't imagine why we would want to test for a prefix here.

RonnyPfannschmidt self-requested a review July 14, 2023 09:14

nicoddemus approved these changes Jul 14, 2023

View reviewed changes

bluetech added 4 commits July 15, 2023 10:06

fixtures: expand comments and annotations on fixture internals

01f38ac

fixtures: change FixtureDef.cached_result[2] from exception triplet…

9d0ddb4

… to exception Fix pytest-dev#11208.

fixtures: remove two unused functions

40ed678

Not used since 310b67b.

bluetech force-pushed the fixtures-doc-comments branch from 7c7e16e to 40ed678 Compare July 15, 2023 07:07

RonnyPfannschmidt reviewed Jul 15, 2023

View reviewed changes

bluetech merged commit 32f4808 into pytest-dev:main Jul 15, 2023

bluetech deleted the fixtures-doc-comments branch July 15, 2023 16:40

The-Compiler mentioned this pull request Jul 14, 2024

Performance regression with skipped tests since pytest 8.0.0 #12608

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixtures: some tweaks & improvements #11209

fixtures: some tweaks & improvements #11209

bluetech commented Jul 14, 2023

nicoddemus left a comment

nicoddemus Jul 14, 2023

bluetech Jul 14, 2023

nicoddemus Jul 14, 2023

nicoddemus Jul 14, 2023

bluetech Jul 14, 2023

nicoddemus Jul 14, 2023

nicoddemus Jul 14, 2023

bluetech Jul 14, 2023

nicoddemus Jul 14, 2023

RonnyPfannschmidt Jul 15, 2023

bluetech Jul 15, 2023

RonnyPfannschmidt Jul 15, 2023

nicoddemus Jul 15, 2023

RonnyPfannschmidt Jul 15, 2023

bluetech Jul 15, 2023

nicoddemus Jul 15, 2023

	# suite defined with this name. Populated by parsefactories().
	# suite and external plugins defined with this name. Populated by parsefactories().

fixtures: some tweaks & improvements #11209

fixtures: some tweaks & improvements #11209

Conversation

bluetech commented Jul 14, 2023

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment