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

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

Merged
merged 2 commits into from
Apr 29, 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
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
Expand Up @@ -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
Expand Down Expand Up @@ -104,8 +105,8 @@
None,
# Cache key.
object,
# Exception if raised.
BaseException,
# The exception and the original traceback.
Tuple[BaseException, Optional[types.TracebackType]],
],
]

Expand Down Expand Up @@ -1049,8 +1050,8 @@
# 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)

Check warning on line 1054 in src/_pytest/fixtures.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/fixtures.py#L1053-L1054

Added lines #L1053 - L1054 were not covered by tests
else:
result = self.cached_result[0]
return result
Expand Down Expand Up @@ -1126,7 +1127,7 @@
# 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__))

Check warning on line 1130 in src/_pytest/fixtures.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/fixtures.py#L1130

Added line #L1130 was not covered by tests
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
Expand Down
14 changes: 10 additions & 4 deletions src/_pytest/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import dataclasses
import os
import sys
import types
from typing import Callable
from typing import cast
from typing import Dict
Expand Down Expand Up @@ -488,8 +489,13 @@
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],
]
],
],
] = {}

Expand All @@ -502,7 +508,7 @@
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])

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

View check run for this annotation

Codecov / codecov/patch

src/_pytest/runner.py#L511

Added line #L511 was not covered by tests

for col in needed_collectors[len(self.stack) :]:
assert col not in self.stack
Expand All @@ -511,7 +517,7 @@
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__))

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

View check run for this annotation

Codecov / codecov/patch

src/_pytest/runner.py#L520

Added line #L520 was not covered by tests
raise

def addfinalizer(self, finalizer: Callable[[], object], node: Node) -> None:
Expand Down
22 changes: 22 additions & 0 deletions testing/python/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
37 changes: 37 additions & 0 deletions testing/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Loading