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

Let pytest.warns check the warning's stacklevel #4343

Open
TomAugspurger opened this issue Nov 8, 2018 · 6 comments
Open

Let pytest.warns check the warning's stacklevel #4343

TomAugspurger opened this issue Nov 8, 2018 · 6 comments
Labels
plugin: warnings related to the warnings builtin plugin type: enhancement new feature or API change, should be merged into features branch

Comments

@TomAugspurger
Copy link

TomAugspurger commented Nov 8, 2018

This is a feature request for adding a check_stacklevel flag to pytest.warns. This helps library authors test that warnings point back to the user's source code, rather than some internal method.

An example: f is the public API exposed to users. g is some internal function that emits a warning.

import warnings
import pytest


def g(x):
    # some internal function
    warnings.warn("warning from g")


def f(x):
    # the public API
    g(x)


def test_foo():
    # proposal for new argument: check_stacklevel
    with pytest.warns(UserWarning, match="from g", check_stacklevel=True):
        f(2)

if __name__ == '__main__':
    f(2)

Ideally, test_foo would fail because the warning message points back to line 7, which is inside g

test_it.py:7: UserWarning: warning from g
  warnings.warn("warning from g")

The test would succeed if the warning pointed back to the call f(2), inside the test. This would require setting stacklevel=2 inside g.

Pandas added this check a couple years ago, and we've been quite happy with it: https://github.com/pandas-dev/pandas/blob/af602c5b702366d8b978ed04cafa371d63f6c32f/pandas/util/testing.py#L2707-L2718

It requires a frame hack, but a small one. Having it be a flag is necessary, in case an internal function is called from two places with different stacklevels. In pandas, we like the default of True. Perhaps this could be handled similarly to strict for xfails.

@blueyed blueyed added type: enhancement new feature or API change, should be merged into features branch plugin: warnings related to the warnings builtin plugin labels Nov 8, 2018
@bluetech
Copy link
Member

I think the idea of providing some assistance with testing that stacklevel is provided correctly is good. Certainly it's easy for that little number to get omitted, wrong or outdated.

The pandas code has a couple of problems:

  1. It only works for cases such as test_warns() -> do_something_that_warns(), which is common for deprecation and such, but doesn't work for test_foo() -> foo() -> do_something_that_warns(), which is common in other cases.

  2. It only checks filenames, which can lead to false negatives in case the frames are in the same file.

In this state I don't think it's good to add to pytest.warns. I'm not sure how to improve it however. catch_warnings only provides (optional) filename and lineno, not the full "origin stack", and that's not enough to fix the two issues above.

If we're fine with reducing the scope to only checking the filename, we can do this:

with pytest.warns(DeprecationWarning, filename='test_stuff.py'):
    deprecated_function()

which is the equivalent of

with pytest.warns(DeprecationWarning) as warning_messages:
    deprecated_function()
assert any(wm.filename == 'test_stuff.py' for wm in warning_messages)

To ease the common case, we can support filename=True to mean the current file.

I think that can be nice, but not 100% sure it's worth it.

@RonnyPfannschmidt
Copy link
Member

an helper that expects the warning to happen either in the current block, or in a specified function would help as well

@bluetech
Copy link
Member

I think checking for a function would be ideal, but it doesn't seem possible to do cleanly, without hacks like reverse-mapping filename+lineno to a function and such.

@Zac-HD
Copy link
Member

Zac-HD commented Jun 19, 2023

I think checking for a function would be ideal, but it doesn't seem possible to do cleanly, without hacks like reverse-mapping filename+lineno to a function and such.

Agreed that a function name would be ideal.

Happily I also think that such a reverse mapping is in fact practical for most functions. We get the filename and lineno from the warning; we check whether the filename matches the __file__ of anything in sys.modules; if so we can iterate over attributes of that module, check the line numbers in the .__code__ attribute of the function, and grab the __name__ of the matching function (again, if any).

If you accept a function object instead of name (which I'd support as an option), we can skip all the iteration - we've got fn.__code__ right there, and sys.modules[fn.__module__].__file__ isn't much harder - though of course it's still fallible. In either case, I'd probably ignore cases where we couldn't determing the function and only fail if we get a location which doesn't match.

@seberg
Copy link

seberg commented Aug 9, 2024

If you are happy to hard-code/vendor more of the warning handling you could implement catch_warnings and easily override warnings.showwarning to store/compare the stack:

sys._getframe().f_back.f_back

with the current stacklevel (or an additional check_stack_depth=0 that allows saying that you expect it to be deeper, i.e. you have to walk higher when checking to reach the callers stack).

Dunno if it is worth the trouble with vendoring and potential Python changes. but maybe it is actually nicer then inspecting files.

@RonnyPfannschmidt
Copy link
Member

Checking stack level cannot be done safely, what can be done is asserting a warning happens within a function via the record attributes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: warnings related to the warnings builtin plugin type: enhancement new feature or API change, should be merged into features branch
Projects
None yet
Development

No branches or pull requests

6 participants