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

Fixed #32114 -- Subtest pickle issue with ParallelTestRunner #13599

Closed
wants to merge 5 commits into from
Closed
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
38 changes: 38 additions & 0 deletions django/test/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,43 @@ def debug(self, error):
pdb.post_mortem(traceback)


class TestCaseDTO:
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

What does DTO stand for? I think it'd be better to use a spelled out name for this class

Copy link
Author

Choose a reason for hiding this comment

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

"""
The ParallelTestSuite requires that all test cases be pickleable. When a
SubTest fails with an exception, it's pickled and shipped back to the main
process to be displayed to the user. Unfortunately, Django's TestCases
aren't always pickleable (for example, they may contain an instance of
django.test.Client, which can't be cleanly pickled if an exception was
raised by the view).
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

If TestCases aren't pikcleable, wouldn't that affect normal tests, and not just subtests too?

Copy link
Author

Choose a reason for hiding this comment

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

The RemoteTestResult.events list is pickled on its way back from django.test.runner._run_subsuite. '

When dealing with normal tests, the data in events just consists of test indices and possibly errors, and the main process uses those indices to retrieve the tests.

However, when a subtest is encountered,django.test.runner.RemoteTestResult.addSubTest puts the actual subtest instance into the events list. This is because unittest.TestResult.addSubTest expects to receive the subtest instance.


Luckily the outer TestRunner only cares about a small subset of the fields
on TestCase, and those are usually pickleable. We use this object to pluck
out those fields, discarding extra unpickleable members.
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This docstring spends a lot of time explainining the problem. I think it can be edited down to something a little more terse to make the class more maintainable. Perhaps:

Represents a failed test or SubTest for use in parallel testing. Needed as not all attributes of a TestCase may be pickleable, so this class extracts only those needed to report the error in the main process.

"""

def __init__(self, test):
self._inner_id = test.id()
self._inner_str = str(test)
self._inner_shortDescription = test.shortDescription()
if hasattr(test, "test_case"):
self.test_case = TestCaseDTO(test.test_case)
if hasattr(test, "_subDescription"):
self._inner_subDescription = test._subDescription()

def _subDescription(self):
"""conforming to _SubTest"""
return self._inner_subDescription

def id(self):
return self._inner_id

def shortDescription(self):
return self._inner_shortDescription

def __str__(self):
return self._inner_str


class RemoteTestResult:
"""
Record information about which tests have succeeded and which have failed.
Expand Down Expand Up @@ -237,6 +274,7 @@ def addSubTest(self, test, subtest, err):
# Follow Python 3.5's implementation of unittest.TestResult.addSubTest()
# by not doing anything when a subtest is successful.
if err is not None:
subtest = TestCaseDTO(subtest)
# Call check_picklable() before check_subtest_picklable() since
# check_picklable() performs the tblib check.
self.check_picklable(test, err)
Expand Down
60 changes: 59 additions & 1 deletion tests/test_runner/test_parallel.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import unittest

from django.test import SimpleTestCase
from django.test.runner import RemoteTestResult
from django.test.runner import RemoteTestResult, TestCaseDTO
from django.utils.version import PY37

try:
Expand Down Expand Up @@ -49,6 +49,15 @@ def dummy_test(self):
with self.subTest(index=i):
self.assertEqual(i, 1)

def pickle_error_test(self):
"""A dummy test for testing TestCaseDTO."""
# ^ note: 3.6's implementation of TestCase.shortDescription()
# doesn't strip whitespace before taking the first line of the
# docstring, so the single-line docstring is part of this test
with self.subTest("I should fail"):
self.not_pickleable = lambda: 0
self.assertTrue(False)


class RemoteTestResultTest(SimpleTestCase):

Expand Down Expand Up @@ -85,3 +94,52 @@ def test_add_failing_subtests(self):

event = events[2]
self.assertEqual(repr(event[3][1]), "AssertionError('2 != 1'%s)" % trailing_comma)

@unittest.skipUnless(tblib is not None, 'requires tblib to be installed')
def test_add_failed_subtest_with_unpickleable_members(self):
"""
When a SubTest fails in a parallel context and the TestCase contains
some unpickleable members, we should still be able to send the SubTest
back to the TestRunner.
"""
result = RemoteTestResult()
subtest_test = SampleFailingSubtest(methodName="pickle_error_test")

# This is the meat of the test: without TestCaseDTO, we'd expect an
# ugly "Can't pickle local object" exception when trying to run.
result = subtest_test.run(result=result)

add_subtest_event = result.events[1]
error = add_subtest_event[3][1]
self.assertEqual(str(error), "False is not true")


class TestCaseDTOTest(SimpleTestCase):
@unittest.skipUnless(tblib is not None, 'requires tblib to be installed')
def test_fields_are_set(self):
"""
TestCaseDTO picks up the right fields from the wrapped TestCase.
"""

result = RemoteTestResult()
subtest_test = SampleFailingSubtest(methodName="pickle_error_test")
result = subtest_test.run(result=result)

add_subtest_event = result.events[1]
subtest = add_subtest_event[2]

self.assertIsInstance(subtest, TestCaseDTO)
self.assertEqual(subtest.shortDescription(), "A dummy test for testing TestCaseDTO.")
self.assertEqual(subtest._subDescription(), "[I should fail]")
self.assertEqual(
subtest.id(),
"test_runner.test_parallel.SampleFailingSubtest.pickle_error_test [I should fail]"
)
self.assertEqual(
str(subtest),
"pickle_error_test (test_runner.test_parallel.SampleFailingSubtest) [I should fail]"
)
self.assertEqual(
subtest.test_case.id(),
'test_runner.test_parallel.SampleFailingSubtest.pickle_error_test'
)