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

Fix teardown error reporting when --maxfail=1 #11721

Merged
merged 7 commits into from
Jan 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Aviral Verma
Aviv Palivoda
Babak Keyvani
Barney Gale
Ben Brown
Ben Gartner
Ben Webb
Benjamin Peterson
Expand Down
1 change: 1 addition & 0 deletions changelog/11706.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix reporting of teardown errors in higher-scoped fixtures when using `--maxfail` or `--stepwise`.
42 changes: 40 additions & 2 deletions src/_pytest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import importlib
import os
import sys
import warnings
from pathlib import Path
from typing import AbstractSet
from typing import Callable
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -548,8 +550,8 @@
)
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()
Expand All @@ -576,6 +578,42 @@
self.testscollected,
)

@property
def shouldstop(self) -> Union[bool, str]:
return self._shouldstop

@shouldstop.setter
def shouldstop(self, value: Union[bool, str]) -> None:
# 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(

Check warning on line 590 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L590

Added line #L590 was not covered by tests
PytestWarning(
"session.shouldstop cannot be unset after it has been set; ignoring."
),
stacklevel=2,
)
return
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluetech should an exception be raised here? Or is simply ignoring the operation okay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think silently ignoring is no good, it will be confusing why there is no effect.

The two options are issuing a warning, or raising an exception. Since we're already stopping raising an exception seems kinda pointless, so I think we should go with a warning. I think it can be something like this:

# The runner checks shouldfail and assumes that if it is set we are definitely stopping,
# so prevent unsetting it.
warnings.warn(PytestWarning("session.shouldfail cannot be unset after it has been set; ignoring."), stacklevel=2)

Unfortunately it does mean you'll need to write extra tests to cover these warnings, let me know if you need help with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sounds good, I've added the warnings and and a unit test for the shouldfail scenario called test_shouldfail_is_sticky. I'm using pytest_sessionfinish to trigger the bad behavior. However this same approach did not work for shouldstop - it seems like shouldstop was still False in pytest_sessionfinish even if I changed pytest.fail to pytest.exit. Any ideas on how to test the shouldstop scenario?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing which sets shouldstop in pytest core is --stepwise, which actually has the same problem as --maxfail. Since I've already checked out the branch, I've pushed the new test along with some tweaks. This is now good to go, thanks!

self._shouldstop = value

Check warning on line 597 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L596-L597

Added lines #L596 - L597 were not covered by tests

@property
def shouldfail(self) -> Union[bool, str]:
return self._shouldfail

@shouldfail.setter
def shouldfail(self, value: Union[bool, str]) -> None:
# 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(

Check warning on line 608 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L608

Added line #L608 was not covered by tests
PytestWarning(
"session.shouldfail cannot be unset after it has been set; ignoring."
),
stacklevel=2,
)
return
self._shouldfail = value

Check warning on line 615 in src/_pytest/main.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/main.py#L614-L615

Added lines #L614 - L615 were not covered by tests

@property
def startpath(self) -> Path:
"""The path from which pytest was invoked.
Expand Down
4 changes: 4 additions & 0 deletions src/_pytest/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@
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

Check warning on line 137 in src/_pytest/runner.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/runner.py#L137

Added line #L137 was not covered by tests
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.
Expand Down
50 changes: 50 additions & 0 deletions testing/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -1087,3 +1087,53 @@ 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 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

@pytest.fixture(scope="module")
def baz():
yield
pytest.fail("This is a failing teardown")

def test_foo(baz):
pytest.fail("This is a failing test")

def test_bar(): pass
"""
)
result = pytester.runpytest("--maxfail=1")
result.assert_outcomes(failed=1, errors=1)


def test_teardown_session_stopped(pytester: Pytester) -> None:
"""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.
"""
pytester.makepyfile(
"""
import pytest

@pytest.fixture(scope="module")
def baz():
yield
pytest.fail("This is a failing teardown")

def test_foo(baz):
pytest.fail("This is a failing test")

def test_bar(): pass
"""
)
result = pytester.runpytest("--stepwise")
result.assert_outcomes(failed=1, errors=1)
60 changes: 60 additions & 0 deletions testing/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -418,3 +418,63 @@ 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.

Issue #11706.
"""
pytester.makeconftest(
"""
def pytest_sessionfinish(session):
assert session.shouldfail
session.shouldfail = False
assert session.shouldfail
"""
)
pytester.makepyfile(
"""
import pytest

def test_foo():
pytest.fail("This is a failing test")

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
"""
)
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*")
Loading