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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Issue summary: View changes
Mile23’s picture

Here 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.

Mile23’s picture

Issue summary: View changes

The last submitted patch, 3: 2881788_3_runTestsInSeparateProcesses.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2881788_3_tearDown.patch, failed testing.

Mile23’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
19.51 KB
19.47 KB

This 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.

Mile23’s picture

Title: Some Drupal\Tests\Core\Extension unit tests have bad isolation » Process-isolate Drupal\Tests\Core\Extension\ModuleHandlerTest
Issue summary: View changes

I can't find any other unit tests that load modules like this.

Mile23’s picture

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Do 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

+++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
@@ -20,28 +23,38 @@ class ModuleHandlerTest extends UnitTestCase {
+   * Since we have to run these tests in separate processes, we have to use
+   * test objects which are serializable. Since ModuleHandler will populate
+   * itself with Extension objects, and since Extension objects will try to
+   * access DRUPAL_ROOT when they're unserialized, we can't store our mocked
+   * ModuleHandler objects as a property in unit tests. They must be generated
+   * by the test method by calling this method.

To be honest its best practise to not do too much in setup anyway.

Mile23’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Done.

Mile23’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

  • catch committed 4689d73 on 8.4.x
    Issue #2881788 by Mile23: Process-isolate Drupal\Tests\Core\Extension\...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.