pytest icon indicating copy to clipboard operation
pytest copied to clipboard

add paramspec to callable forms of raises/warns/deprecated_call, rewrite tests to use CM form

Open jakkdl opened this issue 10 months ago • 20 comments

deprecation

~~Deprecates the legacy callable forms for pytest.raises, pytest.warns and pytest.deprecated_call, and~~ replaces all instances in the code base of them aside from code specifically for testing them.

paramspec

pytest.raises, warns & deprecated_call previously typed *args and **kwargs as Any in the legacy callable form, so this did not raise errors:

def foo(x: int) -> None:
    raise ValueError
raises(ValueError, foo, None)

but now it will give call-overload.

It also makes it possible to pass func as a kwarg, which the type hints previously showed as possible, but it didn't work.

It's possible that func (and the expected type?) should be pos-only, as this looks quite weird:

raises(1, 2, kwarg1=3, func=my_func, kwarg2=4, expected_exception=ValueError)

but if somebody is dynamically generating parameters to send to raises then we probably shouldn't ban it needlessly; and we can't make func pos-only without making expected_exception pos-only, and that could break backwards compatibility.

  • [x] Include documentation when adding new features.
  • [x] Include new tests or update existing tests when applicable.
  • [X] Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.
  • [x] Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

Noticed while working on #13192

jakkdl avatar Feb 20 '25 10:02 jakkdl

I believe the key issue is that back when the api was initially created, neither typing nor POS only arguments where a thing

In hindsight, both func and expected exceptions should be POS only arguments

Some kind of deprecating overload would be needed (no cluesif typing supports anyof that)

I'm thinking whether we should just downright deprecate the call form

Back in python 2.5 times when we had no with statement there only was a call form and a exec form (multiline stringto eval)

Now that soon python 3.8 support is dropped we perhaps ought to just go context only and plan accordingly

RonnyPfannschmidt avatar Feb 20 '25 10:02 RonnyPfannschmidt

I believe the key issue is that back when the api was initially created, neither typing nor POS only arguments where a thing

In hindsight, both func and expected exceptions should be POS only arguments

I could make them pos-only for RaisesExc and RaisesGroup in #13192 to start with, so we only need to do deprecation on pytest.raises itself.

Some kind of deprecating overload would be needed (no cluesif typing supports anyof that)

@warnings.deprecated does indeed allow for deprecating individual overloads.

I'm thinking whether we should just downright deprecate the call form

Back in python 2.5 times when we had no with statement there only was a call form and a exec form (multiline stringto eval)

Now that soon python 3.8 support is dropped we perhaps ought to just go context only and plan accordingly

the docs does say

This form was the original pytest.raises() API, developed before the with statement was added to the Python language. Nowadays, this form is rarely used, with the context-manager form (using with) being considered more readable. Nonetheless, this form is fully supported and not deprecated in any way.

(emphasis mine) which kind of sounds like us wanting to support it indefinitely, but I have never seen the form used outside of the pytest repo itself, so I think we should be plenty fine deprecating it.

jakkdl avatar Feb 20 '25 15:02 jakkdl

"well, let's just deprecate them then!" ...

thankfully I had LLM generate a regex that did a lot of the replacements, but, uh, fun times.

I don't think there's much to gain from deprecating pytest.raises(expected_exception=ValueError)?

TODO:

  • [x] Changelog
  • [x] update PR title & description (lol)
  • [x] update docs

jakkdl avatar Feb 21 '25 17:02 jakkdl

the other stuff causing <100% diff coverage is me editing currently-uncovered code

jakkdl avatar Feb 24 '25 11:02 jakkdl

  • The code to support the legacy form seems pretty minimal to me, so we gain little by removing the support.

It's not a lot of supporting code at its face, but it took me a while to figure out gc troubles in 13192#2c8cd and raises could entirely be removed in place of a from ... export RaisesExc as raises after #13192.

It's also a question of whether to keep it maintained with RaisesExc - e.g. should it now accept the check parameter? If so, that will break code:

def my_fun(check):
    ...

def test_my_fun():
    pytest.raises(my_fun, check=5)

if we wanted to fully embrace it then maybe RaisesGroup should have a callable form, but that'd be kinda silly.

  • Changing to the context manager is non-trivial to do, which will cause a lot of friction in older codebases.

it'd be nice to get a sense of how prevalent it is, I've personally never encountered it outside of the pytest code base itself.

So my vote on this is -0, as I think the disruption/friction outweighs the small benefit of removing the supporting code, but if everybody else thinks this is worthwhile, then I'm OK with it too.

I agree that it's not a slam-dunk, and if we deprecate it we should make sure to have a long deprecation cycle. Heck, we could try deprecating it and if there's complaints we consider reversing. (futuredeprecationwarning? and/or a message explicitly saying so)

jakkdl avatar Mar 03 '25 12:03 jakkdl

perhaps before deprecating we should see to have tools like ruff supply a code fix and/or have a ast-grep receipt to do the code fix

the code change itself is not that hard, but its a massive friction without a tool that does it

RonnyPfannschmidt avatar Mar 10 '25 08:03 RonnyPfannschmidt

perhaps before deprecating we should see to have tools like ruff supply a code fix and/or have a ast-grep receipt to do the code fix

the code change itself is not that hard, but its a massive friction without a tool that does it

I like that, I'll go open some issues. In order not to have this PR stall forever, let's do PendingDeprecationWarning for now.

edit: FutureDeprecationWarning -> PendingDeprecationWarning

jakkdl avatar Mar 10 '25 12:03 jakkdl

Let's see if the future warning triggers any issues for users

I'm on board however python warning is quite some unexpected fun sometimes

RonnyPfannschmidt avatar Mar 10 '25 17:03 RonnyPfannschmidt

hm, while the default python filter will hide PendingDeprecationWarning, it appears pytest does not. So having it be a PendingDeprecationWarning won't really change the direct impact to users, although we're signaling that they don't need to rush with a fix.

If we do want to hold off on even that then I should split this PR and we can merge the test changes + paramspec on their own

jakkdl avatar Mar 11 '25 15:03 jakkdl

Let's put it in for now

We can put it on defer if there's a problematic impact most recent user's shouldn't be affected

RonnyPfannschmidt avatar Mar 11 '25 15:03 RonnyPfannschmidt

codecov diff complaints are solely from me changing callable raises to cm-raises in code that's not covered on main/ - technically adding more uncovered lines

jakkdl avatar Mar 11 '25 16:03 jakkdl

@nicoddemus ? :)

jakkdl avatar Mar 31 '25 11:03 jakkdl

Ruff is actively working on a rule to autofix callable raises: https://github.com/astral-sh/ruff/pull/17368

I also wrote a rule for flake8-pytest-style but have seen no response from maintainers: https://github.com/m-burst/flake8-pytest-style/pull/332

So with ruff helping out I think there's no problem going ahead with this, we could possibly even suggest people make use of it in the error message

jakkdl avatar May 26 '25 10:05 jakkdl

@nicoddemus ? :)

Sorry, completely missed this.

To be perfectly honest, I'm still not 100% on board with this deprecation, but it seems I'm in the minority so I don't want to block this, especially because I'm not sure the impact will be that significant.

That said, it would be great to hear from other core team members (@pytest-dev/core) before we move forward.

nicoddemus avatar May 27 '25 00:05 nicoddemus

Thanks for the proposal @jakkdl. Sorry for not looking earlier, @nicoddemus's comment above prompted me to look.

Like @nicoddemus I also feel uneasy about deprecating the non-cm forms. The reason being, it will cause a lot of churn. There is a lot of code out there using these forms, and I think breaking old working code should only be done for very good reasons.

Looking at the PR I see 3 changes:

  1. Improve typing of call form with ParamSpec.
  2. Change pytest tests to not use call form
  3. Deprecate

1 and 2 LGTM. For 1, IIUC it doesn't pose backward compat issues right? And for 2, we should keep tests that check that the call forms actually work, but not use it in other tests. So if you move these changes to a separate PR I think we can merge it.

As for 3, I am +1 on completely removing it from our docs. A lint would be great. I'm even +0.5 on completely hiding it from type checkers using if TYPE_CHECKING, this way it doesn't show up in autocomplete and docs, etc (this one maybe only after a lint is available). But I'm -1 on actual deprecation and eventual removal.

(This is not a hard block if other devs feel differently. And to be frank, I didn't really try to understand the RaisesGroup motivation you mentioned in https://github.com/pytest-dev/pytest/pull/13241#issuecomment-2694138535...)

bluetech avatar May 27 '25 06:05 bluetech

And just to double-check, you're also against doing PendingDeprecationWarning? (which is the current state of the PR, I just haven't updated the OP) @nicoddemus @bluetech

I kind of think it should be possible for us to deprecate it at some point, the callable form really is quite archaic at this point. But I'm not opposed to letting the lint loose and then perhaps revisiting this in a few years.

(This is not a hard block if other devs feel differently. And to be frank, I didn't really try to understand the RaisesGroup motivation you mentioned in https://github.com/pytest-dev/pytest/pull/13241#issuecomment-2694138535...)

If we explicitly keep it on life-support I think that addresses most of my problems with keeping it around (i.e. don't need to care about updating it or anything).

jakkdl avatar May 27 '25 10:05 jakkdl

Let's prepare to do the deprecation but shedule it for something like 9.y or a timeline

We should have a due date

RonnyPfannschmidt avatar May 27 '25 10:05 RonnyPfannschmidt

And just to double-check, you're also against doing PendingDeprecationWarning? (which is the current state of the PR, I just haven't updated the OP)

Since I don't see a reason to have a PendingDeprecationWarning if there is no intent to eventually fully deprecate, and I am against that, then I must be against PendingDeprecationWarning as well, if I claim to be logical :)

bluetech avatar May 27 '25 18:05 bluetech

I thiiink I managed to cleanly separate out the deprecation, got it saved in a stash and will push it to a separate PR so it's saved somewhere if/when we want to do the deprecation.

I think the codecov misses looks good as well (i.e. any drop is due to uncovered code being longer), but would be nice to have an extra set of eyes on it.

jakkdl avatar Jun 04 '25 14:06 jakkdl

Just to make sure, did you keep some tests around specifically to verify that the legacy forms are still in working order?

yep, there's a bunch of them in testing/python/raises.py. Though it might make sense to consolidate them in a single section, they're currently spread out in the file according to feature.

Found three more stray raises calls when git greping to confirm the above.

jakkdl avatar Jun 10 '25 13:06 jakkdl