Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change pytest deprecation warnings into errors for 6.0 release #7362

Merged
merged 6 commits into from
Jul 23, 2020

Conversation

nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jun 13, 2020

@RonnyPfannschmidt: I updated some tests which didn't use from_parent.

@bluetech: I had to suppress the MINUS_K_COLON and MINUS_K_DASH warnings because they would turn into errors in 6.0, and we want to make them into errors in 7.0 only, given they were added too recently; we should reintroduce the warnings in 6.1 ( (#7361).

Closes #5584

@bluetech
Copy link
Member

I didn't know that this is how we handle deprecations (I assume it's: change pytest.PytestDeprecationWarning to an error in the first major release, then remove all deprecated & change PytestDeprecationWarning back to warning in the next feature release).

So there are actually a few other deprecations which will only be added in 6.0.x and I suppose require the same treatment:

  • FILLFUNCARGS
  • PYTEST_COLLECT_MODULE
  • WARNING_CAPTURED_HOOK

@RonnyPfannschmidt
Copy link
Member

I would like to propose introducing a deprecation helper similar to pip, where declaring a deprecation picks the warning /error class based on the current release and the removed_at parameter

@nicoddemus
Copy link
Member Author

I assume it's: change pytest.PytestDeprecationWarning to an error in the first major release, then remove all deprecated & change PytestDeprecationWarning back to warning in the next feature release

Exactly, that's what we have been doing since 4.0 and onward. 👍

So there are actually a few other deprecations which will only be added in 6.0.x and I suppose require the same treatment

Ahh thanks for catching those! 👍

I would like to propose introducing a deprecation helper similar to pip, where declaring a deprecation picks the warning /error class based on the current release and the removed_at parameter

You mean the one implemented here? https://github.com/pypa/pip/blob/master/src/pip/_internal/utils/deprecation.py#L62

Looks nice, but for pytest we often remove the warning-generating code together with the error message in X.1 releases: for example, in the past we removed deprecated functions (like _fillfuncargs is now deprecated), instead of keeping it around raising an error instead of a warning. How would that work then?

@nicoddemus
Copy link
Member Author

@RonnyPfannschmidt gentle ping. 😁

We might also change the deprecation mechanism past 6.0.

@RonnyPfannschmidt
Copy link
Member

@nicoddemus basically having a deprecation helper that goes by version_removed, we remove the need to have one release behave special and making the warning a error,

instead the deprecations trigger at the intended release (it makes pushing deprecations up to specific releases and also potentially working with calver much more fun

@nicoddemus
Copy link
Member Author

instead the deprecations trigger at the intended release

I believe you mean this bit:

    # Raise as an error if it has to be removed.
    if gone_in is not None and parse(current_version) >= parse(gone_in):
        raise PipDeprecationWarning(message)

My question is: under the current system we have, 6.0 users can turn those errors back into warnings (or even to ignore them) as a stop gap until 6.1 is released, at which point the feature and the warning will be effectively removed.

IIUC, in the proposed change this will not be possible anymore, because we will bypass the warnings module and raise PytestDeprecationWarning directly. Is that correct?

Full disclosure: I'm OK with changing the current mechanism, just wanted to make sure I'm understanding the proposal correctly.

@RonnyPfannschmidt
Copy link
Member

In the project where we adapted this for work we used warnings.warn and added a filter, the filter just also needed to be installed in a pytest hook to manage the warning capture systems

@nicoddemus
Copy link
Member Author

In the project where we adapted this for work we used warnings.warn and added a filter, the filter just also needed to be installed in a pytest hook to manage the warning capture systems

Installing a filter is what we do now to change those warnings into errors... unfortunately I'm still not grasping the core of your proposal, sorry. 😕

To recap, here's what we have today (using 5.X and 6.0 and 6.1 as examples to simplify):

  • Deprecations introduced in 5.X issue a warning PytestDeprecationWarning
  • In 6.0, we turn PytestDeprecationWarning warnings into errors using the standard warnings filter. This lets users update their warning filters to ignore/change those errors back into warnings, as a stop gap measure. The intent here is things to break hard, so users can't just pretend the warnings don't exist anymore, but they at least have the option to explicitly fall back to make PytestDeprecationWarning simple warnings again so they can prepare for 6.1.
  • In 6.1, we effectively remove the deprecated functionality (so just ignoring warnings won't help anymore). We also change PytestDeprecationWarning back into simple warnings, instead of errors, for new deprecations for 7.X.

So it is not yet clear to me if the new proposal would keep the overall workflow above working, or change it to something else.

@RonnyPfannschmidt
Copy link
Member

what i want to propose is to introduce a PytestExpiredDeprecationWarning which is filtered always, and then enabling plugins/filterwarnings to opt out of it if they want/need, and selection the deprecation type based on the intended removal version

@nicoddemus
Copy link
Member Author

what i want to propose is to introduce a PytestExpiredDeprecationWarning which is filtered always, and then enabling plugins/filterwarnings to opt out of it if they want/need, and selection the deprecation type based on the intended removal version

Ahh OK I think I got it, thanks!

We would then have a function like this:

def deprecated(reason: str, due: int):
    if due >= pytest version [0]:
        warnings.warn(PytestExpiredDeprecationWarning(reason))
    else:
        warnings.warn(PytestDeprecationWarning(reason))

And then we configure the default filters to always show PytestDeprecationWarning, and error out on PytestExpiredDeprecationWarning, correct?

@RonnyPfannschmidt
Copy link
Member

i would use the function to create the warnings/unformulated warnings that later on get passed into warnings.warn

@nicoddemus
Copy link
Member Author

i would use the function to create the warnings/unformulated warnings that later on get passed into warnings.warn

Got it. 👍

But all this I believe is best done in 6.1 and onward, I wouldn't like to change how deprecations are handled internally and exposed to users before a major release. What do you think?

@hugovk hugovk mentioned this pull request Jul 8, 2020
@nicoddemus
Copy link
Member Author

So there are actually a few other deprecations which will only be added in 6.0.x

Commented those as well, and will update #7361.

Also moved the warnings listing to the reference docs as I believe it is more appropriate there.

Created #7480 for the followup discussed here.

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicoddemus
Copy link
Member Author

@RonnyPfannschmidt @hugovk gentle ping. 😁

changelog/5584.breaking.rst Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@nicoddemus
Copy link
Member Author

I believe this is the last PR required for 6.0. 👍

@The-Compiler The-Compiler mentioned this pull request Jul 22, 2020
4 tasks
@The-Compiler The-Compiler added this to the 6.0 milestone Jul 22, 2020
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely missed that this PR was updated. LGTM.

@nicoddemus nicoddemus merged commit 7ec6401 into pytest-dev:master Jul 23, 2020
@nicoddemus nicoddemus deleted the deprecation-errors-5584 branch July 23, 2020 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change deprecation warnings into errors for 6.0
6 participants