From 717647086659674046e3dd94a570422f69ddccc4 Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Mon, 18 Dec 2023 11:28:24 -0500 Subject: [PATCH 1/7] Correctly report teardown fixture errors --- AUTHORS | 1 + changelog/11706.bugfix.rst | 1 + src/_pytest/runner.py | 3 +++ testing/test_runner.py | 22 ++++++++++++++++++++++ 4 files changed, 27 insertions(+) create mode 100644 changelog/11706.bugfix.rst diff --git a/AUTHORS b/AUTHORS index 14a35c3d5c3..67e794527fb 100644 --- a/AUTHORS +++ b/AUTHORS @@ -54,6 +54,7 @@ Aviral Verma Aviv Palivoda Babak Keyvani Barney Gale +Ben Brown Ben Gartner Ben Webb Benjamin Peterson diff --git a/changelog/11706.bugfix.rst b/changelog/11706.bugfix.rst new file mode 100644 index 00000000000..75970577cbe --- /dev/null +++ b/changelog/11706.bugfix.rst @@ -0,0 +1 @@ +Fix reporting of teardown errors in session and module scoped fixtures when `--maxfail=1`. diff --git a/src/_pytest/runner.py b/src/_pytest/runner.py index e20338520eb..6dd8e6c40a7 100644 --- a/src/_pytest/runner.py +++ b/src/_pytest/runner.py @@ -179,6 +179,9 @@ def pytest_runtest_call(item: Item) -> None: def pytest_runtest_teardown(item: Item, nextitem: Optional[Item]) -> None: _update_current_test_var(item, "teardown") + # If the session is about to fail, teardown everything - this is necessary + # to correctly report fixture teardown errors (see #11706) + nextitem = None if item.session.shouldfail else nextitem item.session._setupstate.teardown_exact(nextitem) _update_current_test_var(item, None) diff --git a/testing/test_runner.py b/testing/test_runner.py index c8b646857e7..d04822a3a11 100644 --- a/testing/test_runner.py +++ b/testing/test_runner.py @@ -1087,3 +1087,25 @@ def func() -> None: with pytest.raises(TypeError) as excinfo: OutcomeException(func) # type: ignore assert str(excinfo.value) == expected + + +def test_teardown_session_failed(pytester: Pytester) -> None: + """Test that fixture teardown failures are reported after a test fails.""" + pytester.makepyfile( + """ + import pytest + + @pytest.fixture(scope="module") + def baz(): + yield + pytest.fail("This is a failing fixture") + + def test_foo(baz): + pytest.fail("This is a failing test") + + def test_bar(baz): + pass + """ + ) + result = pytester.runpytest("--maxfail=1") + result.stdout.fnmatch_lines(["*1 failed, 1 error*"]) From f90dc391e792f90070f0c28ef528474e76ec47e0 Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Tue, 2 Jan 2024 11:02:46 -0500 Subject: [PATCH 2/7] PR feedback: Move changing of nextitem to runtestprotocol and also check for shouldstop --- src/_pytest/runner.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/_pytest/runner.py b/src/_pytest/runner.py index 6dd8e6c40a7..7ecc5bbebbf 100644 --- a/src/_pytest/runner.py +++ b/src/_pytest/runner.py @@ -131,6 +131,12 @@ def runtestprotocol( show_test_item(item) if not item.config.getoption("setuponly", False): reports.append(call_and_report(item, "call", log)) + + # If the session is about to fail or stop, teardown everything - this is + # necessary to correctly report fixture teardown errors (see #11706) + if item.session.shouldfail or item.session.shouldstop: + nextitem = None + reports.append(call_and_report(item, "teardown", log, nextitem=nextitem)) # After all teardown hooks have been called # want funcargs and request info to go away. @@ -179,9 +185,6 @@ def pytest_runtest_call(item: Item) -> None: def pytest_runtest_teardown(item: Item, nextitem: Optional[Item]) -> None: _update_current_test_var(item, "teardown") - # If the session is about to fail, teardown everything - this is necessary - # to correctly report fixture teardown errors (see #11706) - nextitem = None if item.session.shouldfail else nextitem item.session._setupstate.teardown_exact(nextitem) _update_current_test_var(item, None) From b81532e4432cba5f5ebbb46f2b9d6b51f394fe25 Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Tue, 2 Jan 2024 11:02:59 -0500 Subject: [PATCH 3/7] PR feedback: Improve new unit test --- testing/test_runner.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/testing/test_runner.py b/testing/test_runner.py index d04822a3a11..49654a84f1d 100644 --- a/testing/test_runner.py +++ b/testing/test_runner.py @@ -1090,7 +1090,11 @@ def func() -> None: def test_teardown_session_failed(pytester: Pytester) -> None: - """Test that fixture teardown failures are reported after a test fails.""" + """Test that higher-scoped fixture teardowns run in the context of the last + item after the test session bails early due to --maxfail. + + Regression test for #11706. + """ pytester.makepyfile( """ import pytest @@ -1108,4 +1112,4 @@ def test_bar(baz): """ ) result = pytester.runpytest("--maxfail=1") - result.stdout.fnmatch_lines(["*1 failed, 1 error*"]) + result.assert_outcomes(failed=1, errors=1) From c1adee097bfcc2e5e242a3bd6b4b8b255eef0367 Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Tue, 2 Jan 2024 11:09:26 -0500 Subject: [PATCH 4/7] Add new unit test for new shouldstop scenario --- testing/test_runner.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/testing/test_runner.py b/testing/test_runner.py index 49654a84f1d..9e84b8cda02 100644 --- a/testing/test_runner.py +++ b/testing/test_runner.py @@ -1113,3 +1113,29 @@ def test_bar(baz): ) result = pytester.runpytest("--maxfail=1") result.assert_outcomes(failed=1, errors=1) + + +def test_teardown_session_stopped(pytester: Pytester) -> None: + """Similar to `test_teardown_session_failed`, but uses `pytest.exit` instead + of `pytest.fail`. + + Regression test for #11706. + """ + pytester.makepyfile( + """ + import pytest + + @pytest.fixture(scope="module") + def baz(): + yield + pytest.exit("This is an exiting fixture") + + def test_foo(baz): + pytest.fail("This is a failing test") + + def test_bar(baz): + pass + """ + ) + result = pytester.runpytest("--maxfail=1") + result.assert_outcomes(failed=1, errors=0) From 75e9f3b19d8a4dc5c2facb681592e48322a2e519 Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Tue, 2 Jan 2024 11:27:03 -0500 Subject: [PATCH 5/7] PR feedback: Make shouldfail and shouldstop properties, prevent True -> False transition --- src/_pytest/main.py | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/_pytest/main.py b/src/_pytest/main.py index 9fb96840e5c..d9ecd39748c 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -548,8 +548,8 @@ def __init__(self, config: Config) -> None: ) self.testsfailed = 0 self.testscollected = 0 - self.shouldstop: Union[bool, str] = False - self.shouldfail: Union[bool, str] = False + self._shouldstop: Union[bool, str] = False + self._shouldfail: Union[bool, str] = False self.trace = config.trace.root.get("collection") self._initialpaths: FrozenSet[Path] = frozenset() self._initialpaths_with_parents: FrozenSet[Path] = frozenset() @@ -576,6 +576,28 @@ def __repr__(self) -> str: self.testscollected, ) + @property + def shouldstop(self) -> Union[bool, str]: + return self._shouldstop + + @shouldstop.setter + def shouldstop(self, value: Union[bool, str]) -> None: + """Prevent plugins from setting this to False once it has been set.""" + if value is False and self._shouldstop: + return + self._shouldstop = value + + @property + def shouldfail(self) -> Union[bool, str]: + return self._shouldfail + + @shouldfail.setter + def shouldfail(self, value: Union[bool, str]) -> None: + """Prevent plugins from setting this to False once it has been set.""" + if value is False and self._shouldfail: + return + self._shouldfail = value + @property def startpath(self) -> Path: """The path from which pytest was invoked. From 6bb20d9ead0a2c568f9a7a7b1d68086fa8d4430c Mon Sep 17 00:00:00 2001 From: Ben Brown Date: Tue, 2 Jan 2024 13:41:51 -0500 Subject: [PATCH 6/7] PR feedback: Raise warnings in the new properties for shouldstop and shouldfail, add unit test to test_session.py --- src/_pytest/main.py | 14 ++++++++++++++ src/_pytest/runner.py | 2 -- testing/test_session.py | 24 ++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/_pytest/main.py b/src/_pytest/main.py index d9ecd39748c..e6256fae73b 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -6,6 +6,7 @@ import importlib import os import sys +import warnings from pathlib import Path from typing import AbstractSet from typing import Callable @@ -44,6 +45,7 @@ from _pytest.reports import TestReport from _pytest.runner import collect_one_node from _pytest.runner import SetupState +from _pytest.warning_types import PytestWarning def pytest_addoption(parser: Parser) -> None: @@ -584,6 +586,12 @@ def shouldstop(self) -> Union[bool, str]: def shouldstop(self, value: Union[bool, str]) -> None: """Prevent plugins from setting this to False once it has been set.""" if value is False and self._shouldstop: + warnings.warn( + PytestWarning( + "session.shouldstop cannot be unset after it has been set; ignoring." + ), + stacklevel=2, + ) return self._shouldstop = value @@ -595,6 +603,12 @@ def shouldfail(self) -> Union[bool, str]: def shouldfail(self, value: Union[bool, str]) -> None: """Prevent plugins from setting this to False once it has been set.""" if value is False and self._shouldfail: + warnings.warn( + PytestWarning( + "session.shouldfail cannot be unset after it has been set; ignoring." + ), + stacklevel=2, + ) return self._shouldfail = value diff --git a/src/_pytest/runner.py b/src/_pytest/runner.py index 7ecc5bbebbf..3e19f0de50c 100644 --- a/src/_pytest/runner.py +++ b/src/_pytest/runner.py @@ -131,12 +131,10 @@ def runtestprotocol( show_test_item(item) if not item.config.getoption("setuponly", False): reports.append(call_and_report(item, "call", log)) - # If the session is about to fail or stop, teardown everything - this is # necessary to correctly report fixture teardown errors (see #11706) if item.session.shouldfail or item.session.shouldstop: nextitem = None - reports.append(call_and_report(item, "teardown", log, nextitem=nextitem)) # After all teardown hooks have been called # want funcargs and request info to go away. diff --git a/testing/test_session.py b/testing/test_session.py index 136e85eb640..1ac39f2ef13 100644 --- a/testing/test_session.py +++ b/testing/test_session.py @@ -418,3 +418,27 @@ def test_rootdir_wrong_option_arg(pytester: Pytester) -> None: result.stderr.fnmatch_lines( ["*Directory *wrong_dir* not found. Check your '--rootdir' option.*"] ) + + +def test_shouldfail_is_sticky(pytester: Pytester) -> None: + """Test that session.shouldfail cannot be reset to False after being set.""" + pytester.makeconftest( + """ + def pytest_sessionfinish(session, exitstatus): + session.shouldfail = False + """ + ) + + pytester.makepyfile( + """ + import pytest + + def test_foo(): + pytest.fail("This is a failing test") + + def test_bar(): + pass + """ + ) + result = pytester.runpytest("--maxfail=1") + result.stderr.fnmatch_lines("*session.shouldfail cannot be unset*") From b559724a82e0572adccddc0fe6151ee019926bba Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Wed, 3 Jan 2024 18:28:26 +0200 Subject: [PATCH 7/7] Some tweaks --- changelog/11706.bugfix.rst | 2 +- src/_pytest/main.py | 6 +++-- testing/test_runner.py | 22 ++++++++-------- testing/test_session.py | 52 ++++++++++++++++++++++++++++++++------ 4 files changed, 59 insertions(+), 23 deletions(-) diff --git a/changelog/11706.bugfix.rst b/changelog/11706.bugfix.rst index 75970577cbe..1b90d8f0b60 100644 --- a/changelog/11706.bugfix.rst +++ b/changelog/11706.bugfix.rst @@ -1 +1 @@ -Fix reporting of teardown errors in session and module scoped fixtures when `--maxfail=1`. +Fix reporting of teardown errors in higher-scoped fixtures when using `--maxfail` or `--stepwise`. diff --git a/src/_pytest/main.py b/src/_pytest/main.py index e6256fae73b..51be84164b2 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -584,7 +584,8 @@ def shouldstop(self) -> Union[bool, str]: @shouldstop.setter def shouldstop(self, value: Union[bool, str]) -> None: - """Prevent plugins from setting this to False once it has been set.""" + # The runner checks shouldfail and assumes that if it is set we are + # definitely stopping, so prevent unsetting it. if value is False and self._shouldstop: warnings.warn( PytestWarning( @@ -601,7 +602,8 @@ def shouldfail(self) -> Union[bool, str]: @shouldfail.setter def shouldfail(self, value: Union[bool, str]) -> None: - """Prevent plugins from setting this to False once it has been set.""" + # The runner checks shouldfail and assumes that if it is set we are + # definitely stopping, so prevent unsetting it. if value is False and self._shouldfail: warnings.warn( PytestWarning( diff --git a/testing/test_runner.py b/testing/test_runner.py index 9e84b8cda02..26f5b9a0bc2 100644 --- a/testing/test_runner.py +++ b/testing/test_runner.py @@ -1102,22 +1102,21 @@ def test_teardown_session_failed(pytester: Pytester) -> None: @pytest.fixture(scope="module") def baz(): yield - pytest.fail("This is a failing fixture") + pytest.fail("This is a failing teardown") def test_foo(baz): pytest.fail("This is a failing test") - def test_bar(baz): - pass - """ + def test_bar(): pass + """ ) result = pytester.runpytest("--maxfail=1") result.assert_outcomes(failed=1, errors=1) def test_teardown_session_stopped(pytester: Pytester) -> None: - """Similar to `test_teardown_session_failed`, but uses `pytest.exit` instead - of `pytest.fail`. + """Test that higher-scoped fixture teardowns run in the context of the last + item after the test session bails early due to --stepwise. Regression test for #11706. """ @@ -1128,14 +1127,13 @@ def test_teardown_session_stopped(pytester: Pytester) -> None: @pytest.fixture(scope="module") def baz(): yield - pytest.exit("This is an exiting fixture") + pytest.fail("This is a failing teardown") def test_foo(baz): pytest.fail("This is a failing test") - def test_bar(baz): - pass - """ + def test_bar(): pass + """ ) - result = pytester.runpytest("--maxfail=1") - result.assert_outcomes(failed=1, errors=0) + result = pytester.runpytest("--stepwise") + result.assert_outcomes(failed=1, errors=1) diff --git a/testing/test_session.py b/testing/test_session.py index 1ac39f2ef13..803bbed5434 100644 --- a/testing/test_session.py +++ b/testing/test_session.py @@ -421,14 +421,18 @@ def test_rootdir_wrong_option_arg(pytester: Pytester) -> None: def test_shouldfail_is_sticky(pytester: Pytester) -> None: - """Test that session.shouldfail cannot be reset to False after being set.""" + """Test that session.shouldfail cannot be reset to False after being set. + + Issue #11706. + """ pytester.makeconftest( """ - def pytest_sessionfinish(session, exitstatus): + def pytest_sessionfinish(session): + assert session.shouldfail session.shouldfail = False - """ + assert session.shouldfail + """ ) - pytester.makepyfile( """ import pytest @@ -436,9 +440,41 @@ def pytest_sessionfinish(session, exitstatus): def test_foo(): pytest.fail("This is a failing test") - def test_bar(): - pass + def test_bar(): pass + """ + ) + + result = pytester.runpytest("--maxfail=1", "-Wall") + + result.assert_outcomes(failed=1, warnings=1) + result.stdout.fnmatch_lines("*session.shouldfail cannot be unset*") + + +def test_shouldstop_is_sticky(pytester: Pytester) -> None: + """Test that session.shouldstop cannot be reset to False after being set. + + Issue #11706. """ + pytester.makeconftest( + """ + def pytest_sessionfinish(session): + assert session.shouldstop + session.shouldstop = False + assert session.shouldstop + """ ) - result = pytester.runpytest("--maxfail=1") - result.stderr.fnmatch_lines("*session.shouldfail cannot be unset*") + pytester.makepyfile( + """ + import pytest + + def test_foo(): + pytest.fail("This is a failing test") + + def test_bar(): pass + """ + ) + + result = pytester.runpytest("--stepwise", "-Wall") + + result.assert_outcomes(failed=1, warnings=1) + result.stdout.fnmatch_lines("*session.shouldstop cannot be unset*")