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
Comment | File | Size | Author |
---|---|---|---|
#7 | interdiff.txt | 1.22 KB | Mile23 |
#7 | 2880911_7.patch | 9.2 KB | Mile23 |
#5 | interdiff.txt | 1.46 KB | Mile23 |
#5 | 2880911_5.patch | 9.2 KB | Mile23 |
#3 | 2880911_3.patch | 7.75 KB | Mile23 |
Comments
Comment #2
dawehnerI 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.
Comment #3
Mile23OK, 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 byrun-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 leavesisTestInIsolation()
as vestigial, so it's deprecated.Comment #5
Mile23I thought I searched for all the tests touching that method, but I guess I didn't do a good enough job.
Comment #6
dawehnerI'm wondering whether we could document why it is discouraged ...
Comment #7
Mile23Done.
Comment #8
dawehnerGiven these methods are private they are totally fine to be removed, yeah!
Comment #9
alexpottCommitted 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.
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.
Comment #13
Mile23Did we miss a spot? #3038825: Remove unused code from KernelTestBase