From 0e0006934019aba7a3bbec95e2fac7c1b82d1063 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 6 Jan 2020 21:16:23 -0300 Subject: [PATCH 1/2] Fix serialization of 'None' reprcrashes Tracebacks coming from remote processes crated by the multiprocess module will contain "RemoteTracebacks" which don't have a 'reprcrash' attribute Fix #5971 --- changelog/5971.bugfix.rst | 2 ++ src/_pytest/reports.py | 14 ++++++--- testing/test_reports.py | 62 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 changelog/5971.bugfix.rst diff --git a/changelog/5971.bugfix.rst b/changelog/5971.bugfix.rst new file mode 100644 index 000000000..dbc79dd96 --- /dev/null +++ b/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. diff --git a/src/_pytest/reports.py b/src/_pytest/reports.py index 5d445c2f8..4c93013d6 100644 --- a/src/_pytest/reports.py +++ b/src/_pytest/reports.py @@ -374,8 +374,11 @@ def _report_to_json(report): ] 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 = { @@ -455,8 +458,11 @@ def _report_kwargs_from_json(reportdict): ] 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"] diff --git a/testing/test_reports.py b/testing/test_reports.py index ff813543c..f695965e9 100644 --- a/testing/test_reports.py +++ b/testing/test_reports.py @@ -305,6 +305,8 @@ class TestReportSerialization: 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 @@ -312,6 +314,66 @@ class TestReportSerialization: # 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( + """ + # equivalent of multiprocessing.pool.RemoteTraceback + class RemoteTraceback(Exception): + def __init__(self, tb): + self.tb = tb + def __str__(self): + return self.tb + + def test_a(): + try: + raise ValueError('value error') + except ValueError as e: + # equivalent to how multiprocessing.pool.rebuild_exc does it + e.__cause__ = RemoteTraceback('runtime error') + raise e + """ + ) + 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: runtime error" 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""" From 356d865ad71be56b083c8f1ac8065f6cbec056b8 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 7 Jan 2020 12:45:18 -0300 Subject: [PATCH 2/2] Use concurrent.futures for fidelity to the original report As requested in review --- testing/test_reports.py | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/testing/test_reports.py b/testing/test_reports.py index f695965e9..d0bafec23 100644 --- a/testing/test_reports.py +++ b/testing/test_reports.py @@ -320,25 +320,19 @@ class TestReportSerialization: """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 + from subprocess to main process creates an artificial exception, which ExceptionInfo can't obtain the ReprFileLocation from. """ testdir.makepyfile( """ - # equivalent of multiprocessing.pool.RemoteTraceback - class RemoteTraceback(Exception): - def __init__(self, tb): - self.tb = tb - def __str__(self): - return self.tb + from concurrent.futures import ProcessPoolExecutor + + def func(): + raise ValueError('value error') def test_a(): - try: - raise ValueError('value error') - except ValueError as e: - # equivalent to how multiprocessing.pool.rebuild_exc does it - e.__cause__ = RemoteTraceback('runtime error') - raise e + with ProcessPoolExecutor() as p: + p.submit(func).result() """ ) reprec = testdir.inline_run() @@ -352,8 +346,8 @@ class TestReportSerialization: tb1, fileloc1, desc1 = entry1 tb2, fileloc2, desc2 = entry2 - assert "RemoteTraceback: runtime error" in str(tb1) - assert "ValueError('value error')" in str(tb2) + assert "RemoteTraceback" in str(tb1) + assert "ValueError: value error" in str(tb2) assert fileloc1 is None assert fileloc2.message == "ValueError: value error"