Context manager to ensure no warnings are issued

[nicoddemus]

Follow up to #9386, starting this thread to discuss how to support the use case of "inside the given with block, no warning should be generated".

Seems most users assumed this idiom:

with pytest.warns(None):
    ...

Would ensure that the code inside the with block would not issue any warning, raising an error if it did.

However that was never the case, and pytest.warns(None) would do nothing (warning or no warning generated). To warn users about this common mistake, we added a deprecation warning when None was passed to pytest.warns (#8677).

When 7.0.0rc1 was released, some issues were reported about this: #9402, #9386.

Users would like a way to ensure a block of code does not raise any warning, something which we never really supported.

Our current suggestion is to use warnings.catch_warnings(), followed by simplefilter("error"):

with warnings.catch_warnings():
    warnings.simplefilter("error")
    ...

However as mentioned in #9402, this does not apply directly if you have a dynamic check:

warn_typ = FutureWarning if input == "nc" else None
with pytest.warns(warn_typ):
    ...

So far we have the following proposals:

1. Keep things as is, improving the docs and suggesting catch_warnings (Improve pytest.warns() docs to clarify difference with catch_warnings() #9002).
2. Introduce a new context manager, pytest.does_not_warn() ([prerelease] pytest.warns() doesn't seem to work properly for coverage.py #9386 (comment)).
3. Change pytest.warns(None) to mean that no warning should be raised (FR: Revert pytest.warns(None) changes #9402 (comment)).

I think this is worth reaching a consensus before the 7.0.0 release is out.

[WarrenWeckesser]

Seems most users assumed this idiom [...] Would ensure that the code inside the with block would not issue any warning, raising an error if it did.

I mentioned over in #9002 that SciPy had three uses of pytest.warns(None). Since you are discussing a design issue, I want to clarify exactly what those tests were doing.

The tests were of the form:

with pytest.warns(None) as record:
    <do some stuff>
    <check record for warnings, and possibly raise an error>

The tests were not assuming that the pytest context code would raise an error if a warning occurred. Instead, these tests were doing their own error checking based on record. So they were really using pytest.warns(None) exactly like warnings.catch_warnings(record=True). (There is a pull request to make the change to catch_warnings.)

[bashtage]

Thanks for opening this. It definitely feels helpful to have a pytest-based method to assert that code produces no (visible) warnings. We (statsmodels) use the dynamic pattern in parameterized tests and sometimes with upstream dependencies (e.g., NumPy 1.23 warns but <=1.22 does not). I can imagine a couple of APIs that would be friendly.

1. Make None mean no warnings. Would change the intent of the deprecation but could raise warnings when used in a block with pytest.raises(None) and a warning is raised that this needs to be fixed by pytest 8.
2. Accept a sequence in pytest.raises so that an empty sequence would mean no warnings are raised. If more than 1 warning is in the seeuence, assert that all warnings listed are raised,

# pass
with pytes.warn([FutureWarning, RuntimeWarning]):
    warnings.warn("",FutureWarning)
    warnings.warn("",RuntimeWarning)

# error
with pytes.warn([FutureWarning, RuntimeWarning]):
    warnings.warn("",FutureWarning)

# error
with pytes.warn([]):
    warnings.warn("",FutureWarning)

This approach would replace the often indented use of None with and empty sequence.

FWIW NumPy has numpy.testing.assert_no_warnings (undocumented IIRC, but something like this)

[nicoddemus]

Gentle ping for the maintainers. 😁

I think it is worthwhile to discuss this, even if we decide to go with option 1 (do nothing).

[Zac-HD]

1. Make None mean no warnings. Would change the intent of the deprecation but could raise warnings when used in a block with pytest.raises(None) and a warning is raised that this needs to be fixed by pytest 8.

I'd be OK with either this, or with an explicit with pytest.does_not_warn(): helper. I prefer the latter but recognise that convenience actually does matter and the risk of misunderstandings and consequent test bugs isn't that bad.

I am strongly opposed to the old "allow any or no warnings" though; there's the stdlib warnings.catch_warnings(record) for that!

2. Accept a sequence in pytest.raises so that an empty sequence would mean no warnings are raised. If more than 1 warning is in the seeuence, assert that all warnings listed are raised, ...

pytest.warns() already accept a sequence of Warning classes, with the meaning "instance of any of these" - just like isinstance(..., (int, float)) for example. Same for pytest.raises(), though I think that's a typo?

[bashtage]

pytest.warns() already accept a sequence of Warning classes, with the meaning "instance of any of these" - just like isinstance(..., (int, float)) for example. Same for pytest.raises(), though I think that's a typo?

I checked the docs after I wrote that comment and saw that a sequence is accepted. This would then be the more forward-compatible way IMO.

exp_warn = FutureWarning if COND else [] # or (,)

with pytest.warns(exp_warn):
    code_with_possible_changes()

An empty sequence seems very consistent with the sequence interpretation that all warnings in the list must be raised and no others are allowed).

What happens now with an empty sequence? Is pytest.warns([]) the same as pytest.warns()?

The different context managers is a bit painful to use in runtime scenarios. It would need to be something like

cm = pytest.raises(FutureWarning) if COND else pytest.does_not_raise()
with cm:
    code_with_possible_changes()

[bashtage]

I am strongly opposed to the old "allow any or no warnings" though; there's the stdlib warnings.catch_warnings(record) for that!

I also think this is the right approach. Any or None is not a test, and so doesn't belong in a test runner.

[Zac-HD]

An empty sequence seems very consistent with the sequence interpretation that all warnings in the list must be raised and no others are allowed).

That's not quite right; the block must emit a warning such that isinstance(w, expected_warning) is True. For sequences, that's equivalent to writing any(isinstance(w, ex) for ex in expected_warning).

What happens now with an empty sequence? Is pytest.warns([]) the same as pytest.warns()?

any([]) == False, so this always fails: there are no possible warnings which are an instance of (). See #8646 for discussion of raising an earlier error to explain what's going on with that.

[bashtage]

Sorry - mental tick earlier when I wrote raises instead of warns.

I guess either way is a somewhat breaking change. The empty sequence feels low consequence since it cannot pass now. The None is probably cleaner in terms of reading code.

with pytest.warns(None):
    does_not_warn()

with pytest.warns([]):
    does_not_warn()

# Cleanest, but harder to use at runtime
with pytest.does_not_warn():
    does_not_warn()

[Zac-HD]

The empty sequence feels low consequence since it cannot pass now.

But "Special cases aren't special enough to break the rules." - the empty sequence already has a meaning, and I'd much rather use it to support user learning (with a specific error that explains that meaning) than give it a special-case alternative meaning. None is at least clearly different.

Cleanest, but harder to use at runtime

Yep, that's the tradeoff 😢

[olgarithms]

This issue is about introducing a feature that didn't exist before in pytest, therefore I am in favour of adding a new method to introduce a new functionality. pytest.does_not_warn() would have a very clear scope and wound't leave a lot of room for misinterpretations :)

It is true that the deprecation of pytest.warns(None) is breaking some projects, but I find that changing pytest.warns(None) to mean something else might be confusing for people who were using it correctly (or weren't using it because they understood it wasn't supported).

[bluetech]

TL;DR: I prefer option 2 - pytest.does_not_warn().

Even though warns(None) would have been the best IMO, I think we can't do it for pytest 7 since it breaks compat (especially given @WarrenWeckesser's comment). So an amended option 3 would be to let the deprecation period elapse (i.e. pytest 8.1) then do it.

For deciding between 1 and (2 or 3') I wonder if the pytest.does_not_warn() context manager can have any "value add" over catch_warnings: simplefilter('error'), or it would just do the same - turn the warning to an exception. I think we do want to show a stacktrace, so we do want to turn the warning to an exception, but we can display it more nicely than just an uncaught exception. So I'd go with 2 or 3'. Since I think does_not_warn is nice enough, I'd go with it.

[bashtage]

What about a single argument to does_not_warn, allow (or some better name), with a default value of None, so that something like

with pytest.does_not_warn(allow=[FutureWarning]):
    code_that_might_warn()

This isn't really an ideal test. I have used runtime configuration a lot across a few projects to handle drift in values accepted. For example, we moved from a parameter being "nc" to "n", so I want to check that using "nc" warns while using "n" does not. This happens in a parameterized test that checks these two values along with 3 other allowed values.

[Zac-HD]

If it's not parametrized, then just use the relevant context manager! If it is,

@pytest.mark.parametrize("arg, context", [
    ("old", pytest.warns(FutureWarning)),
    ("new", pytest.does_not_warn()),
])
def test(arg, context):
    with context:
        warns_for_old_not_for_new(arg)

I'm 👎 on the allow argument, because having complicated semantics instead of separate functions is exactly how we got into this mess in the first place. If you need to ignore an error, passing the test whether or not it happened, then the standard library with catch_warnings(): simplefilter("ignore", Warning); ... seems fine - I'm happy that it's more verbose because I think this tends to be a weak test.

[nicoddemus]

Agree with having pytest.does_not_warn(), without any arguments. 👍

I think we have a winner with pytest.does_not_warn(), seems like a good addition even for 7.0 as there's no chance of it breaking anything (being a new feature).

Anybody volunteers to work on a PR? 😁

[olgarithms]

Happy to take this!

[nicoddemus]

Awesome @olgarithms, please go ahead! 👍

[bashtage]

If it's not parametrized, then just use the relevant context manager! If it is,

This just doesn't lead to decent code when you have runtime detection of warning conditions. We have many parameterizations and only a few (deprecated ones) lead to warnings. So something like

exp_warn = FutureWarning if func_param in LIST_OF_BAD_PARAMS else None

which then would work well it was possible to pass a single argument to the warnings assertion checker. This current direction leads to chunkier code where the context manager needs to be passed around.

[The-Compiler]

Another idea which came to mind which I haven't seen so far: A special sentinel value in the pytest namespace, e.g. something like with pytest.warns(pytest.NoWarning):.

I think I'd prefer pytest.does_not_warn() too, though. I don't think having:

exp_warn = pytest.warns(FutureWarning) if func_param in LIST_OF_BAD_PARAMS else pytest.does_not_warn()
with exp_warn:
    ...

is much "chunkier" than

exp_warn = FutureWarning if func_param in LIST_OF_BAD_PARAMS else None
with pytest.warns(exp_warn):
    ...

is. Also, we indeed have some precedence for using pytest.raises and contextlib.nullcontext for parametrization already.

[bashtage]

Another idea which came to mind which I haven't seen so far: A special sentinel value in the pytest namespace, e.g. something like with pytest.warns(pytest.NoWarning):

I had also thought of this, and even the same name. I guess NoWarning could even be subclass of Warning. This said, it doesn't seem that much clearer than pytest.warns(None), aside from both changing the meaning of None (which is changing in pytest 8 anyway).

I think I'd prefer pytest.does_not_warn() too, though. I don't think having:

exp_warn = pytest.warns(FutureWarning) if func_param in LIST_OF_BAD_PARAMS else pytest.does_not_warn()
with exp_warn:
    ...

is much "chunkier" than

It is when you have a 79 character limit.

exp_warn = (
    pytest.warns(FutureWarning)
    if func_param in LIST_OF_BAD_PARAMS
    else pytest.does_not_warn()
)
with exp_warn:
    ...

[The-Compiler]

This said, it doesn't seem that much clearer than pytest.warns(None), aside from both changing the meaning of None (which is changing in pytest 8 anyway).

pytest 8? I assume you mean 7?

As we've seen, None can be understood as different things at that point. Either "no warnings are emitted", or "a warning of any type is emitted" (because we're passing a warning type to filter on, and we set that filter to None).

Also, using a value different from None would mean we can properly deprecate the None usage in pytest 7 rather than just changing its meaning outright.

It is when you have a 79 character limit.

Lines are free. I still think it's better to have a differently named function for doing something different.

[lafrech]

I might be late to the party but I just noticed the deprecation warning in my test suite and I'd like to complete the list of use cases in #9404 (comment).

This is what we do in flask-smorest:

        with pytest.warns(None) as record:
            response = client.get("/test/")
        if some_param is None:
            assert not record
        else:
            assert len(record) == 1
            assert record[0].category == UserWarning
            assert str(record[0].message) == "Some error message"

Basically, we use it to record warnings without asserting any or none, and we check recorded warning(s) depending on some parameter.

IIUC, without pytest.warns(None), we'll have to write two cases, the no warning case and the case with one warning, thus duplicating the code under test in the context (not really acceptable, that code could be many lines). Or we create the context in a first step and pass it around, but then we need to check the parameter several times:

        if some_param is None:
            ctx = warnings.catch_warnings()
        else:
            pytest.warns()
        with ctx:
            if some_param is None:
                warnings.simplefilter("error")
            response = client.get("/test/")
        if some_param is not None:
            assert len(record) == 1
            assert record[0].category == UserWarning
            assert str(record[0].message) == "Some error message"

The docs read

To record with pytest.warns() without asserting anything about the warnings, pass no arguments as the expected warning type and it will default to a generic Warning

but this might be misleading because it seems it does assert at least a warning is emitted.

I'd be happy if there was a simple API to do what I'm trying to do, that is record warnings in the context.

I guess we could use recwarn and assert not len(recwarn) just before the tested code. I find it less clear than the context manager, though.

Or maybe I'm missing something and there is a way to achieve what I want already.