Skip to content

Fixed asserting that no warnings were raised#5259

Merged
hugovk merged 2 commits into
python-pillow:masterfrom
radarhere:warns
Feb 17, 2021
Merged

Fixed asserting that no warnings were raised#5259
hugovk merged 2 commits into
python-pillow:masterfrom
radarhere:warns

Conversation

@radarhere

Copy link
Copy Markdown
Member

The following test is intended to check that no warnings are raised when opening and closing an image.

def test_closed_file():
def open():
im = Image.open(static_test_file)
im.load()
im.close()
pytest.warns(None, open)

If I change it like so

def test_closed_file():
    def open():
        Image.open(static_test_file)

    pytest.warns(ResourceWarning, open)

It passes (except in PyPy). So we know that a ResourceWarning is raised by my modification.

But if I change it back to assert that there are no warnings,

def test_closed_file():
    def open():
        Image.open(static_test_file)

    pytest.warns(None, open)

It still passes.

This is because we are not using pytest correctly.

https://docs.pytest.org/en/stable/warnings.html

To record with func:pytest.warns without asserting anything about the warnings, pass None as the expected warning type:

with pytest.warns(None) as record:
   warnings.warn("user", UserWarning)
   warnings.warn("runtime", RuntimeWarning)

assert len(record) == 2

This PR corrects our usage in various tests.

@nulano

nulano commented Feb 10, 2021

Copy link
Copy Markdown
Contributor

From https://docs.pytest.org/en/stable/warnings.html#custom-failure-messages:

If no warnings are issued when calling f, then not record will evaluate to True.

Would it be clearer to use assert not record instead?

@radarhere

Copy link
Copy Markdown
Member Author

Ok, I've pushed another commit for that change

@hugovk
hugovk merged commit 1857bf5 into python-pillow:master Feb 17, 2021
@hugovk

hugovk commented Feb 17, 2021

Copy link
Copy Markdown
Member

Good catch, thanks!

@radarhere
radarhere deleted the warns branch February 17, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants