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 serialization of 'None' reprcrashes #6412

Merged
merged 2 commits into from Jan 7, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog/5971.bugfix.rst
@@ -0,0 +1,2 @@
Fix a ``pytest-xdist`` crash when dealing with exceptions raised in subprocesses created by the
``multiprocessing`` module.
14 changes: 10 additions & 4 deletions src/_pytest/reports.py
Expand Up @@ -374,8 +374,11 @@ def serialize_repr_traceback(reprtraceback):
]
return result

def serialize_repr_crash(reprcrash):
return reprcrash.__dict__.copy()
def serialize_repr_crash(reprcrash: Optional[ReprFileLocation]):
if reprcrash is not None:
return reprcrash.__dict__.copy()
else:
return None

def serialize_longrepr(rep):
result = {
Expand Down Expand Up @@ -455,8 +458,11 @@ def deserialize_repr_traceback(repr_traceback_dict):
]
return ReprTraceback(**repr_traceback_dict)

def deserialize_repr_crash(repr_crash_dict):
return ReprFileLocation(**repr_crash_dict)
def deserialize_repr_crash(repr_crash_dict: Optional[dict]):
if repr_crash_dict is not None:
return ReprFileLocation(**repr_crash_dict)
else:
return None

if (
reportdict["longrepr"]
Expand Down
56 changes: 56 additions & 0 deletions testing/test_reports.py
Expand Up @@ -305,13 +305,69 @@ def check_longrepr(longrepr):

data = report._to_json()
loaded_report = report_class._from_json(data)

assert loaded_report.failed
check_longrepr(loaded_report.longrepr)

# make sure we don't blow up on ``toterminal`` call; we don't test the actual output because it is very
# brittle and hard to maintain, but we can assume it is correct because ``toterminal`` is already tested
# elsewhere and we do check the contents of the longrepr object after loading it.
loaded_report.longrepr.toterminal(tw_mock)

def test_chained_exceptions_no_reprcrash(
self, testdir, tw_mock,
):
"""Regression test for tracebacks without a reprcrash (#5971)

This happens notably on exceptions raised by multiprocess.pool: the exception transfer
from subprocess to main process creates an artificial exception, which ExceptionInfo
can't obtain the ReprFileLocation from.
"""
testdir.makepyfile(
"""
from concurrent.futures import ProcessPoolExecutor
Copy link
Member Author

@nicoddemus nicoddemus Jan 7, 2020

Choose a reason for hiding this comment

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

Also this is easier to understand the original issue IMO


def func():
raise ValueError('value error')

def test_a():
with ProcessPoolExecutor() as p:
p.submit(func).result()
"""
)
reprec = testdir.inline_run()

reports = reprec.getreports("pytest_runtest_logreport")

def check_longrepr(longrepr):
assert isinstance(longrepr, ExceptionChainRepr)
assert len(longrepr.chain) == 2
entry1, entry2 = longrepr.chain
tb1, fileloc1, desc1 = entry1
tb2, fileloc2, desc2 = entry2

assert "RemoteTraceback" in str(tb1)
assert "ValueError: value error" in str(tb2)

assert fileloc1 is None
assert fileloc2.message == "ValueError: value error"

# 3 reports: setup/call/teardown: get the call report
assert len(reports) == 3
report = reports[1]

assert report.failed
check_longrepr(report.longrepr)

data = report._to_json()
loaded_report = TestReport._from_json(data)

assert loaded_report.failed
check_longrepr(loaded_report.longrepr)

# for same reasons as previous test, ensure we don't blow up here
loaded_report.longrepr.toterminal(tw_mock)


class TestHooks:
"""Test that the hooks are working correctly for plugins"""
Expand Down