Fixed #36083 -- Ran system checks in ParallelTestSuite workers #19070
Conversation
|
@jacobtylerwalls The tests probably aren't done yet, but is this the direction you had in mind when we talked on Discord about unit testing |
There was a problem hiding this comment.
Looks right-track to me. This feels like the root of the problem with the parallel test runner, since system checks can have side effects (and will have them for the foreseeable future).
The symptom with available_apps making it impossible to boot the WSGI server given default middleware seems like just one possible symptom. Adding exceptions around that might just make things harder to reason about later?
Nice coverage increase, too.
I'm sure others will have thoughts. Left a couple minor comments. Thanks for picking this up!
|
It might be nice to look into whether we can de-dupe the results from the system checks. Running with 7 processes and getting 7x the diagnostics is not ideal. |
|
@jacobtylerwalls Do you think anything else is needed to move this PR out of draft status? |
|
Looks ready for review to me. Just reiterating (and I think you already know this) that there will probably be more discussion on whether this is the right path to take, but having a PR ready for review will help with decision making, I'm sure, so thank you! Re: what path to take here, my thinking is that if Django itself has system checks with side effects, it's reasonable to assume users will too. The parallel test runner is supposed to do whatever the regular runner does, just faster. So running the system checks on fork seems like a way to remove a vector for bugs. The alternatives we looked at on the ticket had drawbacks, but maybe someone will still think of something clever 👍 |
| @mock.patch("django.test.runner.setup_test_environment") | ||
| @mock.patch("django.setup") | ||
| @mock.patch("multiprocessing.get_start_method") | ||
| class InitWorkerTests(SimpleTestCase): |
There was a problem hiding this comment.
The _init_worker function was not unit tested.
There was a problem hiding this comment.
I appreciate that these add tests so that something would fail if we revert but I don't love that these tests just check that _init_worker is as defined. Would be good to have something slightly more meaningful if we can
There was a problem hiding this comment.
That's fair. I have a feeling that _init_worker wasn't tested before because it's difficult to test. I'll try to look into a better approach, but I'm open to suggestions in the meantime.
There was a problem hiding this comment.
@jacobtylerwalls Would you have any thoughts or guidance on quality of the test?
Workers created by ParallelTestSuite were not running system checks. In general this is fine, but system checks can have side effects that the tests expect. This patch runs system checks inside of _init_worker, which is only called by ParallelTestSuite.
d839934 to
9c2d1cb
Compare
There was a problem hiding this comment.
I think I'm ok with the approach. Will ask if @adamchainz has an opinion here?
@jacobtylerwalls I couldn't replicate the issue, can you confirm the issue is still there and this fixes it?
| @mock.patch("django.test.runner.setup_test_environment") | ||
| @mock.patch("django.setup") | ||
| @mock.patch("multiprocessing.get_start_method") | ||
| class InitWorkerTests(SimpleTestCase): |
There was a problem hiding this comment.
I appreciate that these add tests so that something would fail if we revert but I don't love that these tests just check that _init_worker is as defined. Would be good to have something slightly more meaningful if we can
|
Yep, the issue is still reproducible on Mac and this PR fixes it. (I would hazard you could reproduce yourself if you However, I'm hoping we can do something about the additional "System check identified no issues" if not too complicated: Example output |
Trac ticket number
ticket-36083
Branch description
Workers created by ParallelTestSuite were not running system checks. In general this is fine, but system checks can have side effects that the tests expect.
This patch runs system checks inside of _init_worker, which is only called by ParallelTestSuite.
Checklist
mainbranch.