Context Navigation

← Previous Ticket
Next Ticket →

#32114 closed Cleanup/optimization (fixed)

Workaround for subtest issue with parallel test runner

Reported by:	Jordan Ephron	Owned by:	David Wobrock
Component:	Testing framework	Version:	3.1
Severity:	Normal	Keywords:	Test subtest parallel
Cc:	Adam Johnson, Sarah Boyce, Sage Abdullah, David Wobrock	Triage Stage:	Ready for checkin
Has patch:	yes	Needs documentation:	no
Needs tests:	no	Patch needs improvement:	no
Easy pickings:	no	UI/UX:	no

Description (last modified by Jordan Ephron)

Related to https://code.djangoproject.com/ticket/26942

Django's 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. Unfortunately,
Django's TestCases aren't always pickleable (for instance,
they may contain an instance of django.test.Client which can't
be cleanly pickled if an exception was raised by a view)

When the following test case is run in a parallel environment,
the test runner is unable to serialize the exception:

class SubtestBugTestCase(TestCase):
    def test_subtest_bug(self):
        with self.subTest("I am the subtest"):
            self.not_pickleable = lambda: 0
            self.assertTrue(False)

# AttributeError: Can't pickle local object 'SubtestBugTestCase.test_subtest_bug.<locals>.<lambda>'

Luckily, Django's DiscoverRunner only actually cares about a
small subset of the fields on TestCase, and those fields are
all of pickleable types. (This is also true of the PyCharm
test runner, which we use)

We work around the subtest issue by wrapping up the subtest
as follows:

class TestCaseDTO:
    def __init__(self, test):
        self._m_id = test.id()
        self._m_str = str(test)
        self._m_shortDescription = test.shortDescription()
        if hasattr(test, "test_case"):
            self.test_case = TestCaseDTO(test.test_case)
        if hasattr(test, "_subDescription"):
            self._m_subDescription = test._subDescription()

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

    def id(self):
        return self._m_id

    def shortDescription(self):
        return self._m_shortDescription

    def __str__(self):
        return self._m_str


class SubtestSerializingRemoteTestResult(RemoteTestResult):
    def addSubTest(self, test, subtest, err):
        subtest = TestCaseDTO(subtest)
        super().addSubTest(test, subtest, err)


class OurRemoteTestRunner(RemoteTestRunner):
    resultclass = SubtestSerializingRemoteTestResult


class OurTestSuite(ParallelTestSuite):
    runner_class = OurRemoteTestRunner


class OurDiscoverRunner(DiscoverRunner):
    parallel_test_suite = OurTestSuite

Now, to be fair, it's possible that some test runners do care
about fields that aren't captured by TestCaseDTO, so I'm
not sure whether this is truly a universal solution, but this
issue was a significant impediment to parallelizing our tests.

Would it be worthwhile to have this behavior in Django core?

Change History (27)

comment:1 by Jordan Ephron, 4 years ago

Description:	modified (diff)

comment:2 by Jordan Ephron, 4 years ago

Component:	Uncategorized → Testing framework

comment:3 by Carlton Gibson, 4 years ago

Triage Stage:	Unreviewed → Accepted

HI Jordan. Thanks for the report.

Sounds like it's definitely worth looking at a patch. It would be a nice improvement if there aren't any issues.
Would you be able to prepare a PR?

Ref also #27301 (and loosely #29023)

comment:4 by Jordan Ephron, 4 years ago

Owner:	changed from nobody to Jordan Ephron
Status:	new → assigned

Patch en route

comment:5 by Jordan Ephron, 4 years ago

Has patch:	set

https://github.com/django/django/pull/13599

comment:6 by Adam Johnson, 4 years ago

Cc:	Adam Johnson added

comment:7 by Mariusz Felisiak, 3 years ago

Type:	Uncategorized → Cleanup/optimization

comment:8 by Mariusz Felisiak, 3 years ago

Patch needs improvement:	set

comment:9 by Chris Jerdonek, 3 years ago

I took a look at the proposed PR for this issue. I want to mention that there's another approach to the whole "pickleability" issue. The problem with the pickling approach is that I don't think it can ever be completely solved. It seems like there will always be cases that can't be pickled, so attempts at further fixing will always be partial workarounds.

Another approach is to render the exception to a string when the exception first happens, and to pass the string back (as strings are pickleable). This approach is discussed further in #29023, and a proof-of-concept patch is linked to there. Another advantage of first rendering to a string is that the full exception chain gets captured, which I don't think pickling addresses either (last I checked). (Ticket #29023 is about the exception chain aspect.)

comment:10 by Jonny Arnold, 2 years ago

Owner:	changed from Jordan Ephron to Jonny Arnold

I ran into this issue recently and feel I can pick this up following the comments on the old pull request.

comment:11 by Jonny Arnold, 2 years ago

Patch needs improvement:	unset

comment:12 by Mariusz Felisiak, 2 years ago

Needs tests:	set
Patch needs improvement:	set

comment:13 by Jonny Arnold, 2 years ago

Needs tests:	unset
Patch needs improvement:	unset

I've responded to the comment on the Pull Request. Please let me know if I need to make any further changes.

comment:14 by Mariusz Felisiak, 2 years ago

Patch needs improvement:	set

comment:15 by Mariusz Felisiak, 21 months ago

Owner:	Jonny Arnold removed
Status:	assigned → new

comment:16 by Sarah Boyce, 9 months ago

Cc:	Sarah Boyce added

comment:17 by Sage Abdullah, 9 months ago

Cc:	Sage Abdullah added

comment:18 by David Wobrock, 7 months ago

Cc:	David Wobrock added

comment:19 by David Wobrock, 4 months ago

Owner:	set to David Wobrock
Patch needs improvement:	unset
Status:	new → assigned

Here's a new minimal PR, based on previous work.

comment:20 by Mariusz Felisiak, 4 months ago

Patch needs improvement:	set

comment:21 by David Wobrock, 4 months ago

Patch needs improvement:	unset

comment:22 by Mariusz Felisiak, 3 months ago

Patch needs improvement:	set

comment:23 by David Wobrock, 2 months ago

Patch needs improvement:	unset

comment:24 by Mariusz Felisiak, 2 months ago

Triage Stage:	Accepted → Ready for checkin

comment:25 by Mariusz Felisiak <felisiak.mariusz@…>, 2 months ago

Resolution:	→ fixed
Status:	assigned → closed

In c09e8f5:

Fixed #32114 -- Fixed parallel test crash on non-picklable objects in subtests.

comment:26 by GitHub <noreply@…>, 2 months ago

In f835787:

Refs #32114 -- Fixed RemoteTestResultTest.test_unpicklable_subtest test without tblib.

Follow up to c09e8f5fd8f977bf16e9ec5d11b370151fc81ea8.

comment:27 by GitHub <noreply@…>, 2 months ago

In ef2434f:

Refs #32114 -- Fixed test crash on non-picklable objects in subtests when PickleError is raised.

Related to the https://github.com/python/cpython/issues/73373.