(fixtures): Replace fixture represenation with a class #12473

Glyphack · 2024-06-18T10:27:53Z

During the sprint we discussed fixing the above issue and thought maybe it's a better idea to add a better represenation to fixtures. To do this without patching the object more, this PR refactors fixtures to have a class with attributes.
The main rational behind that is:

Now we have a type for fixtures makes it easier to check for fixture types and no need to perform cast on objects. Removed monkey patching.
They are no longer functions that carry a class within them.
When you decorate a function with a fixture it's not possible to call it anymore. So it makes more sense for it to not be a function or method.

Example

Previously we had:

===================================== short test summary info ======================================
FAILED tmp31nzhe4b.py::test_something - assert fixt == 42
======================================== 1 failed in 0.03s =========================================

where fixt is a pytest fixture function that is not replaced by it's value(directly used)

Now we print:

===================================== short test summary info ======================================
FAILED tmp31nzhe4b.py::test_something - assert pytest_fixture(<function fixt at *>) == 42 
======================================== 1 failed in 0.03s =========================================

testing/code/test_source.py

testing/test_compat.py

Glyphack · 2024-07-04T18:48:51Z

Hey @The-Compiler @RonnyPfannschmidt I rebased the PR could you please take a look?

changelog/11525.improvement.rst

src/_pytest/compat.py

src/_pytest/fixtures.py

webknjaz · 2024-07-04T19:58:01Z

testing/code/test_source.py

@@ -478,12 +478,13 @@ def deco_mark():
    def deco_fixture():
        assert False

-    src = inspect.getsource(deco_fixture)
+    # Since deco_fixture is now an instance of FixtureFunctionDef the getsource function will not work on it.
+    with pytest.raises(Exception):


Could you assert against a more specific exception? As I understand, this is not testing that something went wrong but it's not clear what exactly. Also, it's usually a good idea to use the match= regex with this helper.

Yes will apply. This tries to document the behavior more than testing anything. Before introducing the FixtureFunctionDefinition it was possible to run inspect.getsource on fixtures but now it's not. This is just describing the behavior. I'm open to removing it as well.

I changed it to a more specific error. Do you think we can remove the test? This just tests that you cannot use inspect on fixture functions.

I think we can remove this test. I removed it.

webknjaz · 2024-10-23T11:18:37Z

Hey @Glyphack, could you resolve the merge conflicts / rebase this?

Glyphack · 2024-10-23T13:51:42Z

Hey @Glyphack, could you resolve the merge conflicts / rebase this?

Yes will do today.

RonnyPfannschmidt

Iove where this is going

It opens up nice follow ups

RonnyPfannschmidt · 2024-10-25T10:07:21Z

src/_pytest/compat.py

-        new_obj = getattr(obj, "__pytest_wrapped__", None)
-        if isinstance(new_obj, _PytestWrapper):
-            obj = new_obj.obj
+    for _ in range(100):


I believe we can drop the loop as nesting is no longer available

You're correct. I removed the loop. We still need to keep the unwrapping behavior. Because we expect the function to also unwrap the decorated functions for example this test: https://github.com/Glyphack/pytest/blob/3d7416c2dd5dcf2d60a8b30636dcc90978c64215/testing/test_compat.py#L63

RonnyPfannschmidt · 2024-10-25T10:08:26Z

src/_pytest/compat.py

 def get_real_func(obj):
    """Get the real function object of the (possibly) wrapped object by
-    functools.wraps or functools.partial."""
+    :func:`functools.wraps`, or :func:`functools.partial`, or :func:`pytest.fixture`."""
+    from _pytest.fixtures import FixtureFunctionDefinition


I believe we can drop fixture definition from that , we should pass the definitions attribute

Are you suggesting to pass the FixtureFunctionDefinition object to this function? If I understand correctly this function is expected to take any object and return the real object according to this test testing the behavior:
https://github.com/Glyphack/pytest/blob/3d7416c2dd5dcf2d60a8b30636dcc90978c64215/testing/test_compat.py#L63

As fixture definitions are no longer wrapping functions into other functions

The suggestion is to no longer consider them wrappers and passing the function directly

@RonnyPfannschmidt Sorry I didn't fully understand your comment.

The current unwrapping behavior here is only for two cases:

The pytest fixture is applied on a function so I can display this case with Fixture(f)

The pytest fixture is decorated so we have deco(Fixture(f))

The first case is handled by obj = obj._get_wrapped_function() line and second case is handled by obj = inspect.unwrap(obj, stop=lambda x: type(x) is FixtureFunctionDefinition).

If we drop any of them we cannot handle one of those two cases.

But I feel I am missing your point but unwrapping to me are these two lines.

Decorators cannot apply to a fixture definition

So that should trigger a error

It's wrong even now

I was wrong about that but the behavior is still needed. For example this test fails if we don't do unwrapping in this function:
https://github.com/Glyphack/pytest/blob/a6318c944f907bc350922c3961365e9ecd8d3e81/testing/python/integration.py#L11

The reason is that getfslineno is using this function to unwrap a decorator:
https://github.com/Glyphack/pytest/blob/a6318c944f907bc350922c3961365e9ecd8d3e81/src/_pytest/_code/code.py#L1341

I think the solution is to make a separate function for the cases when we want to perform the unwrapping? What is your suggestion?

What I meant is that we no longer ought to pass fixture definition to anything that unwrapped

Definition is no longer a function

Perhaps we should drop the call altogether

Thanks I realized the reason my code required this check to be here was that I was getting objects using the fixture name instead of their actual name and mistakenly I calling this on non fixture objects. After fixing that I could remove the get_real_method and simplify this function.

src/_pytest/fixtures.py

RonnyPfannschmidt · 2024-10-25T10:15:07Z

src/_pytest/fixtures.py

+        return fixture_definition
+
+
+class FixtureFunctionDefinition:


We need to log a followup task for paramspec/return type annotation tracking and storing

Added. I just don't fully know what does this mean I added your comment but feel free to edit it with more context.

We should create an issue and reference it here, taking the opportunity of writing a rationale and what needs to be done in the issue.

@RonnyPfannschmidt can you do that, given you asked this originally and @Glyphack doesn't know exactly what you mean? I understand you mean for this to be a Generic which carries around the type information.

I gave this a try in #12999, so we don't need to create an issue after all in the end, we can continue on that PR once this one gets merged.

Glyphack · 2024-10-27T11:17:13Z

Thank you both for the review. I left some TODO comments I will address them in a follow up PR.

This is an internal class users don't need to know about it.

for more information, see https://pre-commit.ci

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>

for more information, see https://pre-commit.ci

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>

nicoddemus

Overall great work!

I think this almost ready to merge, and we can improve on the typing later on #12999.

nicoddemus · 2024-11-26T13:40:36Z

changelog/11525.improvement.rst

@@ -0,0 +1,3 @@
+The fixtures are now represented as fixture in test output.


This sentence by itself, without more context, is confusing.

Perhaps:

Suggested change

The fixtures are now represented as fixture in test output.

Fixtures are now clearly represented in the output as a "fixture object", not as a normal function as before, making it easy for beginners to catch mistakes such as referencing a fixture declared in the same module but not requested in the test function.

(Might be too verbose still).

nicoddemus · 2024-11-26T13:45:22Z

src/_pytest/fixtures.py

+        return fixture_definition
+
+
+class FixtureFunctionDefinition:


We should create an issue and reference it here, taking the opportunity of writing a rationale and what needs to be done in the issue.

@RonnyPfannschmidt can you do that, given you asked this originally and @Glyphack doesn't know exactly what you mean? I understand you mean for this to be a Generic which carries around the type information.

nicoddemus · 2024-11-26T13:45:52Z

src/_pytest/fixtures.py

+        function: Callable[..., Any],
+        fixture_function_marker: FixtureFunctionMarker,
+        instance: type | None = None,
+    ):


Suggested change

):

) -> None:

nicoddemus · 2024-11-26T13:46:19Z

src/_pytest/fixtures.py

+        self,
+        function: Callable[..., Any],
+        fixture_function_marker: FixtureFunctionMarker,
+        instance: type | None = None,


instance seems like it should be an object, not a type?

Suggested change

instance: type | None = None,

instance: object | None = None,

nicoddemus · 2024-11-26T13:51:56Z

src/_pytest/fixtures.py

@@ -1271,7 +1279,7 @@ def fixture(
    autouse: bool = ...,
    ids: Sequence[object | None] | Callable[[Any], object | None] | None = ...,
    name: str | None = None,
-) -> FixtureFunctionMarker: ...
+) -> FixtureFunctionDefinition: ...


Why did this type annotation changed? Seems like it should still return a marker if we are using the @fixture(...) syntax?

nicoddemus · 2024-11-26T13:53:32Z

src/_pytest/fixtures.py

@@ -1223,19 +1195,19 @@ class FixtureFunctionMarker:
    def __post_init__(self, _ispytest: bool) -> None:
        check_ispytest(_ispytest)

-    def __call__(self, function: FixtureFunction) -> FixtureFunction:
+    def __call__(self, function: FixtureFunction) -> FixtureFunctionDefinition:


~~I think we should get rid of FixtureFunction at this point...~~

Done that in #12999.

nicoddemus · 2024-11-26T14:24:29Z

src/_pytest/fixtures.py

+        return fixture_definition
+
+
+class FixtureFunctionDefinition:


I gave this a try in #12999, so we don't need to create an issue after all in the end, we can continue on that PR once this one gets merged.

bluetech

I like this change, thanks!

I left some comments, please take a look.

bluetech · 2024-11-26T14:35:37Z

src/_pytest/fixtures.py

+        instance: type | None = None,
+    ):
+        self.name = fixture_function_marker.name or function.__name__
+        self.__name__ = self.name


Can you add a comment on this line, explaining what it does? IIUC, it's to match the __name__ attribute of function objects, but it's not obvious.

bluetech · 2024-11-26T14:38:11Z

src/_pytest/fixtures.py

+    def __repr__(self) -> str:
+        return f"<pytest_fixture({self._fixture_function})>"
+
+    def __get__(self, instance, owner=None):


Can you add a comment explaining what does? IIUC, it makes the FixtureFunctionDefinition bind like a method, but it's not obvious so would be nice have a comment.

bluetech · 2024-11-26T14:40:14Z

src/_pytest/fixtures.py

@@ -1352,7 +1360,7 @@ def fixture(
 def yield_fixture(
    fixture_function=None,
    *args,
-    scope="function",
+    scope: _ScopeName = "function",


yield_fixture is a legacy thing and is intentionally not typed, so I would omit this change.

bluetech · 2024-11-26T14:43:29Z

src/_pytest/fixtures.py

+# TODO: paramspec/return type annotation tracking and storing
+class FixtureFunctionDefinition:
+    def __init__(
+        self,


These days I prefer to force keyword arguments in such cases:

Suggested change

self,

self,

*,

bluetech · 2024-11-26T14:46:17Z

src/_pytest/fixtures.py

+
+# TODO: paramspec/return type annotation tracking and storing
+class FixtureFunctionDefinition:
+    def __init__(


While this type is not public, it's seems like something that plugin might start to use directly. For this, we have an "opt in" mechanism which makes it clear the ctor is internal and can change between pytest versions without warning. The mechanism is to add a _ispytest: bool = False, parameter, always call with _ispytest=True inside of pytest, and issue a warning when it's False. There are many examples in the code you can copy from.

bluetech · 2024-11-26T14:50:25Z

testing/python/fixtures.py

+    @pytest.fixture
+    def fixture_function_def_test_method(self):
+        return self.i
+
+
+@pytest.fixture
+def fixture_function_def_test_func():


I would write tests that register fixtures as pytester tests, to avoid "polluting" the actual pytest-testsuite pytest session.

bluetech · 2024-11-26T14:52:02Z

src/_pytest/assertion/rewrite.py

@@ -472,7 +473,7 @@ def _format_assertmsg(obj: object) -> str:

 def _should_repr_global_name(obj: object) -> bool:
    if callable(obj):
-        return False
+        return isinstance(obj, FixtureFunctionDefinition)


Can you add a comment on this line?

bluetech · 2024-11-26T14:59:27Z

src/_pytest/fixtures.py

-        "https://docs.pytest.org/en/stable/deprecations.html#calling-fixtures-directly about how to update your code."
-    )
-
-    @functools.wraps(function)


The previous code used functools.wraps, while the new code does the wrapping "manually". But functools.wraps/functools.update_wrapper does some stuff like copy __doc__ and such. Are we losing that?

Glyphack commented Jun 20, 2024

View reviewed changes

testing/code/test_source.py Show resolved Hide resolved

Glyphack changed the title ~~(assertion): Use repr for pytest fixtures and set repr for pytest fixtures~~ (fixtures): Replace fixture represenation with a class Jun 20, 2024

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jun 20, 2024

Glyphack force-pushed the reprs-assert-callable branch 3 times, most recently from bf90fca to d25a8d9 Compare June 20, 2024 13:51

Glyphack commented Jun 20, 2024

View reviewed changes

testing/test_compat.py Outdated Show resolved Hide resolved

Glyphack marked this pull request as ready for review June 20, 2024 15:19

Glyphack force-pushed the reprs-assert-callable branch 2 times, most recently from 3d2ec33 to 84cc5b7 Compare July 4, 2024 18:48

webknjaz reviewed Jul 4, 2024

View reviewed changes

Glyphack force-pushed the reprs-assert-callable branch 3 times, most recently from f0324a5 to 9d2a916 Compare July 30, 2024 18:37

RonnyPfannschmidt mentioned this pull request Aug 19, 2024

Pytest integration reagento/dishka#124

Open

Glyphack force-pushed the reprs-assert-callable branch from 641a3a1 to bc0a0e5 Compare October 23, 2024 14:30

Glyphack requested review from webknjaz and RonnyPfannschmidt October 23, 2024 19:37

RonnyPfannschmidt approved these changes Oct 25, 2024

View reviewed changes

RonnyPfannschmidt added this to the 8.4 milestone Nov 12, 2024

Glyphack added 6 commits November 23, 2024 16:20

feat: use repr for pytest fixtures

41c7c2c

(fixture) create a new class to represent fixtures

cee9044

fix: update code to use new fixture definitions

004f29f

Update AUTHORS

224595a

chore: add changelog entry

8666dfb

add test case for asserting missing fixture

b724030

Glyphack and others added 26 commits November 23, 2024 16:20

refactor: remove _pytestfixturefunction attribute

b6f87a3

refactor: remove unused code

bc69c19

ci: set no cover for test function

da5e8e9

docs: fix docstring

ec7f7b1

refactor: replace attribute check with type check

f61c23c

chore: update changelog

144befb

test: add test for _get_wrapped_function

e556218

refactor: remove unused code

a48b2a0

refactor: replace type with isinstance

c7d8339

docs: update docstring

6ba2370

refactor: use instance check to print repr

3bc5ad1

docs: ignore FixtureFunctionDefinition from docs

04a0cde

This is an internal class users don't need to know about it.

[pre-commit.ci] auto fixes from pre-commit.com hooks

9822c94

for more information, see https://pre-commit.ci

Update changelog/11525.improvement.rst

786aec5

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>

Update src/_pytest/fixtures.py

afe72f6

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>

Update src/_pytest/fixtures.py

3b66f72

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

149cc30

for more information, see https://pre-commit.ci

Update src/_pytest/compat.py

eb3943c

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>

fix: update fixture repr in text

2891be5

fix: check TypeError raised

1734722

test: remove redundant test

d749473

Fix tests

fa2f2b5

Handle scenarios where fixture is renamed in decorator

1b5b4c7

Add follow ups as todo comments

6ee850a

Remove get_real_func loop

131e306

Use _get_wrapped_function to unwrap fixtures

f062ff6

Glyphack force-pushed the reprs-assert-callable branch from 50a001e to f062ff6 Compare November 23, 2024 15:21

Merge branch 'main' into reprs-assert-callable

7f1b8d3

nicoddemus reviewed Nov 26, 2024

View reviewed changes

bluetech reviewed Nov 26, 2024

View reviewed changes

		@@ -0,0 +1,3 @@
		The fixtures are now represented as fixture in test output.

	The fixtures are now represented as fixture in test output.
	Fixtures are now clearly represented in the output as a "fixture object", not as a normal function as before, making it easy for beginners to catch mistakes such as referencing a fixture declared in the same module but not requested in the test function.

	instance: type \| None = None,
	instance: object \| None = None,

(fixtures): Replace fixture represenation with a class #12473

Are you sure you want to change the base?

(fixtures): Replace fixture represenation with a class #12473

Conversation

Glyphack commented Jun 18, 2024 • edited Loading

Example

Glyphack commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glyphack Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

webknjaz commented Oct 23, 2024

Glyphack commented Oct 23, 2024

RonnyPfannschmidt 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

Glyphack Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glyphack Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glyphack Nov 17, 2024 • edited Loading

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

Glyphack commented Oct 27, 2024 • edited Loading

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

nicoddemus Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluetech 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

Glyphack commented Jun 18, 2024 •

edited

Loading

Glyphack Oct 23, 2024 •

edited

Loading

Glyphack Oct 27, 2024 •

edited

Loading

Glyphack Nov 15, 2024 •

edited

Loading

Glyphack Nov 17, 2024 •

edited

Loading

Glyphack commented Oct 27, 2024 •

edited

Loading

nicoddemus Nov 26, 2024 •

edited

Loading