Treating warnings as errors may interrupt teardown

[PiotrBatko-BlockchainWares]

Description

Treating warnings as errors (e.g. with option -W error) may interrupt teardown. When warning is reported (with warnings.warn) during teardown, teardown is stopped. Teardown interruption is serious problem which leads to leak of resources (e.g. processes are not closed or temporary files are not deleted).

Expected behavior is teardown completion and then displaying warning message as error.

Minimal example

import warnings

import pytest


@pytest.fixture
def example_fixture():
    yield
    print('Teardown started')
    warnings.warn('Some warning')
    print('Teardown finished')


def test_teardown_bug(example_fixture):
    pass

Without -W error, teardown is fully executed correctly. (Note "Teardown finished" in output below).

$ pytest -s
======================================== test session starts ========================================
platform linux -- Python 3.8.12, pytest-7.0.0, pluggy-1.0.0
rootdir: /home/dev/Desktop/pytest-bug-reproduction
collected 1 item                                                                                    

test_teardown_bug.py .Teardown started
Teardown finished


========================================= warnings summary ==========================================
test_teardown_bug.py::test_teardown_bug
  /home/dev/Desktop/pytest-bug-reproduction/test_teardown_bug.py:10: UserWarning: Some warning
    warnings.warn('Some warning')

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=================================== 1 passed, 1 warning in 0.00s ====================================

When warnings are treated as errors, then teardown is interrupted. (Note that now "Teardown finished" is missing in output below).

$ pytest -s -W error
======================================== test session starts ========================================
platform linux -- Python 3.8.12, pytest-7.0.0, pluggy-1.0.0
rootdir: /home/dev/Desktop/pytest-bug-reproduction
collected 1 item                                                                                    

test_teardown_bug.py .Teardown started
E

============================================== ERRORS ===============================================
______________________________ ERROR at teardown of test_teardown_bug _______________________________

    @pytest.fixture
    def example_fixture():
        yield
        print('Teardown started')
>       warnings.warn('Some warning')
E       UserWarning: Some warning

test_teardown_bug.py:10: UserWarning
====================================== short test summary info ======================================
ERROR test_teardown_bug.py::test_teardown_bug - UserWarning: Some warning
==================================== 1 passed, 1 error in 0.01s =====================================

Environment

$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.6 LTS
Release:	18.04
Codename:	bionic

$ python --version
Python 3.8.12

$ pytest --version
pytest 7.0.0

$ pip list
Package    Version
---------- -------
attrs      21.4.0
iniconfig  1.1.1
packaging  21.3
pip        22.0.3
pluggy     1.0.0
py         1.11.0
pyparsing  3.0.7
pytest     7.0.0
setuptools 56.0.0
tomli      2.0.1

[PiotrBatko-BlockchainWares]

Similar problem from the past. Maybe it will help to solve this issue:
#5729

[nicoddemus]

Hi @PiotrBatko-BlockchainWares,

Thanks for the report.

What happens when warnings are configured as errors, is that warning.warn itself raises an exception.

So warnings.warn('Some warning') in that context is the same as (for example) raise RuntimeError: it will stop the execution of the code in that function.

I'm not sure if pytest should make a special case of catching warnings as part of the teardown process (#5729 was a different problem, is it would break pytest entirely).

That's my two cents, interested on hearing what others think.

[RonnyPfannschmidt]

im under the impression that pytest should gain a own extra mode to report warnins as errors after a testrun

imho -W error is actually a test antipattern as it breaks test cleanup and code under test just to record the error later

i beleive we had a short discussion on that on twitter a few days back

[nicoddemus]

im under the impression that pytest should gain a own extra mode to report warnins as errors after a testrun

Not sure this will bring its own set of problems though... warnings are already a pretty complex topic as is, adding another layer of complexity seems at first glance it might make matters worse. But that's just an eagle's eye view opinion.

[RonnyPfannschmidt]

@nicoddemus bascially test runners should be able to error out on warnings without making the code that triggers warnings fall off the world all over the place

we already can go yellow on warnings, we should have a opt in to go into the reds

it needs a little design to make sense in integration

[Zac-HD]

I agree in general, but

imho -W error is actually a test antipattern as it breaks test cleanup and code under test just to record the error later

to me just says that you're not using enough try/finally or with ...: - both are fine cleanup approaches that work even in the face of unexpected exceptions.

So IMO we should treat "an Exception raised during a teardown task stops teardown" as a bug, and make that logic more resilient to unstable plugins and user code.

[RonnyPfannschmidt]

@Zac-HD IMHO that's a strawman, a freshly deprecated api in a new release of a dependency is most likely not going to be multi protected in any sensible codebase, yet warning errors will blow usages up needlessly in such cases

Additionally its going to be a utter pain in case one has to support a superset of versions

[asottile]

@RonnyPfannschmidt that's kinda a strawman too -- an api change which raises an exception would have the same problem

I don't think we should deviate from what python does here, and with proper context management (finally / with) this isn't really an issue

[RonnyPfannschmidt]

@asottile i stongly disagree, actual api bugs indeed need workarounds,
actual warnings that in normal usage don't break things shouldnt break things under test either

[RonnyPfannschmidt]

for me the equivalent is when in a car the oil warning lamp lights up vs the oil tank falls off

one doent handle those the same

the thing with -W error is that we don't get error indication safely deferred, it just blows up where/whenever

for a test runner i want the equivalent of the oil warning lamp, not the "blow off the tank YOLO"

[asottile]

then you don't want -W error -- that's saying "I don't have any warnings and I want the tank to blow off if it does"

[RonnyPfannschmidt]

@asottile the thing is - when the test runner doesnt allow safe warning errors asa feature, thats just the next best thing
however people use it like for a compiler when its in fact a test run

[asottile]

ok I don't think we're going to agree on that then. anyway my 2c is that pytest should not deviate from how python works here lest we have to explain a complex and difficult-to-understand (and surprisingly different) warning system that deviates from python. I think our recommendation should be that you be careful to write context-safe cleanup (use with / finally) when exceptions could be raised (which isn't really different from the status quo imo) -- and especially when converting warnings to errors