#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 )
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 , 4 years ago
| Description: | modified (diff) |
|---|
comment:2 by , 4 years ago
| Component: | Uncategorized → Testing framework |
|---|
comment:3 by , 4 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:6 by , 4 years ago
| Cc: | added |
|---|
comment:7 by , 4 years ago
| Type: | Uncategorized → Cleanup/optimization |
|---|
comment:8 by , 4 years ago
| Patch needs improvement: | set |
|---|
comment:9 by , 4 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 , 3 years ago
| Owner: | changed from to |
|---|
I ran into this issue recently and feel I can pick this up following the comments on the old pull request.
comment:11 by , 3 years ago
| Patch needs improvement: | unset |
|---|
comment:12 by , 3 years ago
| Needs tests: | set |
|---|---|
| Patch needs improvement: | set |
comment:13 by , 3 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 , 3 years ago
| Patch needs improvement: | set |
|---|
comment:15 by , 2 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:16 by , 16 months ago
| Cc: | added |
|---|
comment:17 by , 15 months ago
| Cc: | added |
|---|
comment:18 by , 14 months ago
| Cc: | added |
|---|
comment:19 by , 11 months ago
| Owner: | set to |
|---|---|
| Patch needs improvement: | unset |
| Status: | new → assigned |
Here's a new minimal PR, based on previous work.
comment:20 by , 11 months ago
| Patch needs improvement: | set |
|---|
comment:21 by , 11 months ago
| Patch needs improvement: | unset |
|---|
comment:22 by , 10 months ago
| Patch needs improvement: | set |
|---|
comment:23 by , 9 months ago
| Patch needs improvement: | unset |
|---|
comment:24 by , 9 months ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
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)