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

pytest.warns hides type errors in warnings.warn calls #10865

Closed
3 tasks done
zanieb opened this issue Apr 4, 2023 · 13 comments · Fixed by #11804
Closed
3 tasks done

pytest.warns hides type errors in warnings.warn calls #10865

zanieb opened this issue Apr 4, 2023 · 13 comments · Fixed by #11804
Labels
good first issue easy issue that is friendly to new contributor plugin: warnings related to the warnings builtin plugin status: help wanted developers would like help from experts on this topic type: bug problem that needs to be addressed

Comments

@zanieb
Copy link

zanieb commented Apr 4, 2023

  • a detailed description of the bug or problem you are having
  • [ ] output of pip list from the virtual environment you are using
  • pytest and operating system versions
  • minimal example if possible

When using pytest.warns, incorrect values sent to warnings.warn are not reported obscuring bugs:

import pytest
import warnings


def test_example_one():
    with pytest.warns(UserWarning):
        warnings.warn(1)

    # This test passes but should raise a `TypeError` as demonstrated in `example_two`


def test_example_two():
    with pytest.raises(TypeError):
        warnings.warn(1)
macOS 13.2.1
pytest 7.2.2
python 3.11

Related to #9288

@nicoddemus
Copy link
Member

Hmm that's unfortunate, thanks for the report!

Should be simple to fix.

@zanieb
Copy link
Author

zanieb commented Apr 4, 2023

You're welcome! Certainly confused me for a minute :)

I'm happy to contribute a fix — want to just raise a type error if it's not a string? Looks like CPython allows Warning types too https://github.com/python/cpython/blob/f513d5c80672c76acbdaf7d5b601f4bbe9fae56a/Lib/warnings.py#L299-L301

@zanieb
Copy link
Author

zanieb commented Apr 4, 2023

While trying to find the logic where this TypeError is raised I've gotten a bit confused. Weirdly this is fine

❯ python -c "import warnings; warnings.warn(1)"
<string>:1: UserWarning: 1

but the TypeError is definitely raised in pytest i.e. if I remove the pytest.raises:

    def test_example_two():
>       warnings.warn(1)
E       TypeError: expected string or bytes-like object, got 'int'

I feel like this caused me an error outside of pytest as well though

@nicoddemus
Copy link
Member

Are you sure? This works for me:

import warnings
def test_example_two():
    warnings.warn(1)

Which Python and pytest versions are you using?

@zanieb
Copy link
Author

zanieb commented Apr 16, 2023

The Python and pytest versions are included in the original post, but consider me very confused — I did not reproduce this with the minimal example and a test matrix at https://github.com/madkinsz/pytest-10865/actions/runs/4714285419/jobs/8360551859

It does however reproduce quite clearly when I'm working in PrefectHQ/prefect. Here's the example running in our test suite PrefectHQ/prefect#9231

@zanieb
Copy link
Author

zanieb commented Apr 16, 2023

The issue appears to be related to filterwarnings, for example:

import warnings

# Does not raise a type error
warnings.warn(1)

# Raises a type error
with warnings.catch_warnings():
    warnings.filterwarnings("ignore", "test")
    warnings.warn(1)

with the addition of a warnings filter to pytest zanieb/pytest-10865@1c3e4e3 the type error is raised on all Python and 7.x pytest versions https://github.com/madkinsz/pytest-10865/actions/runs/4714336351

@zanieb
Copy link
Author

zanieb commented Apr 16, 2023

I've opened a CPython issue as this doesn't really seem like pytest's fault python/cpython#103577 but pytest probably shouldn't hide this error still.

@zanieb
Copy link
Author

zanieb commented Jun 14, 2023

@nicoddemus I don't expect there to be changes upstream for this. Should we validate that argument is of the correct type in pytest?

@RonnyPfannschmidt
Copy link
Member

as far as im concerned, type metadata in typeshed is provided, and my understandig is, that it would point to the error

@zanieb
Copy link
Author

zanieb commented Jun 14, 2023

@RonnyPfannschmidt the problem here is that when using filterwarnings the TypeError is raised outside of pytest but not raised inside pytest.warns blocks. This means that an error that can occur at runtime for users is hidden during execution of the test suite. I agree that this mistake would also be caught by a type-checker, but many people do not use static analysis tools and it feels rough to obscure the error that the standard library would raise.

@RonnyPfannschmidt RonnyPfannschmidt added type: enhancement new feature or API change, should be merged into features branch status: help wanted developers would like help from experts on this topic labels Jun 14, 2023
@avshisnyk avshisnyk mentioned this issue Jun 15, 2023
@Zac-HD Zac-HD added type: bug problem that needs to be addressed and removed type: enhancement new feature or API change, should be merged into features branch labels Aug 22, 2023
@Zac-HD
Copy link
Member

Zac-HD commented Aug 22, 2023

Retagging as bug based on upstream discussion, where consensus looks like "this should always error" and CPython may change accordingly.

@whysage
Copy link
Contributor

whysage commented Jan 12, 2024

Hi, please review
#11804

@whysage
Copy link
Contributor

whysage commented Jan 24, 2024

Hi, any comments on pull request?
#11804
@zanieb @Zac-HD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue easy issue that is friendly to new contributor plugin: warnings related to the warnings builtin plugin status: help wanted developers would like help from experts on this topic type: bug problem that needs to be addressed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants