Problem/Motivation

I'm calling this a bug because when you lift the box, the roaches scurry.

This is all the result of trying to work on deprecation error problems from #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code

KernelTestBase has two items of interest. Here is the docblock for KernelTestBase::getCompiledContainerBuilder(), which builds a container and stores it for use in all tests.

  /**
   * [...]
   * This method is not invoked if there is only a single test method. It is
   * also not invoked for tests running in process isolation (since each test
   * method runs in a separate process).
   * [...]
   */
  private function getCompiledContainerBuilder(array $modules) {

Note that it says we don't use this precompiled container if the test class is in a separate process.

Now, let's take a look at KernelTestBase::$runTestInSeparateProcess, which it inherits from PHPUnit_Framework_TestCase:

  /**
   * {@inheritdoc}
   *
   * Kernel tests are run in separate processes to prevent collisions between
   * code that may be loaded by tests.
   */
  protected $runTestInSeparateProcess = TRUE;

This forces all tests to be run in separate processes, always.

If we comment out this property (causing all tests to be run in the same process), we end up with assertion errors from ContainerBuilder::__sleep(). That's because getCompiledContainerBuilder() is so vestigial that it breaks when it tries to instantiate a container.

This is because it's never used, since KernelTestBase tests are always process isolated, and because there's no test for it.

If we bypass it, we get failed tests:

$ ./vendor/bin/phpunit -c core/ --testsuite kernel --filter DatabaseBackendTest
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing 
F..........

Time: 29.45 seconds, Memory: 56.00MB

There was 1 failure:

1) Drupal\KernelTests\Core\Cache\DatabaseBackendTest::testSetGet
Created time is correct.
Failed asserting that false is true.

/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/Core/Cache/GenericCacheBackendUnitTestBase.php:158
/Users/paul/pj2/drupal/core/tests/Drupal/KernelTests/Core/Cache/DatabaseBackendTest.php:35

FAILURES!
Tests: 11, Assertions: 136, Failures: 1.

Proposed resolution

Remove getCompiledContainerBuilder().

Deprecate the now-vestigial isTestInIsolation().

Figure out and fix resulting failing tests.

Remaining tasks

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.

dawehner’s picture

Consider allowing KernelTestBase to not be a separate process by default, and allowing child tests to mark themselves as separate as needed.

I don't think this will fly. Kernel tests will always change state, as for example it will load module code in. Once this code is loaded, it will never be unloaded. This itself adds some unforeseeable and really hard to debug risk.

Mile23’s picture

Title: KernelTestBase doesn't need to be a separate process always » Remove unused KernelTestBase::getCompiledContainerBuilder()
Issue summary: View changes
Status: Active » Needs review
FileSize
7.75 KB

OK, so if we always run kernel tests in separate processes, why do we keep a cached container?

run-tests.sh (and thus the testbot) always runs all test classes as a separate process.

On top of that, phpunit (launched by run-tests.sh) runs each test method as another process, because we have $runTestInSeparateProcess = TRUE.

Here's some rescope and a patch which removes the private getCompiledContainerBuilder(). This leaves isTestInIsolation() as vestigial, so it's deprecated.

Status: Needs review » Needs work

The last submitted patch, 3: 2880911_3.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
9.2 KB
1.46 KB

I thought I searched for all the tests touching that method, but I guess I didn't do a good enough job.

dawehner’s picture

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -1073,13 +988,21 @@ protected function prepareTemplate(\Text_Template $template) {
+   *
+   * Note that KernelTestBase will run in a separate process by default.
+   * Subclasses can override this by setting
+   * KernelTestBase::$runTestInSeparateProcess to FALSE. This is discouraged.

I'm wondering whether we could document why it is discouraged ...

Mile23’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Given these methods are private they are totally fine to be removed, yeah!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1588e2b and pushed to 8.4.x. Thanks!
Committed 4858475 and pushed to 8.3.x. Thanks!

This is really nice work. Less complexity FTW.

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -1073,13 +989,20 @@ protected function prepareTemplate(\Text_Template $template) {
+    @trigger_error(__CLASS__ . '::' . __FUNCTION__ . '() is deprecated in Drupal 8.4.x, for removal before the Drupal 9.0.0 release. KernelTestBase tests are always run in isolated processes.', E_USER_DEPRECATED);

Normally we'd have a CR for something like this but I'm not sure it is worth it. It's great that there is a test for this though.

  • alexpott committed 1588e2b on 8.4.x
    Issue #2880911 by Mile23, dawehner: Remove unused KernelTestBase::...

  • alexpott committed 4858475 on 8.3.x
    Issue #2880911 by Mile23, dawehner: Remove unused KernelTestBase::...

Status: Fixed » Closed (fixed)

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

Mile23’s picture