Skip to content

fixtures,runner: fix tracebacks getting longer and longer #12264

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

Merged
merged 2 commits into from
Apr 29, 2024
Merged
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
7 changes: 7 additions & 0 deletions changelog/12204.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Fix a regression in pytest 8.0 where tracebacks get longer and longer when multiple tests fail due to a shared higher-scope fixture which raised.

Also fix a similar regression in pytest 5.4 for collectors which raise during setup.

The fix necessitated internal changes which may affect some plugins:
- ``FixtureDef.cached_result[2]`` is now a tuple ``(exc, tb)`` instead of ``exc``.
- ``SetupState.stack`` failures are now a tuple ``(exc, tb)`` instead of ``exc``.
Comment on lines +6 to +7
Copy link
Member

Choose a reason for hiding this comment

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

@bluetech FYI this list seems to need indentation or something like that. See the preview of how it's rendered: https://docs.pytest.org/en/latest/changelog.html#bug-fixes.

P.S. I integrated my changelog draft preview Sphinx extension during the sprint. So now, you're going to be able to preview the rendering in new pull requests, even before merging.

Copy link
Member

@webknjaz webknjaz Jul 2, 2024

Choose a reason for hiding this comment

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

I submitted a fix via #12563. It's better with that patch: https://pytest--12563.org.readthedocs.build/en/12563/changelog.html#bug-fixes.

11 changes: 6 additions & 5 deletions src/_pytest/fixtures.py
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@
import os
from pathlib import Path
import sys
import types
from typing import AbstractSet
from typing import Any
from typing import Callable
@@ -104,8 +105,8 @@
None,
# Cache key.
object,
# Exception if raised.
BaseException,
# The exception and the original traceback.
Tuple[BaseException, Optional[types.TracebackType]],
],
]

@@ -1049,8 +1050,8 @@ def execute(self, request: SubRequest) -> FixtureValue:
# numpy arrays (#6497).
if my_cache_key is cache_key:
if self.cached_result[2] is not None:
exc = self.cached_result[2]
raise exc
exc, exc_tb = self.cached_result[2]
raise exc.with_traceback(exc_tb)
else:
result = self.cached_result[0]
return result
@@ -1126,7 +1127,7 @@ def pytest_fixture_setup(
# Don't show the fixture as the skip location, as then the user
# wouldn't know which test skipped.
e._use_item_location = True
fixturedef.cached_result = (None, my_cache_key, e)
fixturedef.cached_result = (None, my_cache_key, (e, e.__traceback__))
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to just pass the dunder traceback into the method to avoid tracking the concrete one for reraise?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, the exception is saved on the FixtureDef and can be needed again at an arbitrary point in the future without a caller-callee relation to the time it is saved.

raise
fixturedef.cached_result = (result, my_cache_key, None)
return result
14 changes: 10 additions & 4 deletions src/_pytest/runner.py
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@
import dataclasses
import os
import sys
import types
from typing import Callable
from typing import cast
from typing import Dict
@@ -488,8 +489,13 @@ def __init__(self) -> None:
Tuple[
# Node's finalizers.
List[Callable[[], object]],
# Node's exception, if its setup raised.
Optional[Union[OutcomeException, Exception]],
# Node's exception and original traceback, if its setup raised.
Optional[
Tuple[
Union[OutcomeException, Exception],
Optional[types.TracebackType],
]
],
],
] = {}

@@ -502,7 +508,7 @@ def setup(self, item: Item) -> None:
for col, (finalizers, exc) in self.stack.items():
assert col in needed_collectors, "previous item was not torn down properly"
if exc:
raise exc
raise exc[0].with_traceback(exc[1])

for col in needed_collectors[len(self.stack) :]:
assert col not in self.stack
@@ -511,7 +517,7 @@ def setup(self, item: Item) -> None:
try:
col.setup()
except TEST_OUTCOME as exc:
self.stack[col] = (self.stack[col][0], exc)
self.stack[col] = (self.stack[col][0], (exc, exc.__traceback__))
raise

def addfinalizer(self, finalizer: Callable[[], object], node: Node) -> None:
22 changes: 22 additions & 0 deletions testing/python/fixtures.py
Original file line number Diff line number Diff line change
@@ -3397,6 +3397,28 @@ def test_something():
["*def gen(qwe123):*", "*fixture*qwe123*not found*", "*1 error*"]
)

def test_cached_exception_doesnt_get_longer(self, pytester: Pytester) -> None:
"""Regression test for #12204."""
pytester.makepyfile(
"""
import pytest
@pytest.fixture(scope="session")
def bad(): 1 / 0

def test_1(bad): pass
def test_2(bad): pass
def test_3(bad): pass
"""
)

result = pytester.runpytest_inprocess("--tb=native")
assert result.ret == ExitCode.TESTS_FAILED
failures = result.reprec.getfailures() # type: ignore[attr-defined]
assert len(failures) == 3
lines1 = failures[1].longrepr.reprtraceback.reprentries[0].lines
lines2 = failures[2].longrepr.reprtraceback.reprentries[0].lines
assert len(lines1) == len(lines2)


class TestShowFixtures:
def test_funcarg_compat(self, pytester: Pytester) -> None:
37 changes: 37 additions & 0 deletions testing/test_runner.py
Original file line number Diff line number Diff line change
@@ -142,6 +142,43 @@ def raiser(exc):
assert isinstance(func.exceptions[0], TypeError) # type: ignore
assert isinstance(func.exceptions[1], ValueError) # type: ignore

def test_cached_exception_doesnt_get_longer(self, pytester: Pytester) -> None:
"""Regression test for #12204 (the "BTW" case)."""
pytester.makepyfile(test="")
# If the collector.setup() raises, all collected items error with this
# exception.
pytester.makeconftest(
"""
import pytest

class MyItem(pytest.Item):
def runtest(self) -> None: pass

class MyBadCollector(pytest.Collector):
def collect(self):
return [
MyItem.from_parent(self, name="one"),
MyItem.from_parent(self, name="two"),
MyItem.from_parent(self, name="three"),
]

def setup(self):
1 / 0

def pytest_collect_file(file_path, parent):
if file_path.name == "test.py":
return MyBadCollector.from_parent(parent, name='bad')
"""
)

result = pytester.runpytest_inprocess("--tb=native")
assert result.ret == ExitCode.TESTS_FAILED
failures = result.reprec.getfailures() # type: ignore[attr-defined]
assert len(failures) == 3
lines1 = failures[1].longrepr.reprtraceback.reprentries[0].lines
lines2 = failures[2].longrepr.reprtraceback.reprentries[0].lines
assert len(lines1) == len(lines2)


class BaseFunctionalTests:
def test_passfunction(self, pytester: Pytester) -> None: