Prohibit pytest.mark.usefixtures in pytest.param by Hawk777 · Pull Request #12828 · pytest-dev/pytest

Hawk777 · 2024-09-20T17:22:53Z

No description provided.

It doesn’t work, but it fails in a non-obvious way: the marker is accepted and then the fixture is silently not run. Fail with an explanatory message instead to save debugging time.

Hawk777 · 2024-09-20T17:30:37Z

I’m not certain this is the best place for this change. As far as I know, pytest.param is only useful to either pass to pytest.mark.parametrize or to Metafunc.parametrize, and usefixtures doesn’t work in either of those places, so this seemed logical to me, but I don’t know Pytest’s architecture.

Hawk777 · 2024-09-20T17:31:12Z

See GH-4112.

The-Compiler

Seems reasonable to me, and definitely is a step forward from silently ignoring those. Could you add a test case for this as well? TestMarkersWithParametrization in testing/python/metafunc.py seems like the right place to me.

Hawk777 · 2024-09-24T01:19:23Z

I can’t seem to run the tests locally (ERROR: /tmp/pytest/pyproject.toml: 'minversion' requires pytest-2.0, actual pytest-0.1.dev1+gc40a0d5'), so I’ve tried to do that in 0cb2599 and I’ll let CI see if I got it right.

Hawk777 · 2024-09-24T01:35:50Z

Looks like a pass. That fixup commit should probably be squashed once you’ve reviewed it (the merge/squash guidelines in CONTRIBUTING.md say that you’ll do that, but let me know if you prefer that I do).

Hawk777 · 2024-10-04T03:26:58Z

@The-Compiler just checking if there’s anything else you were waiting for from me? Not sure if you get notifications or not for messages that don’t ping you. No hurry obviously.

The-Compiler · 2024-10-04T05:59:01Z

Sorry for the radio silence! Seems fine to me, though I'd like a second pair of eyes on the question whether this is a good place to do this kind of thing. @RonnyPfannschmidt @nicoddemus any opinions?

nicoddemus

Thanks @Hawk777 this is a great start!

Sorry for the delay, and thanks for the ping.

Please take a look at my comments.

nicoddemus · 2024-10-04T09:12:32Z

+    def test_param_usefixtures_rejected(self, pytester: Pytester) -> None:
+        """Verify that passing usefixtures to pytest.param fails loudly rather than silently."""
+        pytest.raises(
+            ValueError,
+            lambda: pytest.param("x", [1], marks=pytest.mark.usefixtures("foo")),
+        )


I think it would be better for us to have an integration test instead, because in this it is important for us to see how the error will appear to the user: for example, it should not blow up an entire traceback of the pytest internals, it should show a succinct message pointing out the fixture, location, etc.

I would start by using pytest.fail(..., pytrace=False) or raising UsageError, whichever produces a better result.

@nicoddemus UsageError and ValueError look the same to me, like this (other than the exception name, obviously):

___________________ ERROR collecting test_foo.py _____________________ test_foo.py:6: in <module> pytest.param( /usr/lib/python3.12/site-packages/_pytest/mark/__init__.py:72: in param return ParameterSet.param(*values, marks=marks, id=id) /usr/lib/python3.12/site-packages/_pytest/mark/structures.py:92: in param raise ValueError( E ValueError: pytest.param cannot add pytest.mark.usefixtures; see https://github.com/pytest-dev/pytest/issues/4112

while pytest.fail looks like this:

___________________ ERROR collecting test_foo.py _____________________ pytest.param cannot add pytest.mark.usefixtures; see https://github.com/pytest-dev/pytest/issues/4112

So it looks like pytest.fail produces the least output (and without any Pytest internals mentioned). However, it also doesn’t mention the line number on the user test file where the error occurred (remember this happens at import time, not execution time, so it also doesn’t mention the function name), which the others (since they include traces) do. Is that a problem? It seems to me it would be useful to point the user at the specific param call that has the problem, but if you prefer the shorter output, I’ll go ahead with that.

So, given the above tradeoff, what’s your preference? ValueError, UsageError, or pytest.fail?

Hmm I see thanks.

Let's keep it ValueError for now then, I agree that the location/line number is really important -- due to how pytest.param is exposed in the API, this is reasonable to do for now (we could do some sys.frame() shenaningans to get the pytest.param location invocation and build up a message ourselves, but not sure it is worth doing that for this quite uncommon occurrence).

But we should move forward with the integration test I mentioned: it is important that we check that test_foo.py:6 and pytest.param( appear in the message (from your example posted above).

Also, we probably should put a link to the docs somewhere explaining that (possibly we can add a note to pytest.param documentation), rather than pointing to the GitHub issue.

Yep, I will definitely do the integration test change, just waiting to do that until we’ve nailed down what we want the behaviour to be (no point changing the test twice).

In terms of a link to documentation, my understanding was that one should be able to usefixtures in param, but it just didn’t work right now due to a bug. Is it actually intended that usefixtures not be used in param? Or do you think, even though it’s a bug, it should be in the manual (typically I don’t see bugs documented in manuals, but rather only bug trackers, on most projects)?

Yep, I will definitely do the integration test change, just waiting to do that until we’ve nailed down what we want the behaviour to be (no point changing the test twice).

Oh sure, thanks, just wanted to take the opportunity to point out what I would like to see checked in the integration test.

Is it actually intended that usefixtures not be used in param?

We don't have any plans to support usefixtures there (nor in fixtures), so we might as well place the error (as you are doing) and leave a note in the docs (even if that changes in the future, I think it is harmless to let users know the current situation explicitly).

I was hoping to make fixing this one commit, but I had no idea CI was going to come in and add extra commits, so that didn’t work out. So this is now b3f232a and ccc1e18, plus anything else that needs to follow it.

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>

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

nicoddemus

Thanks! 🙇

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>

Hawk777 · 2024-10-04T22:23:02Z

The warnings in the docs build AFAICT are unrelated to this patch, so I think this is good to go now?

nicoddemus · 2024-10-05T01:00:32Z

It is related:

 /home/docs/checkouts/readthedocs.org/user_builds/pytest/envs/12828/lib/python3.12/site-packages/_pytest/mark/__init__.py:docstring of _pytest.mark.param:21: WARNING: undefined label: 'pytest.mark.usefixtures' [ref.ref]

But I just fixed it!

I will leave it open for a few more days to give others a chance to review it, otherwise I will merge it.

Thanks again!

Hawk777 · 2024-10-05T01:08:43Z

Oh, sorry, somehow I didn’t see that message. I only saw things about Spinx 90 compatibility issues in CSS and Javascript references, or something like that (I can’t find the page any more now that the new commit is running in CI). Thanks for the review!

Prohibit pytest.mark.usefixtures in pytest.param

c40a0d5

It doesn’t work, but it fails in a non-obvious way: the marker is accepted and then the fixture is silently not run. Fail with an explanatory message instead to save debugging time.

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

The-Compiler reviewed Sep 21, 2024

View reviewed changes

fixup! Prohibit pytest.mark.usefixtures in pytest.param

0cb2599

nicoddemus requested changes Oct 4, 2024

View reviewed changes

Hawk777 and others added 3 commits October 4, 2024 14:21

fixup! Prohibit pytest.mark.usefixtures in pytest.param

bb4da1b

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>

fixup! Prohibit pytest.mark.usefixtures in pytest.param

b3f232a

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

2456003

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

nicoddemus reviewed Oct 4, 2024

View reviewed changes

Comment thread src/_pytest/mark/structures.py Outdated

nicoddemus approved these changes Oct 4, 2024

View reviewed changes

Hawk777 and others added 3 commits October 4, 2024 14:29

fixup! Prohibit pytest.mark.usefixtures in pytest.param

ccc1e18

fixup! Prohibit pytest.mark.usefixtures in pytest.param

3508622

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>

fixup! Prohibit pytest.mark.usefixtures in pytest.param

dd9ea62

Fix docs reference

fae9fe4

nicoddemus merged commit 23256dd into pytest-dev:main Oct 9, 2024

Zac-HD mentioned this pull request Jul 22, 2026

Cannot use pytest.mark.usefixtures() in pytest.param #4112

Closed

Conversation

Hawk777 commented Sep 20, 2024

Uh oh!

Hawk777 commented Sep 20, 2024

Uh oh!

Hawk777 commented Sep 20, 2024

Uh oh!

The-Compiler left a comment

Choose a reason for hiding this comment

Uh oh!

Hawk777 commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hawk777 commented Sep 24, 2024

Uh oh!

Hawk777 commented Oct 4, 2024

Uh oh!

The-Compiler commented Oct 4, 2024

Uh oh!

nicoddemus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hawk777 Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

nicoddemus Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

Hawk777 Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

nicoddemus Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

Hawk777 Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Hawk777 commented Oct 4, 2024

Uh oh!

nicoddemus commented Oct 5, 2024

Uh oh!

Hawk777 commented Oct 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Hawk777 commented Sep 24, 2024 •

edited

Loading

nicoddemus left a comment •

edited

Loading

nicoddemus Oct 4, 2024 •

edited

Loading