Pytest: Replace internal warnings system by standard warnings and use them to make deprecations visible and later enforced

[RonnyPfannschmidt]

in order to support users in finding bad code, we should introduce Warnings subclasses similar to django

for example RemovedInPytest40Warning(DeprecationWarning)

we can then use the normal warnings modules to start and show those warnings, and even error out once we do a release

this enables a few positives

1. control over what deprecation show
2. disconnect breaking a feature from removing its code
   (a warning error can be disabled by users or subsequent releases depending on need)

[nicoddemus]

That's a great idea!

Particularly not a big fan of adding the deprecation name to the class, but it might be a matter of opinion because I can't point out a concrete problem with it at the moment.

One thing I wonder is what to do about the current config.warn function and the pytest_logwarning hook.

Here's config.warn implementation:

    def warn(self, code, message, fslocation=None, nodeid=None):
        """ generate a warning for this test session. """
        self.hook.pytest_logwarning.call_historic(kwargs=dict(
            code=code, message=message,
            fslocation=fslocation, nodeid=nodeid))

So it directly invokes the hook implementations.

Also the hook signature:

@hookspec(historic=True)
def pytest_logwarning(message, code, nodeid, fslocation):
    """ process a warning specified by a message, a code string,
    a nodeid and fslocation (both of which may be None
    if the warning is not tied to a partilar node/location)."""

Does not deal with any warning class at all.

Do you have an idea/proposal on what to do with the config.warn (and Node.warn for that matter) and the hook, and how those new classes will play out with the warnings plugin?

[RonnyPfannschmidt]

my oppinion is that config.warn was a mistake and i'd like to shift torwards using warnings and eventually deprecate it

[RonnyPfannschmidt]

btw, the introduction of the classes itself can be used in minor invisible refactorings, and then we can add the code to make them visible in the features branch as followup

[nicoddemus]

I agree with the general idea of deprecating config.warn and Node.warn and use plain warnings instead, with pytest-specific subclasses.

How about:

3.2

1. Create pytest-specific warnings:

   class PytestWarning(UserWarning):
   
   class PytestUsageWarning(PytestWarning):
      """Warnings about usage, for example "Test" prefixed classes
         with __init__ method.
      """
   
   class PytestDeprecationWarning(PytestWarning, DeprecationWarning):
      """
      For deprecating functionality. The functionality will be removed accordingly to our
      deprecation policy: either major version or major version + at least two minor versions.
      """
   
   

2. Create a new hook, pytest_warning_captured(warning, when, item), with warning being the warning instance, when being either "collect" (for warnings during collection) and "runtest" (in which case item is the running item). We can expand the when parameter later, but those two feels like they should cover a good ground.

3. Update warnings plugin to emit pytest_warning_captured and pytest_logwarning for backward compatibility. Also capture warnings during collection.

4. Update terminal plugin to use the new hook.

5. Show PytestUsageWarning and PytestPendingDeprecation by default.

6. Deprecate config.warn and Node.warn by emitting PytestDeprecationWarning from them.

7. Emit PytestDeprecationWarning if there are implementators for the pytest_logwarning hook.

4.0

1. Remove config.warn, Node.warn and pytest_logwarning.

What do you think?

[RonnyPfannschmidt]

@nicoddemus sounds fabulous - i do wonder about doing something like django and having RemovedInPytest40 warnings

as for the deprecation cycle, i would like to propose turning them into errors in 4.0 and removing the code in 4.1 (so we can easily allow users to reenable the affected bits or backpedal on removing something when a good case is made to keep it another iteration)

[RonnyPfannschmidt]

2517 introduced this one

[nicoddemus]

Reopening to track adding PytestWarning subclasses and changing config.warn to raise it instead.

[nicoddemus]

After some more thought I'm inclined on keeping config.warn and item.warn as shortcuts that raise an appropriate warning subclass but add useful information by default (for example the item node id and fslocation for item.warn).

[RonnyPfannschmidt]

@nicoddemus that needs a new hook and a new name, the old api is completely missfit and it will be a major mess for any consumer to change the behavior that way

python warnings differ in behaviour wrt encoding location, relation and types - at first glance it seems to unify those apis would be to create a major pain in some way

[nicoddemus]

I agree that we need a new hook, I proposed one here.

About a new name (I understand you mean a new function to replace, config.warn and item.warn), do you have a suggestion?

The current signature for config.warn is:

def warn(self, code, message, fslocation=None, nodeid=None):

Except for code, the rest can be mapped to a specific exception class (PytestWarning) and a suitable message using fslocation and nodeid when given.

If that seems messy (it might be), we will need a new function, but I don't know how to name it so it is not confusing.

[RonnyPfannschmidt]

i propose introducing std_warn and legacy_warn with warn being a alias triggering a deprecationwarning in the 3.x series explaining the breaking change in 4.x

then in 4.0 we can change to std_warn being an alias sheduled for removal in the 4 series or 5

[nicoddemus]

@RonnyPfannschmidt I just had an idea.

We are inclined in keeping config.warn and item.warn (or variants) as shortcuts that raise a warning and include useful information by default like fspath and test name, but that brings the problem that their signature is not really compatible, hence your suggestion of introducing std_warn and legacy_warn variants.

How about instead we make PytestWarning and subclasses optionally take an item and use that to include relevant information into the message automatically? This way we can retain the usability aspect without introducing a new function just to act as a "wrapper".

IOW, instead of:

if hasinit(self.obj):
    self.warn("C1", "cannot collect test class %r because it has a "
        "__init__ constructor" % self.obj.__name__)

We would write instead:

if hasinit(self.obj):
    msg = "cannot collect test class %r because it has a __init__ constructor" % self.obj.__name__
    warnings.warn(PytestWarning(msg, item=self))

PytestWarning then takes care of adding additional information to the message based on item (such as fspath location).

If I'm not missing anything, this would be simpler to implement and avoid adding confusing "warn" variants that have to be deprecated/switched in the future.

What do you think?

[RonnyPfannschmidt]

Wer create wrong warnings metadata unless we introduce a own wrapper and use waren_explizit

Im strictly opposed to put in to strings what we can correctly pass AS metadata

[nicoddemus]

Wer create wrong warnings metadata unless we introduce a own wrapper and use waren_explizit

Not sure what you mean, but I meant this:

class PytestWarning(UserWarning):

    def __init__(self, message, item=None):
        if item is not None:
            message = message + ('(item at %s)' % item.fslocation)
        UserWarning.__init__(message)
        self.item = item

Warnings (like exceptions) can add attributes that make sense to them (like OSError has an errno attribute).

[RonnyPfannschmidt]

@nicoddemus thats simply not how the warnings module passes on "fs locations" - the api that passes the data puts them in distinct parameters, and so should we

[RonnyPfannschmidt]

else we get a warning with one structured but wrong location information and one textual but messy to obtain correct one, which is pretty much to be considered abuse for any data consumer

[nicoddemus]

thats simply not how the warnings module passes on "fs locations"

Yep, I had foresee that as well, didn't dig into it yet to see if it will be a problem or not. In particular, I don't know how it will play with stacklevel for example. I will explore this a bit.

For the record, I agree with you that we shouldn't just change the text to point to "fslocation" and leave some other meta-data with the correct information.

[RonnyPfannschmidt]

@nicoddemus simple, we don't use warnings.warn ever for bits where we want to set the fslocation ourselfes

see https://docs.python.org/2/library/warnings.html#warnings.warn_explicit

i propose we take a look at adding a experimental pytest.warn function that helps with stacklevel and/or getting the location from a item/object

[nicoddemus]

I see thanks (that's the kind of exploration I planned to do later 😉)

I agree that it seems we do need a wrapper function in the end. pytest.warn is too close to pytest.warns though, will try to come up with a better name (other suggestions are welcome as well).

[RonnyPfannschmidt]

@nicoddemus we should brainstorm the facade for the new api - it has many pitfalls and the legacy cost of the previous failed experiment

[nicoddemus]

we should brainstorm the facade for the new api

Definitely! 👍

I think the idea of using standard warnings is a great improvement over the current system: we don't reinvent the wheel and users can use the standard filters to silence pytest's own warnings. So anything based on warning classes seem to me the way to go.

The warning "type" and message are simple I think, just use PytestWarning(message)

As for the location, we have a few options:

1. fslocation the filename/line number directly:
2. item gives item name and fslocaltion.

Any other cases?

[RonnyPfannschmidt]

we should try to mimic the existing api closely, else we just create a minefield (so there should be a category parameter) as for getting locations - we should have a very close look at the relations of items and warnings, else we create a false layering that will be a quite long term mistake

[RonnyPfannschmidt]

in response to #2652 we should also take a look at ensuring pytests warnings are usable by plugins and dont error out testsutes if users want own warnings as errrors

[Zac-HD]

One suggestion, based on my frustration with Django: define high-level exception and warning types, so users can catch a single thing to silence all problems in Pytest.

For example, Hypothesis has:

class HypothesisException(Exception): ...
class InvalidArgument(HypothesisException, TypeError): ...
class HypothesisWarning(HypothesisException, Warning): ...
class HypothesisDeprecationWarning(HypothesisWarning, FutureWarning): ... 

and plenty of others - the idea is that users can catch either the functional type which would usually be raised, or catch the Hypothesis-specific type if they want to e.g. ignore all our deprecation warnings but see others.

This is particularly useful if you have RemovedInPytestX warnings, where catching them all individually would be a pain.

[nicoddemus]

@Zac-HD thanks for the suggestion, this was similar to what I had in mind as well. 👍

[flub]

hey, just a quick not that I'll retract what I said on the mailing list about warnings. After reading this it does sound like a good approach and improvement (and I had forgotten just how weird the pytest warnings experiment was).

[nicoddemus]

@flub thanks for double checking that this is the right direction! 👍

[nicoddemus]

Closed by #3931