Deprecation of `msg=` for both `pytest.skip()` and `pytest.fail()`. #8950

symonk · 2021-07-27T23:02:06Z

Hi all, hope you are well :) I have an initially poc at refactoring the pytest.skip(msg=...) syntax in favour of a reason= arg in the sig, I have attempted to deprecate this for now in the simplest way I can think possible. I have ran out of time tonight so opened this PR for a discussion in the interim.

I am not familiar with how we deprecate things in general here in pytest, especially not function signature args. Please let me know what you think, if i'm on the right track etc.

We have a few circular imports I need to spend more time on, we should maybe also consider OutcomeException using a reason attr itself, however I'm aware there are other use cases that currently use msg=.

closes #8948

This needs a lot of work, opening it as a draft

…g= instead

symonk · 2021-07-27T23:08:48Z

@The-Compiler @RonnyPfannschmidt @nicoddemus opened a draft here, I am unsure the best approach for something like this so I welcome any feed back, I have ran out of time this evening so I thought I would try get a conversation started around it for me to pick up later and tweak, few queries:

Should OutcomeException attr be reason
I'll need to investigate the circle imports going on here around the deprecation imports
Another outlier with msg= I think its pytest.fail will I include that and unify across the board?
Is there a better way to deprecate this than my approach?
Any other tests you'd like to see added?
Update docs
Deprecate all msg= functions
Change log

src/_pytest/outcomes.py

RonnyPfannschmidt

great start 👍 , please address the minor nitpicks

src/_pytest/outcomes.py

symonk · 2021-07-28T20:53:01Z

I have excluded exit(msg=...) on purpose here for now pending core feedback on it

src/_pytest/outcomes.py

nicoddemus

@symonk thanks for the PR, great work!

I left some minor comments, please take a look.

Also, it is important to add a section to doc/en/deprecations.rst mentioning the deprecation, rationale, and how to update existing code. After doing that, add a link to the new section to the changelog/8948.deprecation.rst file.

changelog/8948.deprecation.rst

src/_pytest/outcomes.py

changelog/8948.deprecation.rst

src/_pytest/outcomes.py

testing/test_skipping.py

src/_pytest/outcomes.py

symonk · 2021-07-30T19:10:48Z

@symonk thanks for the PR, great work!

I left some minor comments, please take a look.

Also, it is important to add a section to doc/en/deprecations.rst mentioning the deprecation, rationale, and how to update existing code. After doing that, add a link to the new section to the changelog/8948.deprecation.rst file.

As always, thanks again! appreciate it.

doc/en/deprecations.rst

nicoddemus

Looks great, thanks! Left two minor changes, otherwise LGTM!

Co-authored-by: Bruno Oliveira <[email protected]>

symonk · 2021-07-31T00:33:24Z

pre-commit.ci run

symonk · 2021-08-02T16:41:09Z

thoughts on getting this in for 7.0? would let us be uniform by 8.1 in this regard atleast - Thoughts on pytest.exit(msg...) - I am happy to integrate it here but i wasen't sure if we want that

nicoddemus · 2021-08-02T17:41:07Z

I'm fine with getting this for 7.0. 👍

The-Compiler · 2021-08-02T19:04:42Z

Fine by me, given that 7.0 is delayed a bit more anyways (I intend to pick it up again in 2-3 weeks after I've had some holidays)

symonk · 2021-08-03T18:19:31Z

please ignore spending any time looking at this until I sort it out, as soon as possible :) I'm going to implement deprecation as a decorator to handle all 3 cases, pytest.exit() having a non optional msg arg made things extra tricky (maybe im just over thinking it) and I want to handle all 3 cases in a clean manner

nicoddemus · 2021-08-05T11:42:49Z

'm going to implement deprecation as a decorator to handle all 3 cases, pytest.exit() having a non optional msg arg made things extra tricky (maybe im just over thinking it) and I want to handle all 3 cases in a clean manner

Cool, thanks!

Also reminder to then mention pytest.exit in the docs too.

symonk · 2021-08-05T11:45:33Z

'm going to implement deprecation as a decorator to handle all 3 cases, pytest.exit() having a non optional msg arg made things extra tricky (maybe im just over thinking it) and I want to handle all 3 cases in a clean manner

Cool, thanks!

Also reminder to then mention pytest.exit in the docs too.

will do, got it done locally just need to test my changes but I've had no time the past 2 days, hopefully do it tonight because i'm away this weekend :)

symonk · 2021-08-05T20:03:29Z

@nicoddemus any advice on how best to handle the exit() case, by default it is msg: str (non optional) which is a little trickier, is it acceptable to change that sig to reason: str = "" and apply the same solution as the others here?

alternatively, all I can think of is something like a decorator like:

def deprecate_msg_for_reason(f):
    @functools.wraps(f)
    def wrapper(*args, **kwargs):
        if "msg" in kwargs:
            kwargs["reason"] = kwargs["msg"]
            del kwargs["msg"]
            warnings.warn(KEYWORD_MSG_ARG.format(func=f.__name__), stacklevel=2)
            return f(*args, **kwargs)
    return wrapper

Curious on some feed back before i spend time implementing it, the problem with the decorator approach is it leaves a lackluster sig where an arg is probably defined but unused, or like:

def exit(reason: str, returncode: Optional[int] = None, **kwargs) -> "NoReturn":
def exit(reason: str, returncode: Optional[int] = None, msg: Optional[str] = None) -> "NoReturn":

which feels bad to me, in the decorator approach neither are used in the function scope

nicoddemus · 2021-08-05T20:05:21Z

Hey @symonk

is it acceptable to change that sig to reason: str = "" and apply the same solution as the others here?

I think so, any reason you feel this should be handled differently?

symonk · 2021-08-05T20:06:11Z

Thanks @nicoddemus I wasen't sure if there was a reason why pytest.exit() enforced a msg from the user w/o a default, i guess not? :)

edit: That makes my life a little easier! I was cautious of changing as I assumed there was a reason behind why it was non optional

nicoddemus · 2021-08-05T20:15:44Z

Ahh I see! Sorry, now I get why you asked.

But we need to have a message associated with it, otherwise we don't have a reason to show.

How about this?

def exit(reason: str="", *, msg: Optional[str]=None):
    if reason and msg:
        raise UsageError("Cannot pass both reason and msg to exit")
    if not reason:
        if msg is None:
            raise UsageError("exit() needs a reason argument")
        warnings.warn("msg is deprecated, use reason instead")    
        reason = msg
    # use `reason`

We can then explain in the docstring that reason has a default value only because msg is deprecated, but exit needs one argument.

symonk · 2021-08-05T20:32:44Z

Ahh I see! Sorry, now I get why you asked.

But we need to have a message associated with it, otherwise we don't have a reason to show.

How about this?
def exit(reason: str="", *, msg: Optional[str]=None):
    if reason and msg:
        raise UsageError("Cannot pass both reason and msg to exit")
    if not reason:
        if msg is None:
            raise UsageError("exit() needs a reason argument")
        warnings.warn("msg is deprecated, use reason instead")    
        reason = msg
    # use `reason`
We can then explain in the docstring that reason has a default value only because msg is deprecated, but exit needs one argument.

I like it, i will keep the reuse for skip/fail as is, but i did wonder if calling fail() for the exit scenario would cause a problem, this is a happy middle ground 👍

symonk · 2021-08-05T21:02:09Z

out of time tonight, need to look into a few linting oddities in:

src/_pytest/scope.py:62: error: Missing return statement  [return]
Found 1 error in 1 file (checked 4 source files)

ailing in CI too, kinda puzzled as i haven't touch that file, i did change fail() tho ofc, maybe to do with that, will chase that up later

and tidy up a few loose ends, will prompt for review when I finish up, thanks again.

mini task list for later:

fix linter
review docs
review test coverage
review calling fail() inside a deprecated pytest.skip() scenario

symonk · 2021-08-05T21:07:07Z

src/_pytest/outcomes.py

+    if msg is not None:
+
+        if reason:
+            fail(


is this technically correct for the skip scenario? not sure.

To call fail you mean?

Hmmm you probably should not call fail, but raise UsageError.

The-Compiler · 2021-08-26T18:04:46Z

What's the state of this? I plan to get back to a 7.0 rc1 soon (possibly next week), and if this doesn't make it into the RC, I don't think we should have it in the 7.0 final either.

symonk · 2021-08-27T13:56:44Z

@The-Compiler I will set aside some time tomorrow morning and finish this off, I have some family stuff on after work this evening, hopefully it will be wrapped up by midday or so tomorrow.

edit: more than expected to sort when bringing main in ^^, working on it

….fail

symonk · 2021-08-28T09:11:35Z

src/_pytest/scope.py

            )
+            # I don't know why mypy complains here, `fail()` is typed as `typing.NoReturn`
+            # yet it complains of error: Missing return statement [return] ?
+            assert False, "unreachable"


Anyone savvy with mypy knows why it complains of a missing return here? the Scope instantiation is correctly catching the ValueError and fail() has a forward ref return type of "No Return". This seems unnecessary? to me

cc @bluetech

Looks like a mypy bug or at least something mypy can improve. Maybe the "missing return value" analysis does not consider unreachability (which is correctly detected in this case, I verified this by adding some statement after the fail and seeing that mypy complains that the statement is unreachable).

If you want to avoid the assert you can replace return ... with scope = ... and return scope outside of the try-except. Slightly less clean but OK I think.

Whatever you folks think is best I will happily implement, kind of a rock and a hard place here :)

keep assert as is

hold a reference scope and return it after the except

(may have some issues around local variables referenced before assignment that won't break anything in the latter)

In the mean time I will try recreate and open a mypy issue (if one is required)

I'd go with "hold a reference scope and return it after the except".

Reported to mypy would be nice, if you are able to reduce it to a standalone example. You should check if it has already been reported though.

@bluetech Will do! thanks

symonk · 2021-08-28T09:13:34Z

Moved to a UsageError instead of calling fail for bad args, Got a query on a mypy assert I had to add to make it work (which seems unnecessary to me, can't quite put my foot on why its complaining inside scope.py here. Will address code cov / outliers shortly

nicoddemus

LGTM, thanks a lot @symonk!

symonk · 2021-08-30T18:27:01Z

@nicoddemus @RonnyPfannschmidt @The-Compiler @bluetech - Thanks as always for all the feed back here, I think it is good to go (I've applied Rans latest suggestion), would be good to wrap this up for 7.0. If there is any other pressing issues we need some resource to look at as priority please send it my way.

The-Compiler

Phew, this turned out quite a bit more complex than I had imagined! Great work @symonk, thanks for the determination! 👍

The-Compiler · 2021-11-08T10:52:45Z

@RonnyPfannschmidt I think your review comments have all been addressed, correct?

RonnyPfannschmidt · 2021-11-08T11:18:58Z

Indeed

The-Compiler · 2021-11-08T14:31:46Z

Let's get this in then, thanks everyone!

symonk added 2 commits July 27, 2021 23:42

porting pytest.skip() to use reason=, adding tests

c9f199c

avoid adding **kwargs, it breaks other functionality, use optional ms…

94dbc1f

…g= instead

RonnyPfannschmidt reviewed Jul 28, 2021

View reviewed changes

src/_pytest/outcomes.py Outdated Show resolved Hide resolved

RonnyPfannschmidt reviewed Jul 28, 2021

View reviewed changes

src/_pytest/outcomes.py Outdated Show resolved Hide resolved

RonnyPfannschmidt requested changes Jul 28, 2021

View reviewed changes

deprecation of pytest.fail(msg=...)

8d69d07

symonk marked this pull request as ready for review July 28, 2021 19:55

symonk added 2 commits July 28, 2021 21:00

fix bug with not capturing the returned reason value

4b67ce0

pass reason= in acceptance async tests instead of msg=

a7e84bc

symonk commented Jul 28, 2021

View reviewed changes

src/_pytest/outcomes.py Outdated Show resolved Hide resolved

symonk requested review from RonnyPfannschmidt, The-Compiler and nicoddemus July 28, 2021 20:49

symonk commented Jul 29, 2021

View reviewed changes

src/_pytest/outcomes.py Outdated Show resolved Hide resolved

nicoddemus requested changes Jul 30, 2021

View reviewed changes

finalising deprecations of msg in pytest.skip() and pytest.fail()

faec46b

symonk changed the title ~~Deprecating pytest.skip(msg=...) in favour of pytest.skip(reason=...)~~ Deprecation of msg= for both pytest.skip() and pytest.fail(). Jul 30, 2021

nicoddemus reviewed Jul 30, 2021

View reviewed changes

doc/en/deprecations.rst Outdated Show resolved Hide resolved

nicoddemus reviewed Jul 30, 2021

View reviewed changes

doc/en/deprecations.rst Outdated Show resolved Hide resolved

nicoddemus approved these changes Jul 30, 2021

View reviewed changes

symonk and others added 2 commits July 30, 2021 23:15

Update doc/en/deprecations.rst

5bd7bab

Co-authored-by: Bruno Oliveira <[email protected]>

Update doc/en/deprecations.rst

1b7768c

Co-authored-by: Bruno Oliveira <[email protected]>

Merge branch 'main' into deprecate-skip-msg-for-reason

9fdefaa

fix failing test after upstream merge

d87ba07

adding deprecation to pytest.exit(msg=...)

4524086

add docs for pytest.exit deprecations

54e8a41

symonk commented Aug 5, 2021

View reviewed changes

symonk added 2 commits August 28, 2021 09:11

Merge branch 'main' into deprecate-skip-msg-for-reason

929ba85

finalising deprecation of msg for pytest.skip, pytest.exit and pytest…

e8558d0

….fail

symonk commented Aug 28, 2021

View reviewed changes

symonk requested a review from nicoddemus August 28, 2021 09:49

nicoddemus approved these changes Aug 30, 2021

View reviewed changes

hold a reference to the Scope instance to please mypy

d48173e

The-Compiler approved these changes Nov 8, 2021

View reviewed changes

RonnyPfannschmidt approved these changes Nov 8, 2021

View reviewed changes

The-Compiler merged commit eb6c449 into pytest-dev:main Nov 8, 2021

symonk deleted the deprecate-skip-msg-for-reason branch November 8, 2021 14:39

Uh oh!

Deprecation of msg= for both pytest.skip() and pytest.fail(). #8950

Deprecation of msg= for both pytest.skip() and pytest.fail(). #8950

Uh oh!

Conversation

symonk commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

symonk commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

symonk commented Jul 28, 2021

Uh oh!

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

symonk commented Jul 30, 2021

Uh oh!

Uh oh!

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

symonk commented Jul 31, 2021

Uh oh!

symonk commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus commented Aug 2, 2021

Uh oh!

The-Compiler commented Aug 2, 2021

Uh oh!

symonk commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus commented Aug 5, 2021

Uh oh!

symonk commented Aug 5, 2021

Uh oh!

symonk commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus commented Aug 5, 2021

Uh oh!

symonk commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus commented Aug 5, 2021

Uh oh!

symonk commented Aug 5, 2021

Uh oh!

symonk commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

The-Compiler commented Aug 26, 2021

Uh oh!

symonk commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Deprecation of `msg=` for both `pytest.skip()` and `pytest.fail()`. #8950

Deprecation of `msg=` for both `pytest.skip()` and `pytest.fail()`. #8950

symonk commented Jul 27, 2021 •

edited

Loading

symonk commented Jul 27, 2021 •

edited

Loading

symonk commented Aug 2, 2021 •

edited

Loading

symonk commented Aug 3, 2021 •

edited

Loading

symonk commented Aug 5, 2021 •

edited

Loading

symonk commented Aug 5, 2021 •

edited

Loading

symonk commented Aug 5, 2021 •

edited

Loading

symonk commented Aug 27, 2021 •

edited

Loading