Problem/Motivation
As @dawehner points out in #2866779-24: Add a way to trigger_error() for deprecated hooks unit tests which end up loading modules should run in isolated processes using @runTestsInSeparateProcesses
.
Since the patch he's referring to was somewhat copy-pasted from Drupal\Tests\Core\Extension\ModuleHandlerTest
we should do that, too.
It sets up a fixture ModuleHandler
object, and then calls ->load()
and ->loadAll()
on that fixture. This results in loading fixture modules such as module_handler_test_added
and module_handler_test_all1
.
The ModuleHandler
object is discarded and re-created with each test method, but the autoloadable namespace remains between each test method, and is subsequently present in all other unit tests running in the same process.
So adding @runTestsInSeparateProcesses
would serve to isolate this autoloading behavior from other tests. Except for one thing: This then causes the system under test to need to serialize the test class.
@runTestsInSeparateProcesses
works by serializing the test class and then making a PHP file with the serialized data, which is then reconstituted in a separate process.
That's fine except that ModuleHandler
stores an array of Extension
objects. Extension::unserialize()
has a dependency on DRUPAL_ROOT
in order to guess the installation root directory. DRUPAL_ROOT
is (rightly) not initialized for unit tests, so this results in an error.
Therefore, since Extension::unserialize()
makes assumptions about Drupal's bootstrap level, we can't test ModuleHandler
in isolation as a unit test. At least with the fixture ModuleHandler
we currently use.
In practice, using run-tests.sh
(and thus the testbot) forces all test classes to be run as separate processes. This mean that ModuleHandlerTest
will be isolated unto itself, but we can't guarantee process isolation for each test method against other methods, which @runTestsInSeparateProcesses
does.
Proposed resolution
Re-architect Drupal so it doesn't use DRUPAL_ROOT
ever. I only half-way joke here: https://www.drupal.org/node/2330441 and #2881981: Mitigate Extension dependency on DRUPAL_ROOT
Work to isolate the ModuleHandler
dependency on DRUPAL_ROOT
from the serialization process in the test by removing it from the setUp()
phase and generating the mock within the test method itself.
Determine if other adjacent tests need similar treatment. None of the other Extension tests seem to have this problem.
Remaining tasks
Follow-up to do a better job of unserializing Extension objects using services if available. #2881981: Mitigate Extension dependency on DRUPAL_ROOT
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#7 | interdiff.txt | 19.47 KB | Mile23 |
#7 | 2881788_7.patch | 19.51 KB | Mile23 |
#3 | 2881788_3_tearDown.patch | 763 bytes | Mile23 |
#3 | 2881788_3_runTestsInSeparateProcesses.patch | 502 bytes | Mile23 |
Comments
Comment #2
Mile23Comment #3
Mile23Here are two patches:
One only annotates
@runTestsInSeparateProcesses
which shows you the explosion.The other adds a
tearDown()
implementation which wipes out the fixture objects so they don't have to be serialized. This causes test fails which may illustrate bad tests.Comment #4
Mile23Comment #7
Mile23This patch just fixes the test by not populating the module handler as a property during setUp().
This resulted in two test methods failing because they inadvertently depended on a fixture module already being loaded by the module handler.
The interdiff is kind of meaningless but diffs off the tearDown patch above.
Comment #8
Mile23I can't find any other unit tests that load modules like this.
Comment #9
Mile23Comment #10
dawehnerDo you mind updating the issue summary with the version of your latest patch? It talks about a completely different solution, compared to what the patch is doing. Thank you
To be honest its best practise to not do too much in
setup
anyway.Comment #11
Mile23Done.
Comment #12
Mile23Comment #13
dawehnerThank you
Comment #15
catchCommitted/pushed to 8.4.x, thanks!