KernelTestBaseTNG™

*awesome*

::hands prize::

I suspect that Drush attempts to install Drupal in a wrong filesystem directory (since no longer absolute)

Drush is good.

Dependency on Drush is bad.

Amazing! Magnificent. Wow.

So. Much. Awesome.

As a part of BrowserTestBase we started migrating alot of the asserts over to PHPUnit and implemented it as the runner. Looks like Im overlapping alot with your work.

https://twitter.com/sanic1989/status/497249190074781698

Left a tell on IRC, looking to have a chat about what steps I should be taking from here.

How are we going to run tests like the Database tests against multiple database backends if we're forcing it down to sqlite here? Fixing all the broken sqlite tests and discovering them is great, but how do we ensure that they continue to work against mysql and postgreSQL too?

Yesterday @nick_schuch showed me a working prototype of a new container-based testbot using Jenkins and Travis CI's runner component with this patch.
It was over my head so I'll leave it to him to better describe what he's done, but it was pretty nice to see it run through using travis config but on private hardware.

Following on from #13 and #14...

Roughly a week ago myself, @larowlan and @grom358 had a chat with @sun about some work that we were doing with an "Acceptence test base" running on Mink and PHPUnit. During this chat @sun pointed us to this story and the work that he had been doing, it's freaking awesome by the way. I noticed that this was being tested with TravisCI.

Over the weekend I found the following project while looking through the TravisCI github repo: https://github.com/travis-ci/travis-build. At the time I did not think of core and thought of how I could bring it into my PreviousNext workflow, Im now using it for Puppet unit tests and deployments. The workflow for this is 1) Mount the files into a containers that has travis-build 2) Run travis-build 3) Tear it down afterwards. This was a huge win for me.

Yesterday I had a crazy idea, how hard would it be to bring this into core. Being a part of the DrupalCI group I didn't want to stand on any toes, but wanted to experiment and see if I could take @sun's repo/branch and run it on Jenkins with this travis project. It was a success, but I didn't have anything PHP packed away, so what I have now is:

* 1 x Base Docker container that installs PHPENV, Composer and Travis build.
* 1 x PHP 5.4 Docker container.
* 1 x PHP 5.5 Docker container.

While the travis-build project could only run the "env, before_script, script....." commands, I saw the need for something that builds a little script to run all the permutations (PHP 5.4 + Mysql + Postgres and PHP 5.5 + Mysql + Postgres). So I wrote a little "compiler" as a proof of concept, all it does is 1) Read the .travis.yml file 2) Return a bash script with Docker commands that link services (containers).

The work I have done so far can be found here: https://github.com/nickschuch/docker-drupal
A link to the build (just running PHP 5.4): http://107.170.87.127:8080/job/travisci/70/console
A link to @sun's travis build (to show that my Jenkins build produces the exact same result): https://travis-ci.org/sun/drupal/builds/31851982

@nick_schuch: Pretty cool. :-) Standing on toes isn't such a bad thing. Please ping jthorson about it if you haven't already.

Thanks guys!

I have left tells to @jthorson and @ricardoamaro on IRC with the link to this issue. We have our weekly meetings so wil bring it up there if I don't here from them prior.

Needs re-roll. Sad to say.

Worked on a reroll for fun, but this is certainly in not the best state.

Note: The patchsize might be reduced that much because the changes to phpunit got revereted (I assume that all the changes upstream got in again).

For the testbot

just some work.

Fatal error: Call to undefined function Drupal\Component\PhpStorage\drupal_tempnam() in /Users/tim.plunkett/www/d8/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php on line 73

Call Stack:
    0.0011     246296   1. {main}() /Users/tim.plunkett/.bin/drush_lib/drush.php:0
    0.0342    2465760   2. drush_main() /Users/tim.plunkett/.bin/drush_lib/drush.php:16
    0.1767    7257432   3. Drush\Boot\DrupalBoot->bootstrap_and_dispatch() /Users/tim.plunkett/.bin/drush_lib/drush.php:76
    0.1769    7265528   4. drush_bootstrap_to_phase() /Users/tim.plunkett/.bin/drush_lib/lib/Drush/Boot/DrupalBoot.php:35
    0.2287    7017944   5. drush_bootstrap() /Users/tim.plunkett/.bin/drush_lib/lib/Drush/Boot/bootstrap.inc:308
    0.2288    7019024   6. _drush_bootstrap_drupal_full() /Users/tim.plunkett/.bin/drush_lib/lib/Drush/Boot/bootstrap.inc:186
    0.2288    7036480   7. drupal8_bootstrap() /Users/tim.plunkett/.bin/drush_lib/lib/Drush/Boot/bootstrap.inc:688
    0.2296    7040040   8. Drupal\Core\DrupalKernel->boot() /Users/tim.plunkett/.bin/drush_lib/lib/Drush/Boot/bootstrap.inc:812
    0.2298    7056256   9. Drupal\Core\DrupalKernel->initializeContainer() /Users/tim.plunkett/www/d8/core/lib/Drupal/Core/DrupalKernel.php:399
    0.7435   16715432  10. Drupal\Core\DrupalKernel->dumpDrupalContainer() /Users/tim.plunkett/www/d8/core/lib/Drupal/Core/DrupalKernel.php:747
    0.8681   17797160  11. Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->save() /Users/tim.plunkett/www/d8/core/lib/Drupal/Core/DrupalKernel.php:1120

Just posting some general progress.

CONCAT_WS is fixed in #2427311: SQLite does not natively support CONCAT_WS().

How to continue with this issue:

1. Creating a drop-in replacement for Drupal\simpletest\KernelTestBase is not going to make it in this stage of Drupal 8. Creating a parallel class Drupal\simpletest\KernelUnitTestBase and moving the actual kernel-tests in follow-ups is the only viable solution at this point in the Drupal 8 core development.
2. This issue want to use SQLite in memory to speed up the testbot. The current issue for that is #2318191: [meta] Database tests fail on SQLite. That issue makes SQLite pass all the database tests. Is that enough or does SQLite need to pass all other tests as well?

@daffie
In general, have a look at https://www.drupal.org/core/beta-changes#unfrozen ... if we think this is an improvement and speed development up, I think its
worth to rip out the existing api. Note: This patch still includes the old class, so you could use it, if needed.

@dawehner: I thought that a change like this issue was not allowed any more. Nice to hear. :) Can you reply to my second point. #2318191: [meta] Database tests fail on SQLite is almost finished, so do we need SQLite to pass all tests or only the database ones.

Some work, now many tests are running again (17 fails in 400 running of 1131 tests), it just needs time.

For the testbot

A couple of test fixes in the meantime, many of them actually reduce the patch size, as they have been rebase conflicts.

Worked on quite additional test coverage.

Note: Personally I'm not sure wether we want to add the log checking as part of the patch. One thing I did in the recent interdiff,
is to remove the requirement to set the expected logs, but just make it optional to check it.

In total we are with that done to:

1131 total, 27 failed, 1104 passed

@berdir
Currently focused on the tests but I really think we should be able to run them via SQL as well.

As far as I see the remaining failures are all caused by some kind of missing deterministic sort order of Config\DatabaseStorage::listAll()

here is some various cleanups and bug fixes.

Here is an initial piece of code which allows you to (optionally) specify a different db connection than the one used by default, which is some form of sqlite.

In order to do that, at least for now, you have to specify an environment variable:

cd core
PHPUNIT_DBURL=mysql://root:@localhost/dev_d8
phpunit --testsuite Kernel

(which can also be done in phpstorm, just in case someone is interested).

Due to using the same prefix as simpletest we can leverage the cleanup functionality of simpletest itself, in case needed.
@berdir
Do you think something like that would be okay?

Fixed the DB connection test.

When #2363341: Throw exception in Drupal::service() and friends, if container not initialized yet. lands the patch needs updating.

A first look. Will do more reviewing later. Good work dawehner! :)

1. +++ b/.travis.yml
   @@ -0,0 +1,18 @@
   +  # @todo Update to PHPUnit 4.3 (once released).
   

   We are currently at PHPUnit 4.4

2. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -306,13 +310,15 @@ protected function getAllCollectionNamesHelper($directory) {
   diff --git a/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   
   +++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
   @@ -113,6 +113,10 @@ public function render(ResultRow $values) {
   diff --git a/core/modules/views/src/Plugin/views/query/Sql.php b/core/modules/views/src/Plugin/views/query/Sql.php
   

   These two parts are also in #2428695: SQLite date handling is wrongly implemented and arguments handling needs override. If we get that committed this patch get a bit smaller. Also the critical issue #2318191: [meta] Database tests fail on SQLite is then fixed.

3. +++ b/core/modules/views/src/Tests/Handler/ArgumentDateTest.php
   @@ -163,15 +163,20 @@ public function testWeekHandler() {
   +    // SQLite does not support advanced date operations (such as interpreting
   +    // 2000-01-01 as the 52nd week of 1999).
   +    if ($this->container->get('database')->driver() !== 'sqlite') {
   +      $view->setDisplay('embed_3');
   +      // The first jan 2000 was still in the last week of the previous year.
   +      $this->executeView($view, array(52));
   +      $expected = array();
   +      $expected[] = array('id' => 1);
   +      $expected[] = array('id' => 2);
   +      $expected[] = array('id' => 3);
   +      $this->assertIdenticalResultset($view, $expected, $this->columnMap);
   +      $view->destroy();
   +    }
   

   Do we need this in this issue?

4. Is is possible to create some child issues for this patch so that the main patch will get a bit smaller.

Some more reviewing

1. +++ b/.travis.yml
   @@ -0,0 +1,18 @@
   +language: php
   
   +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   @@ -103,7 +103,7 @@ public function save($name, $data) {
   diff --git a/core/lib/Drupal/Core/Config/FileStorage.php b/core/lib/Drupal/Core/Config/FileStorage.php
   
   +++ b/core/lib/Drupal/Core/Queue/QueueMemoryFactory.php
   @@ -0,0 +1,28 @@
   + * Contains \Drupal\Core\Queue\QueueMemoryFactory.
   ...
   diff --git a/core/lib/Drupal/Core/StreamWrapper/LocalStream.php b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   
   +++ b/core/modules/config/src/Tests/Storage/CachedStorageTest.php
   @@ -36,7 +36,7 @@ class CachedStorageTest extends ConfigStorageTestBase {
   +    $this->fileStorage = new FileStorage(config_get_config_directory(CONFIG_ACTIVE_DIRECTORY));
   
   +++ b/core/modules/file/src/Tests/FileManagedUnitTestBase.php
   @@ -32,6 +32,8 @@ protected function setUp() {
   diff --git a/core/modules/file/tests/file_test/file_test.module b/core/modules/file/tests/file_test/file_test.module
   

   Can we create a child issue for the stream wrapper/vfs stuff?

2. +++ b/core/includes/bootstrap.inc
   @@ -106,7 +106,9 @@
   -define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
   +if (!defined('REQUEST_TIME')) {
   +  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
   +}
   
   @@ -134,7 +136,9 @@
   -define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
   +if (!defined('DRUPAL_ROOT')) {
   +  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
   +}
   

   These lines are also added in #2232861: Create BrowserTestBase for web-testing on top of Mink.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php
   @@ -49,9 +49,10 @@ public function __sleep() {
   diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php
   
   +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1054,13 +1074,23 @@ protected function compileContainer() {
   diff --git a/core/lib/Drupal/Core/DrupalKernelInterface.php b/core/lib/Drupal/Core/DrupalKernelInterface.php
   

   Maybe we can also create a child issue for the Drupal/Core/DrupalKernel stuff.

@daffie
Thank you for your reviews!

So to be clear, there are more failing tests than just the DB tests itself for sqlite. I think we really should solve them as well.

In general, sure, we should extract many of those tiny fixes into subissues, but continuously work on this issue, to have the big picture available.

@dawehner: There is a problem with PHPUnit tests and the database connection. The run-tests.sh script creates a database connection and then executes the phpunit command. The problem is that the database connection is lost after calling the phpunit command. #2232861: Create BrowserTestBase for web-testing on top of Mink has the same problem.

@dawenher: So we should create an issue for SQLite like #2157455: [Meta] Make Drupal 8 work with PostgreSQL or remove support from core before release.

So we have

FAILURES!                                                           
Tests: 1131, Assertions: 15091, Failures: 5, Errors: 11, Skipped: 5.

see the full travis result here https://travis-ci.org/jibran/drupal/builds/51709661

+++ b/core/tests/Drupal/Tests/KernelTestBase.php
@@ -0,0 +1,1304 @@
+    \Drupal::setContainer(NULL);
...
+      \Drupal::setContainer(NULL);
...
+    \Drupal::setContainer(NULL);

#2363341: Throw exception in Drupal::service() and friends, if container not initialized yet. has landed. These lines can now be replaced by:

\Drupal::unsetContainer();

Just a reroll with the fixes in #53 and some work on various tests.

After talking a bit with @alexpott we decided to NOT convert all existing tests over but rather migrate over them.
The only tests which are left:

• ModuleHandlerTest, to have a good and complex example
• KernelTestBaseTest, to test the new test class itself
• PhpStorageFactoryTest, because we require changes in the PhpStorageFactory to run anyway

This should maybe at least get the installer working again.

• Removed the functionality to check for logs, we can add this later
• Fixed the failures, YEAH!!!
• Ensured that migrate/user unit tests can be executed again.
• Maybe this is green for itself.

This is a big tricky patch to review. Couple quick observations.

1. +++ b/core/lib/Drupal/Core/Extension/InfoParser.php
   @@ -19,6 +19,8 @@ class InfoParser implements InfoParserInterface {
   +   * Especially during kernel tests, YAML files are re-parsed often.
   +   *
   

   This is a weird comment on an untouched file. There are a couple issues looking at slow YAML problems already so I'm not sure its needed. The reference to kernel tests is the sort of thing that would stick around and be cruft long after it was fixed anyways.

2. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1210 @@
   +      'databasePrefix', // @todo ¶
   

   undocumented todo and a trailing whitespace.

+++ b/.travis.yml
@@ -0,0 +1,18 @@
+  # @todo Update to PHPUnit 4.3 (once released).
+  - ./core/vendor/phpunit/phpunit/phpunit -v -c core --testsuite Kernel

4.5 is the latest stable and core has 4.4.2 so this is at the least outdated

Why are we adding a travis file?

Thank you @neclimdul
All of those are valid points, let's clean things up.

1. +++ b/core/includes/bootstrap.inc
   @@ -87,7 +87,9 @@
   -define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
   +if (!defined('REQUEST_TIME')) {
   +  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
   +}
   
   @@ -115,7 +117,9 @@
   -define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
   +if (!defined('DRUPAL_ROOT')) {
   +  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
   +}
   

   Could we add some short docs explaining why these are conditionally defined?

   I can imagine why, but we don't want future readers of this code be left wondering why.

2. +++ b/core/includes/install.inc
   @@ -211,7 +211,7 @@ function drupal_rewrite_settings($settings = array(), $settings_file = NULL) {
   -  $contents = file_get_contents(DRUPAL_ROOT . '/' . $settings_file);
   +  $contents = file_get_contents($settings_file);
   
   @@ -316,7 +316,7 @@ function drupal_rewrite_settings($settings = array(), $settings_file = NULL) {
   -    if (file_put_contents(DRUPAL_ROOT . '/' . $settings_file, $buffer) === FALSE) {
   +    if (file_put_contents($settings_file, $buffer) === FALSE) {
   

   Is this change really necessary?

3. +++ b/core/includes/install.inc
   --- a/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   
   +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
   --- a/core/lib/Drupal/Core/Config/FileStorage.php
   +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   
   +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   index 3386bba..72d5f31 100644
   --- a/core/lib/Drupal/Core/PhpStorage/PhpStorageFactory.php
   

   The changes in these files look like they could be done in a child issue. That'd make this patch itself simpler, too.

4. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   index cb6a27d..9de7e71 100644
   --- a/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php
   
   +++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php
   index eb841b2..94061f8 100644
   --- a/core/lib/Drupal/Core/DrupalKernel.php
   
   +++ b/core/lib/Drupal/Core/DrupalKernel.php
   --- a/core/lib/Drupal/Core/DrupalKernelInterface.php
   +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   

   Another child issue, as also requested in #48.

5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -696,6 +704,13 @@ protected function initializeContainer($rebuild = FALSE) {
   +    // @see \Drupal\Tests\KernelTestBase::setUp()
   

   A reference to KTB in the kernel? That seems weird.

6. +++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
   @@ -80,8 +80,12 @@ public function getInfo($type) {
   -    $info = isset($this->elementInfo[$theme_name][$type]) ? $this->elementInfo[$theme_name][$type] : array();
   -    $info['#defaults_loaded'] = TRUE;
   +    $info = [];
   +    if (isset($this->elementInfo[$theme_name][$type])) {
   +      $info = $this->elementInfo[$theme_name][$type];
   +      $info['#defaults_loaded'] = TRUE;
   +    }
   

   Good catch, but could also be in a child issue.

7. +++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
   @@ -21,7 +21,12 @@ class ModuleHandlerTest extends KernelTestBase {
   +    // @fixme ModuleInstaller calls system_rebuild_module_data which is part of system.module.
   

   What would the fix for this look like?

8. +++ b/core/modules/system/src/Tests/PhpStorage/PhpStorageFactoryTest.php
   @@ -23,6 +23,19 @@
   +    // Empty the php storage settings, as KernelTestBase sets it by default.
   

   Nit: s/php/PHP/

9. +++ b/core/modules/system/src/Tests/PhpStorage/PhpStorageFactoryTest.php
   @@ -23,6 +23,19 @@
   +  }
   +
   +
   +  /**
   

   Nit: 2 instead of 1 \n.

10. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    + * @todo Extend ::setRequirementsFromAnnotation() and ::checkRequirements() to
    + *   account for '@requires module'.
    

    Wow. That sounds like a big DX win for KTB tests!

11. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +   * Reset any manipulations to global variables between tests.
    ...
    +   * Reset all static class properties between tests.
    ...
    +   * Exclude a few static class properties for performance.
    

    These begin with verbs, but they're properties, not methods.

12. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +   * this property. The values of all properties in all classes in the hierarchy
    

    s/hierarchy/class hierarchy/

13. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +  public static function setUpBeforeClass() {
    +    parent::setUpBeforeClass();
    +    chdir(__DIR__ . '/../../../../');
    +  }
    

    Could use a comment.

14. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +    $this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));
    

    No suggestions (is fine), just wanted to say that this looks like dark magic :)

15. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +    // Prepare a precompiled container for all tests of this class.
    +    // Substantially improves performance, since ContainerBuilder::compile()
    +    // is very expensive. Encourages testing best practices (small tests).
    

    \o/

16. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +    // @todo Move StreamWrapper management into DrupalKernel.
    ...
    +    // The temporary stream wrapper is able to operate both with and without
    +    // configuration.
    +    $this->registerStreamWrapper('temporary', 'Drupal\Core\StreamWrapper\TemporaryStream');
    +//    $this->registerStreamWrapper('stream_wrapper.vfs', 'vfs', 'Drupal\Tests\VfsStreamWrapperWrapper');
    

    A @todo and a commented line. Not sure if they're related?

17. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +   * Configuration accessor for tests. Returns non-overriden configuration.
    

    Nit: s/overriden/overridden/

18. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +   * @param $name
    

    Nit: typehint to string.

19. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +      // In the case we don't use sqlite directly we need to generate a DB prefix.
    

    Nit: 80 cols, s/sqlite/SQLite/

20. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +   *    flags [cf. Patchwork] to allow for `include 'vfs://container.php';`).
    

    Patchwork?

21. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +    // @todo Remove this after updating symfony/dependency-injection.
    +    // @see https://github.com/symfony/symfony/pull/11422
    +    $container->set('service_container', $container);
    

    That PR was merged a long time ago, so we should be able to fix this todo.

22. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +   * @param array $modules
    ...
    +   * @param array $modules
    ...
    +   * @param array $modules
    

    Nit: string[]

23. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +    // @todo Missing QueueMemoryFactory + QueueFactory type hints + no interface.
    +    //   Temporarily tampering with Settings instead.
    +    // @see \Drupal\Tests\KernelTestBase::bootEnvironment()
    +    $container
    +      ->register('queue.memory', 'Drupal\Core\Queue\QueueMemoryFactory');
    +    //$container
    +    //  ->setAlias('queue', 'queue.memory');
    

    I think this can be resolved now also?

24. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +   * @param string|array $modules
    ...
    +    foreach ((array) $modules as $module) {
    ...
    +   * @param string|array $tables
    

    Clever!

    But let's change it to string|string[]?

25. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1209 @@
    +   *   Code of previously enabled modules is still loaded. The modules are only
    +   *   removed from the active module list.
    

    That sounds pretty scary, but it's harmless, right? Because if a module is not in the active module list, its hooks aren't invoked etc.?

26. +++ b/core/tests/Drupal/Tests/KernelTestBaseTest.php
    @@ -0,0 +1,228 @@
    +class KernelTestBaseTest extends KernelTestBase {
    

    Wow. KTBTest both extends and tests KTB. Not sure what to think of that :D

Regarding point 26 in #64 -- IIRC, BrowserTestBaseNG (#2232861: Create BrowserTestBase for web-testing on top of Mink) does the same thing, and it seems to work OK :)

Thank you for your review!

Could we add some short docs explaining why these are conditionally defined?

I can imagine why, but we don't want future readers of this code be left wondering why.

Well yeah, we load bootstrap.inc for the tests, which conflicts with the constants defined by bootstrap.php of our phpunit configuration.

Is this change really necessary?

The changes in these files look like they could be done in a child issue. That'd make this patch itself simpler, too.

Its needed for VFS support. For example the call to tempnam() doesn't support any kind of streamwrappers.

+++ b/core/lib/Drupal/Core/DrupalKernelInterface.php

Well, setContainer() is called by KernelTestBase ... I doubt we can skip that :P

+++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php

Note: phpunit calls seiralize/unserialize for its internal static magic handling ... which causes the needs for those changes.

A reference to KTB in the kernel? That seems weird.

Well, I think its fair to document the case of having an already initialized container without a booted one.

Good catch, but could also be in a child issue.

... We have some basic test coverage of drupal_render() in the new KernelTestBaseTest ... and they fail due to this bug.

What would the fix for this look like?

#2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList added a todo.

No suggestions (is fine), just wanted to say that this looks like dark magic :)

Same black magic as UnitTestCase :)

A @todo and a commented line. Not sure if they're related?

We can clean things up here quite a bit.

Patchwork?

Rewrote the comment.

That PR was merged a long time ago, so we should be able to fix this todo.

Good catch!

I think this can be resolved now also?

We still don't have an interface for QueueFactory, but yeah I think we could partially skip this from the patch, if really needed.

That sounds pretty scary, but it's harmless, right? Because if a module is not in the active module list, its hooks aren't invoked etc.?

Well yeah. Just imagine some testcode calling $this->enableModules() ... $this->disableModules()
Note: This code is coming from the old KernelTestBase

1. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -189,6 +180,8 @@
   +    // Move the current dir to DRUPAL_ROOT.
   

   s/Move/Change/

2. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -370,17 +363,6 @@ private function bootKernel() {
   -    // Register basic stream wrappers to avoid dependencies on System module.
   -    // The public stream wrapper only depends on 'file_public_path'.
   -    // @todo Move StreamWrapper management into DrupalKernel.
   -    // @see https://drupal.org/node/2028109
   -    $this->streamWrappers = [];
   -    $this->registerStreamWrapper('public', 'Drupal\Core\StreamWrapper\PublicStream');
   -    // The temporary stream wrapper is able to operate both with and without
   -    // configuration.
   -    $this->registerStreamWrapper('temporary', 'Drupal\Core\StreamWrapper\TemporaryStream');
   -//    $this->registerStreamWrapper('stream_wrapper.vfs', 'vfs', 'Drupal\Tests\VfsStreamWrapperWrapper');
   -
   

   WOOT! :)

3. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -577,7 +558,7 @@ public function register(ContainerBuilder $container) {
   -    //$container
   +//    $container
   

   ?

Well, I think its fair to document the case of having an already initialized container without a booted one.

TRUE!

... We have some basic test coverage of drupal_render() in the new KernelTestBaseTest ... and they fail due to this bug.

Yes, not questioning that. Just saying we could split it off. Perhaps not worth it for such a tiny fix though.

Overall: what's necessary to push this to the finish line (i.e. get it committed)? Resolve the @todos? Can those be done in follow-ups? Should they?

Yes, not questioning that. Just saying we could split it off. Perhaps not worth it for such a tiny fix though.

Well, sure, you could say that. I indeed splitted up a lot in the meantime, see #2304461-56: KernelTestBaseTNG™. My idea was to keep those changes which would be needed for many other converted tests, so those conversions could happen in parallel.

Added all the tests we have to convert to the other issue.

RTBC+1 from me, but I think we need Wim and/or neclimdul to review the changes.

I think getting feedback from berdir or alexpott would be great.

Very nice work on the patch @dawehner. I went through all the patch mostly I found nits.

1. +++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
   @@ -21,7 +21,13 @@ class ModuleHandlerTest extends KernelTestBase {
   +    // @fixme ModuleInstaller calls system_rebuild_module_data which is part of system.module.
   

   More then 80 chars. Is it still pending?

2. +++ b/core/phpunit.xml.dist
   @@ -1,6 +1,10 @@
   -<phpunit bootstrap="tests/bootstrap.php" colors="true">
   +<phpunit
   +  bootstrap="tests/bootstrap.php"
   +  colors="true"
   +  verbose="true"
   +>
   

   I think we can revert that or make it one line.

3. +++ b/core/phpunit.xml.dist
   @@ -8,17 +12,23 @@
   +    <!-- @todo Verify glob patterns -->
   +    <!-- @todo Use TestSuite.php -->
   ...
   +      <!-- @todo Move to /tests/src/Kernel/* -->
   

   Are these still pending?

4. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1175 @@
   +   * @todo Move into Config-specific test base class.
   

   Another @todo.

More then 80 chars. Is it still pending?

Last time I checked, this was the case, see :

~/w/d8 (8.0.x) $ ag "system_rebuild_module_data" | grep "ModuleInstaller"                                                                                                                   │········································································
core/lib/Drupal/Core/Extension/ModuleInstaller.php:87:      $module_data = system_rebuild_module_data();

Oh right, just check the next line of the diff, it points to the corresponding issue already :)]

Are these still pending?

I killed 2 of them, as they are obvious, the other one is still valid.

Another @todo.

Still valid, because this could speed up some things a bit, if we don't install config by default.

Let's fix it.

+++ b/core/phpunit.xml.dist
@@ -1,10 +1,6 @@
-<phpunit
-  bootstrap="tests/bootstrap.php"
-  colors="true"
-  verbose="true"
->
+<phpunit bootstrap="tests/bootstrap.php" colors="true">

Just a silly question: was 'verbose="true"' removed intentionally?

Just a silly question: was 'verbose="true"' removed intentionally?

Yes, because @jibran asked for. This was reverted to the status of core.

I think all the new PHPUnit driven KernelTestBase tests will need to use the @runTestsInSeparateProcesses annotation since they all potentially load code and this preserves test isolation.

See https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi...

+++ b/core/includes/bootstrap.inc
@@ -86,7 +86,9 @@
-define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+if (!defined('REQUEST_TIME')) {
+  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+}

@@ -114,7 +116,9 @@
-define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+if (!defined('DRUPAL_ROOT')) {
+  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+}

Maybe we can use https://phpunit.de/manual/current/en/appendixes.annotations.html#appendixes.annotations.preserveGlobalState to not have this awkwardness

At the same time, running phpunit tests in separate process has a very high cost in terms of test time (quite often an order of magnitude), which may not always be necessary : most tests should not need it.

I think all the new PHPUnit driven KernelTestBase tests will need to use the @runTestsInSeparateProcesses annotation since they all potentially load code and this preserves test isolation.

At the same time, running phpunit tests in separate process has a very high cost in terms of test time (quite often an order of magnitude), which may not always be necessary : most tests should not need it.

@alexpott
I don't see the point why we need to mark all of them like that? If a test causes problem, as it loads code, we can still mark it.
Then we would also have the information that the test is dangerous.

Maybe we can use https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi... to not have this awkwardness

Well, if we set this, we pass over all the various global crap, which we want to avoid, no matter whether its in a separate process or not.

But Kernel tests have different modules loaded and you can't unload code. A kernel test is not a unit test.

And if a kernel test does not load any module code it does not need to be a kernel test.

Alright.

... I think we should opt the setting in for every test which causes issues, but let's see what happens if we enable it for the two tests we have.

There are two in complete tests here https://travis-ci.org/jibran/drupal/builds/51709661.
1) Drupal\Tests\migrate_drupal\Unit\source\d6\NodeRevisionByNodeTypeTest::testRetrieval
FakeSelect does not support multiple source identifiers, can not test.
2) Drupal\Tests\migrate_drupal\Unit\source\d6\NodeRevisionTest::testRetrieval
FakeSelect does not support multiple source identifiers, can not test.

There are two in complete tests here https://travis-ci.org/jibran/drupal/builds/51709661.
1) Drupal\Tests\migrate_drupal\Unit\source\d6\NodeRevisionByNodeTypeTest::testRetrieval
FakeSelect does not support multiple source identifiers, can not test.
2) Drupal\Tests\migrate_drupal\Unit\source\d6\NodeRevisionTest::testRetrieval
FakeSelect does not support multiple source identifiers, can not test.

So is it in HEAD and nobody cared forever.

I really don't have an answer to that.

There is an issue for those two incomplete tests: #2299795: Fix migrate NodeRevisionTest & NodeRevisionByNodeTypeTest. Wouldn't that actually be critical?

#89 it's because marked explicitly as incomplete:

  // core/modules/migrate_drupal/tests/src/Unit/source/d6/NodeRevisionByNodeTypeTest.php
  /**
   * {@inheritdoc}
   */
  public function testRetrieval() {
    // @todo: Fix this as per https://www.drupal.org/node/2299795
    $this->markTestIncomplete('FakeSelect does not support multiple source identifiers, can not test.');
  }

and

  // core/modules/migrate_drupal/tests/src/Unit/source/d6/NodeRevisionTest.php

  /**
   * {@inheritdoc}
   */
  public function testRetrieval() {
    // @todo: Fix this as per https://www.drupal.org/node/2299795
    $this->markTestIncomplete('FakeSelect does not support multiple source identifiers, can not test.');
  }

We need #2299795: Fix migrate NodeRevisionTest & NodeRevisionByNodeTypeTest

To be clear, we don't need that at all here, the testbot ignores that now, so will it in the future.
Let's please focus on this issue.

Incomplete tests are not critical.

+1 on #81 in spirit... Not sure we should use separate processes, however. The solution is that we shouldn't do things this way. :-) We should make an object similar to Symfony's Client class which encapsulates the kernel and all its dependencies. Then saying $client = NULL solves the problem. However, since we're basically making a SimpleTest compatibility layer we can't really do that.

Some stuff:

1. +++ b/core/includes/bootstrap.inc
   @@ -86,7 +86,9 @@
   -define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
   +if (!defined('REQUEST_TIME')) {
   +  define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
   +}
   

   Create a separate bootstrap file which defines these constants, and other constants needed by both testing systems and the front controller and other systems. As bootstrap.inc is eroded away in favor of OOP, the bootstrap you create here will grow.

   (The problem with bootstrap.inc is that it defines both constants and functions. Separate these concerns.)

2. +++ b/core/phpunit.xml.dist
   @@ -8,17 +8,21 @@
   +    <testsuite name="Kernel">
   +      <!-- @todo Move to /tests/src/Kernel/* -->
   +      <directory>./modules/**/src/Tests</directory>
   +      <directory>../modules/**/src/Tests</directory>
   +      <directory>../sites/*/modules/**/src/Tests</directory>
   +      <exclude>**/vendor</exclude>
   

   +1 on a separate suite. -1 on having it in a Kernel directory per the @todo. Instead use or whatever better file suffix you'd like.

3. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1180 @@
   +    $this->siteDirectory = vfsStream::url('root/sites/simpletest/' . $suffix);
   

   This makes me happier than you can understand.

4. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1180 @@
   +    // DrupalKernel::boot() is not sufficient as it does not invoke preHandle(),
   +    // which is required to initialize legacy global variables.
   +    $request = Request::create('/');
   +    $kernel->prepareLegacyRequest($request);
   

   Is there an issue for this, either way? That is: Either to invoke preHandle() or finalize the deprecation of legacy dependencies?

1) #2459155: Remove REQUEST_TIME from bootstrap.php

Thank you for the great feedback @mile23!

+1 on a separate suite. -1 on having it in a Kernel directory per the @todo. Instead use or whatever better file suffix you'd like.

Can you explain why you dislike a separate directory? Note: We already have the unit tests in tests/src/Unit/... for modules,
why should we not follow that scheme for kernel tests? I think its also a great idea in general to make it clear that things aren't unit tests but more integration tests. Doing that as part of the namespace seems to be, at least for me, a good solution for the problem.

Is there an issue for this, either way? That is: Either to invoke preHandle() or finalize the deprecation of legacy dependencies?

Well, feel free to create one, I'm not aware of one yet.

Let's give the reroll a try.

Let's fix failures!

Is there an issue for this, either way? That is: Either to invoke preHandle() or finalize the deprecation of legacy dependencies?

Well, feel free to create one, I'm not aware of one yet.

If you do lets go for the later rather then the former. preHandle() is connect to handle() not to boot() as is suggested by the name so lets not change that. boot's responsibility is suppose to be the most basic of tasks, mostly just getting a container. You should be able to boot a kernel and interact with that container without triggering those heavy legacy processing or doing any significant "bootstrapping". IMHO, anything in preHandle() is de facto deprecated as all "bootstrap" behaviors should be in the container going forward.

+++ b/core/tests/Drupal/Tests/KernelTestBaseTest.php
@@ -104,7 +104,8 @@ public function testSetUp() {
+    // We store teh active configuration in the DB.

w00t!

+++ b/core/includes/bootstrap.inc
@@ -114,7 +116,9 @@
+if (!defined('DRUPAL_ROOT')) {
+  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
+}

As requested before in #64 and #81, could this be documented or worked around? E.g. in which (valid) cases would this constant already exist?

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1042,7 +1058,12 @@ protected function initializeServiceProviders() {
   +        if (!is_object($class)) {
   +          $this->serviceProviders[$origin][$name] = new $class;
   +        }
   +        else {
   +          $this->serviceProviders[$origin][$name] = $class;
   +        }
   

   It is unclear why this check is performed, especially because DrupalKernel::serviceProviderClasses does not sufficiently document its structure.

2. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   @@ -58,6 +59,18 @@ public function getServiceProviders($origin);
   +   * Must be called before boot() in order to bootstrap the given container.
   

   Is this just a warning to inform people about the order of actions that are to be taken by them, or should we document DrupalKernelInterface::boot() that is throws an exception if no container is set?

1. +++ b/core/lib/Drupal/Core/Queue/QueueMemoryFactory.php
   @@ -0,0 +1,28 @@
   +class QueueMemoryFactory {
   

   I wonder if the introduction of a second queue factory warrants the introduction of a QueueFactoryInterface as well.

2. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -127,6 +127,14 @@ protected function getLocalPath($uri = NULL) {
   +    // In PHPUnit tests, the base path for local streams may be a virtual
   +    // filesystem stream wrapper URI, in which case this local stream acts like
   +    // a proxy. realpath() is (obviously) not supported by vfsStream.
   +    if (strpos($path, 'vfs://') === 0) {
   +      return $path;
   +    }
   

   Does this apply to any stream path?

3. +++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
   @@ -21,7 +21,13 @@ class ModuleHandlerTest extends KernelTestBase {
   +    // @fixme ModuleInstaller calls system_rebuild_module_data which is part of
   

   IIRC, we use @todo and not @fixme in Drupal.

4. +++ b/core/tests/Drupal/Tests/AssertLegacyTrait.php
   @@ -0,0 +1,105 @@
   +trait AssertLegacyTrait {
   

   Really lovely :)

5. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1180 @@
   +   * TRUE by default, but MUST be TRUE for kernel tests.
   

   This comes across a little confusing, because this is in KernelTestBase. I think this needs at least a better definition of what constitutes kernel tests, like do they use or test the kernel?

If you do lets go for the later rather then the former.

It would be great to streamline all that!
I hope you don't want to do that before this issue, given the explained fact that KTB does the same already. Here is a follow up: #2467021: Make it possible to just use Kernel::boot() and not require preHandle

Thank you @xano for you review! I'm ghat that is not a academic project but rather something people actually care about.

+    // We store teh active configuration in the DB.

Ha, here is a typo!

As requested before in #64 and #81, could this be documented or worked around? E.g. in which (valid) cases would this constant already exist?

Now, we at least for now, don't need that any longer. We might have to convert more uses of DRUPAL_ROOT once we do more tests.

It is unclear why this check is performed, especially because DrupalKernel::serviceProviderClasses does not sufficiently document its structure.

Alright, let's document it.

Is this just a warning to inform people about the order of actions that are to be taken by them, or should we document DrupalKernelInterface::boot() that is throws an exception if no container is set?

Well, I think if someone does, its pretty obvious that it happens, isn't it?

I wonder if the introduction of a second queue factory warrants the introduction of a QueueFactoryInterface as well.

Let's open an issue for that: #2467039: Add a QueueFactoryInterface

Does this apply to any stream path?

Well, ... its every non local stream wrapper, and VFS is kinda not a local stream wrapper.

IIRC, we use @todo and not @fixme in Drupal.

Good point. @fixme is about something which blocks the commit.

Really lovely :)

Yeah, most of the patch is coming from sun, also that really nice solution!

This comes across a little confusing, because this is in KernelTestBase. I think this needs at least a better definition of what constitutes kernel tests, like do they use or test the kernel?

What about rewriting the documentation in that way?

Some documentation improvements.

Thank you @xano!

Reroll. go go functional tests!

I'm not sure if run-test will pick up these tests after the run but i think it might. Worth a shot.

+++ b/core/tests/Drupal/Tests/KernelTestBase.php
@@ -0,0 +1,1188 @@
+      'database' => ':memory:',

I'm getting :memory: files in my filesystem. not sure this is working as intended.

I'm getting :memory: files in my filesystem. not sure this is working as intended.

@amateescu just explained me what is going on:

Note that ':memory:' cannot be used, because this script spawns
sub-processes. However, you may use e.g. '/tmpfs/test.sqlite'

... in previous versions of the patch though, we did not executed subprocesses for tests. Now we though do, due to the request of alexpott.
Given that we should remove that :memory: bit.

... even more, alexpott required us to always specify the database ... as everything else would be too much magic. Instead we should
add a handbook page on drupal.org how to setup your IDE / phpunit to run theme properly, by specifying the environment variable.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -148,13 +148,16 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +   * Allowing objects is for example used for allowing
   

   s/for allowing/to allow

2. +++ b/core/lib/Drupal/Core/PhpStorage/PhpStorageFactory.php
   --- /dev/null
   +++ b/core/lib/Drupal/Core/Queue/QueueMemoryFactory.php
   
   +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1188 @@
   +      // @todo Remove this; fix Queue factories + consuming code.
   +      // @see \Drupal\Tests\KernelTestBase::register()
   +      'queue_default' => 'queue.memory',
   

   Does comment #108 mean this can/should be removed from the patch?

3. +++ b/core/modules/simpletest/src/RandomGeneratorTrait.php
   @@ -0,0 +1,120 @@
   +    // Drupal\simpletest\WebTestBase::assertLink().
   

   Nit: missing leading backslash.

4. +++ b/core/modules/system/tests/src/Kernel/ModuleHandlerTest.php
   @@ -21,7 +21,13 @@ class ModuleHandlerTest extends KernelTestBase {
   +    // @todo ModuleInstaller calls system_rebuild_module_data which is part of
   +    // system.module, see https://www.drupal.org/node/2208429
   +    include_once $this->root . '/core/modules/system/system.module';
   

   Nit: This todo could be clarified: will this be solved as part of that linked issue, or is that merely an explanation of why this is necessary?

5. +++ b/core/modules/system/tests/src/Kernel/PhpStorage/PhpStorageFactoryTest.php
   @@ -23,6 +23,18 @@
   +    new Settings($settings);
   

   Interesting that this doesn't get assigned to anything.

6. +++ b/core/phpunit.xml.dist
   @@ -19,6 +19,17 @@
   +      <!-- Exclude Drush tests. -->
   +      <exclude>./drush/tests</exclude>
   

   Kinda strange that core cares about a specific contrib module?

7. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1188 @@
   + * installed. Modules need to be installed manually, if needed.
   

   Nit: s/need/must/

8. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1188 @@
   +   * Back-up and restore any global variables that may be changed by tests.
   ...
   +   * Back-up and restore static class properties that may be changed by tests.
   

   s/Back-up/Back up/

9. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1188 @@
   +    'Drupal\Component\Discovery\YamlDiscovery' => array('parsedFiles'),
   +    'Drupal\Core\DependencyInjection\YamlFileLoader' => array('yaml'),
   +    'Drupal\Core\Extension\ExtensionDiscovery' => array('files'),
   +    'Drupal\Core\Extension\InfoParser' => array('parsedInfos'),
   +    // Drupal::$container cannot be serialized.
   +    'Drupal' => array('container'),
   +    // Settings cannot be serialized.
   +    'Drupal\Core\Site\Settings' => array('instance'),
   

   s/array()/[]/

10. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1188 @@
    +    // Bootstrap the kernel. Do not use createFromRequest to retain Settings.
    

    s/createFromRequest/createFromRequest()/

11. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1188 @@
    +  /**
    +   * Returns the Database connection info to be used for this test.
    +   *
    +   * This method only exists for tests of the Database component itself, because
    +   * they require multiple database connections. Each SQLite :memory: connection
    +   * creates a new/separate database in memory. A shared-memory SQLite file URI
    +   * triggers PHP open_basedir/allow_url_fopen/allow_url_include restrictions.
    +   * Due to that, Database tests are running against a SQLite database that is
    +   * located in an actual file in the system's temporary directory.
    +   *
    +   * Other tests should not override this method.
    +   *
    +   * @return array
    +   *   A Database connection info array.
    +   *
    +   * @internal
    +   */
    +  protected function getDatabaseConnectionInfo() {
    +    $databases['default']['default'] = array(
    +      'driver' => 'sqlite',
    +      'namespace' => 'Drupal\\Core\\Database\\Driver\\sqlite',
    +      'host' => '',
    +      'database' => ':memory:',
    +      'username' => '',
    +      'password' => '',
    +      'prefix' => array(
    +        'default' => '',
    +      ),
    +    );
    +
    +    // Allow to specify a custom db_url.
    +    if ($db_url = getenv('PHPUNIT_DBURL')) {
    +      $databases['default']['default'] = $this->parseDbUrl($db_url);
    +
    +      // In the case we don't use SQLite directly we need to generate a DB
    +      // prefix.
    +      $suffix = mt_rand(100000, 999999);
    +      $this->databasePrefix = 'simpletest' . $suffix;
    +      $databases['default']['default']['prefix'] = ['default' => $this->databasePrefix];
    +    }
    +    return $databases;
    +  }
    +
    

    This looks also affected by the :memory: stuff in #108.

12. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1188 @@
    +   * @param array $modules
    

    s/array()/string[]/

13. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1188 @@
    +    // @todo Missing QueueMemoryFactory + QueueFactory type hints + no interface.
    +    //   Temporarily tampering with Settings instead.
    +    // @see \Drupal\Tests\KernelTestBase::bootEnvironment()
    +    $container
    +      ->register('queue.memory', 'Drupal\Core\Queue\QueueMemoryFactory');
    +    // $container
    +    //  ->setAlias('queue', 'queue.memory');
    

    Again #108-related.

14. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1188 @@
    +    // @todo Remove this BC layer.
    +    $this->containerBuild($container);
    

    Follow-up? Here?

15. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1188 @@
    +    // Destroy the in-memory database.
    

    Again #108-related.

16. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1188 @@
    +    // Ensure isLoaded() is TRUE in order to make _theme() work.
    ...
    +    // Ensure isLoaded() is TRUE in order to make _theme() work.
    

    _theme() doesn't exist anymore.

17. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1188 @@
    +    $content = drupal_render($elements);
    

    Nit: could call the service directly.

18. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1188 @@
    +   * @todo Move into Config-specific test base class.
    +   */
    +  protected function configImporter() {
    ...
    +   * @todo Move into Config-specific test base class.
    +   */
    +  protected function copyConfig(StorageInterface $source_storage, StorageInterface $target_storage) {
    

    Follow-up or here?

Rewrote the database handling to not have a default DB anymore but rather always require a SIMPLETEST_DB_URL to be set.

Does comment #108 mean this can/should be removed from the patch?

Removed it from the patch, we can come back to those memory implementations later again.

Nit: This todo could be clarified: will this be solved as part of that linked issue, or is that merely an explanation of why this is necessary?

IMHO that todo explains it. ModuleInstaller() calls to system_rebuild_module_data() which is part of system.module. What is hard to understand here?

Interesting that this doesn't get assigned to anything.

Its how it is.

Kinda strange that core cares about a specific contrib module?

Feel free to open up an issue for that, its in core already ...

Follow-up or here?

What about removing the todos? Here is a follow up: #2480055: Provide a config specific kerneltestbaseNG base class

Holy Carp, this issue!

+++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
@@ -58,6 +59,18 @@ public function getServiceProviders($origin);
   /**
+   * Sets the current container.
+   *
+   * Subsequently call static::boot() to bootstrap the container.
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   *   The container to set.
+   *
+   * @return $this
+   */
+  public function setContainer(ContainerInterface $container = NULL);

If this is actually needed, just use ContainerAwareInterface/ContainerAwareTrait. Interfaces can extend interfaces just fine.

Might want to wait on this, I think: #2389811: Move all the logic out of index.php (again)

If this is actually needed, just use ContainerAwareInterface/ContainerAwareTrait. Interfaces can extend interfaces just fine.

Interesting idea ...

Might want to wait on this, I think: #2389811: Move all the logic out of index.php (again)

As we are just interfering with the way how the container is build, this should not affect that particular issue.

The patch is green ...

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -471,6 +474,14 @@ public function getContainer() {
   +    $this->container = $container;
   +    return $this;
   

   Should this also update the global Drupal singleton?

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -563,7 +574,7 @@ public function discoverServiceProviders() {
   +        if (is_object($class) || class_exists($class)) {
   

   Can we/should we limit this to an interface? e.g (is_object($class) && $class instanceof SomeInterface)

3. +++ b/core/modules/system/tests/src/Kernel/ModuleHandlerTest.php
   @@ -21,7 +21,13 @@ class ModuleHandlerTest extends KernelTestBase {
   +    // @todo ModuleInstaller calls system_rebuild_module_data which is part of
   +    // system.module, see https://www.drupal.org/node/2208429
   +    include_once $this->root . '/core/modules/system/system.module';
   

   :) one day

4. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -0,0 +1,229 @@
   +    putenv('PHPUNIT_DBURL=sqlite://localhost/:memory:');
   

   Should we align this with the SIMPLETEST_DB_URL used by BrowserTestBase? Alternatively a followup to make it to the line with the convention here.

5. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -0,0 +1,229 @@
   +   * @depends testSetUp
   ...
   +    $this->assertArrayNotHasKey('destroy-me', $GLOBALS);
   ...
   +    $this->assertEquals($expected, $schema->findTables('%'));
   

   love it

6. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1180 @@
   +  /**
   +   * The name of the session cookie.
   +   */
   +  protected $strictConfigSchema = TRUE;
   

   c/p error?

7. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1180 @@
   +  protected function parseDbUrl($db_url) {
   

   Missing docblock

8. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1180 @@
   +      $info['path'] = DRUPAL_ROOT . '/' . $info['path'];
   ...
   +    $settings_services_file = DRUPAL_ROOT . '/sites/default' . '/testing.services.yml';
   ...
   +      $testing_services_file = DRUPAL_ROOT . '/' . $this->siteDirectory . '/services.yml';
   ...
   +    if (file_exists($test_env = DRUPAL_ROOT . '/sites/default/testing.services.yml')) {
   

   Anyway we avoid these new uses of DRUPAL_ROOT?

9. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1180 @@
   +  private function bootKernel() {
   

   Can we document why we went with private - to prevent someone inadvertently changing it later?

10. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1180 @@
    +    $modules = self::getModulesToEnable(get_class($this));
    

    Is there a reason we went with self here and not static? Would a child-class ever want to change the implementation?

11. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1180 @@
    +    $db_url = getenv('SIMPLETEST_DB');
    

    Early reference used a different environment variable - is this intended? If so can we document the difference.

12. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1180 @@
    +   * Based on the (always identical) list of $modules to enable, an initial
    +   * container is compiled, all instantiated services are reset/removed, and
    +   * this precompiled container is stored in a static class property. (Static,
    +   * because PHPUnit instantiates a new class instance for each test *method*.)
    +   *
    +   * 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).
    +   *
    +   * The ContainerBuilder is not dumped into the filesystem (which would yield
    +   * an actually compiled Container class), because
    +   *
    +   * 1. PHP code cannot be unloaded, so e.g. 900 tests would load 900 different,
    +   *    full Container classes into memory, quickly exceeding any sensible
    +   *    memory consumption (GigaBytes).
    +   * 2. Dumping a Container class requires to actually write to the system's
    +   *    temporary directory. This is not really easy with vfs, because vfs
    +   *    doesn't support yet "include 'vfs://container.php'.". Maybe we could fix
    +   *    that upstream.
    +   * 3. PhpDumper is very slow on its own.
    +   *
    

    <3<3<3 this doco

13. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1180 @@
    +      // Prevent the alias-based path processor, which requires a url_alias db
    +      // table, from being registered to the path processor manager. We do this
    +      // by removing the tags that the compiler pass looks for. This means the
    +      // url generator can safely be used within tests.
    +      $container->getDefinition('path_processor_alias')
    +        ->clearTag('path_processor_inbound')
    +        ->clearTag('path_processor_outbound');
    

    Should we check if $modules contains 'path' before doing this? Does this mean we can't test path with this base class. If so, should we document that.

14. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1180 @@
    +        ->setArguments(array(1));
    

    This will be ignored, see PhpassHashedPassword::enforceLog2Boundaries which is called in PhpassHashedPassword::__construct - maybe we should create a sub-class with a different enforceLog2Boundaries method?

15. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1180 @@
    +  public static function generatePermutations(array $parameters) {
    

    Should we move this to a trait instead of duplicating (already exists on TestBase)?

16. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1180 @@
    +  private static function getModulesToEnable($class) {
    

    This is duplicated from elsewhere too - should we have a trait?

17. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1180 @@
    +    // Fix missing bootstrap.php when $preserveGlobalState is FALSE.
    +    // @see https://github.com/sebastianbergmann/phpunit/pull/797
    

    Referenced PR is closed - so do we need to update this - or do we need a new reference?

18. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1180 @@
    +    // PHPUnit_Util_DeprecatedFeature no longer exists and is replaced by simply
    +    // triggering E_USER_DEPRECATED errors in upcoming PHPUnit versions.
    +    // @todo Remove this entire method after upgrading.
    +    if (class_exists('PHPUnit_Util_DeprecatedFeature')) {
    

    This contradicts itself. If the class no longer exists - why are we checking for it?

19. +++ b/core/tests/Drupal/Tests/LogException.php
    @@ -0,0 +1,49 @@
    +    $names = ['EMERGENCY', 'ALERT', 'CRITICAL', 'ERROR', 'WARNING', 'NOTICE', 'INFO', 'DEBUG'];
    +    if (isset($names[$this->severity])) {
    +      $name = 'WATCHDOG_' . $names[$this->severity];
    

    Does this predate PSR3?

20. +++ b/core/tests/Drupal/Tests/VfsStreamWrapperWrapper.php
    @@ -0,0 +1,151 @@
    +  public function __construct() {
    

    nit - most of the methods here need {@inheritdoc} but meh

Should this also update the global Drupal singleton?

Doesn't Drupal\Core\DrupalKernel::initializeContainer already do that, which is part of DrupalKernel::boot()

Can we/should we limit this to an interface? e.g (is_object($class) && $class instanceof SomeInterface)

Well, it would be one of two interfaces potentially, but sure, let's check it.

Should we align this with the SIMPLETEST_DB_URL used by BrowserTestBase? Alternatively a followup to make it to the line with the convention here.

Ups right, well, tests passed locally because I still had the right env variable setup, and testbot uses run-tests.sh which passes the stuff along differently.

c/p error?

yes.

Anyway we avoid these new uses of DRUPAL_ROOT?

Sure we can fix that.

Can we document why we went with private - to prevent someone inadvertently changing it later?

Well, i'm pretty sure I know why this approach was chosen in the first patch of sun. We want to ensure that Drupal runs with the limited setup code needed and not more.
Every additional needed dependencies should be part of the container / be done in clean ways so in case you would have to override it, we would sacrifice the future of having a specific limited amount of code to setup Drupal but instead
would end up again with more / unspecified setup code.

Is there a reason we went with self here and not static? Would a child-class ever want to change the implementation?

Well, not if you just call it from a private method anyway ...

Early reference used a different environment variable - is this intended? If so can we document the difference.

There is no difference ... just a user error.

Should we check if $modules contains 'path' before doing this? Does this mean we can't test path with this base class. If so, should we document that.

This will be ignored, see PhpassHashedPassword::enforceLog2Boundaries which is called in PhpassHashedPassword::__construct - maybe we should create a sub-class with a different enforceLog2Boundaries method?

This is a direct c&p from the old kernel test base.

Should we move this to a trait instead of duplicating (already exists on TestBase)?

What about moving this into its own component? This could be useful for itself.

This is duplicated from elsewhere too - should we have a trait?

At least the method name just exists one, do you know where its coming from?

Referenced PR is closed - so do we need to update this - or do we need a new reference?

Not sure

This contradicts itself. If the class no longer exists - why are we checking for it?

Good catch, well with that we talked about old versions of PHPUNIT, let's replace the usages with trigger_error directly as well.

Does this predate PSR3?

Well the problem is that PSR3 uses different constants as pretty much everything else, well for now this is not needed as part of the patch, as we removed the logger integration.

nit - most of the methods here need {@inheritdoc} but meh

We also don't need it for now ...

Form https://travis-ci.org/jibran/drupal/jobs/61527372 PR is at https://github.com/jibran/drupal/pull/1

33.85s$ ./core/vendor/phpunit/phpunit/phpunit -v -c core --testsuite kernel
PHPUnit 4.6.4 by Sebastian Bergmann and contributors.
Configuration read from /home/travis/build/jibran/drupal/core/phpunit.xml.dist
....ES..........E......
Time: 33.14 seconds, Memory: 6.75Mb
There were 2 errors:
1) Drupal\KernelTests\KernelTestBaseTest::testSetUp
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'DEFAULT NULL
) ENGINE = InnoDB DEFAULT CHARACTER SET utf8' at line 2: CREATE TABLE {foo} (
`name` VARCHAR DEFAULT NULL
) ENGINE = InnoDB DEFAULT CHARACTER SET utf8; Array
(
)
/home/travis/build/jibran/drupal/core/lib/Drupal/Core/Database/Connection.php:609
/home/travis/build/jibran/drupal/core/lib/Drupal/Core/Database/Connection.php:573
/home/travis/build/jibran/drupal/core/lib/Drupal/Core/Database/Schema.php:519
/home/travis/build/jibran/drupal/core/tests/Drupal/KernelTests/KernelTestBaseTest.php:97
Caused by
PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'DEFAULT NULL
) ENGINE = InnoDB DEFAULT CHARACTER SET utf8' at line 2
/home/travis/build/jibran/drupal/core/lib/Drupal/Core/Database/Statement.php:61
/home/travis/build/jibran/drupal/core/lib/Drupal/Core/Database/Connection.php:548
/home/travis/build/jibran/drupal/core/lib/Drupal/Core/Database/Schema.php:519
/home/travis/build/jibran/drupal/core/tests/Drupal/KernelTests/KernelTestBaseTest.php:97
2) Drupal\Tests\system\Kernel\Extension\ModuleHandlerTest::testUninstallContentDependency
PHPUnit_Framework_Exception: PHP Fatal error:  Class 'Drupal\Component\Utility\String' not found in /home/travis/build/jibran/drupal/core/tests/Drupal/Tests/KernelTestBase.php on line 767
Fatal error: Class 'Drupal\Component\Utility\String' not found in /home/travis/build/jibran/drupal/core/tests/Drupal/Tests/KernelTestBase.php on line 767
--
There was 1 skipped test:
1) Drupal\KernelTests\KernelTestBaseTest::testSetUpDoesNotLeak
This test depends on "Drupal\KernelTests\KernelTestBaseTest::testSetUp" to pass.
                                                  
FAILURES!                                         
Tests: 23, Assertions: 103, Errors: 2, Skipped: 1.
The command "./core/vendor/phpunit/phpunit/phpunit -v -c core --testsuite kernel" exited with 2.

Oh wow, thank you @jibran!

All good now https://travis-ci.org/jibran/drupal/jobs/61595149

#118 didn't fail. Is it alarming? Do we need a follow up to fix it?

#118 didn't fail. Is it alarming? Do we need a follow up to fix it?

Well, I fear you are right, those tests seems to be ignored at the moment by testbot, seriously, how did that changed

... this problem got introduced by #2304461-106: KernelTestBaseTNG™ the only patch without an interdiff :( :( :( The general change wasn't bad, functional tests also had its new place for \Drupal ...

Anyway figured it out now.

Reroll

So, I messed up the reroll :(
We need a beta evaluation and change notice.

@msonnabaum and myself discussed the process isolation a bit. Given that its a flag in phpunit itself, you can set it locally to run tests faster (I hope this is the right).

@jibran
Thank you for your try to reroll.
Tried at well, and now its at least green

Ha, and this time with a patch.

Fixed the failure by applies changes to KernelTestBase to thew new one.

Interdiff is from #2465887-21: Extract the install/uninstall functionality to a ThemeInstaller :(

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -148,13 +149,16 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   -   * List of discovered service provider class names.
   +   * List of discovered service provider class names or objects.
   
   @@ -563,7 +575,7 @@ public function discoverServiceProviders() {
   -        if (class_exists($class)) {
   +        if ((is_object($class) && ($class instanceof ServiceProviderInterface || $class instanceof ServiceModifierInterface)) || class_exists($class)) {
   

   This is kinda odd to me, but I guess it works.

2. +++ b/core/modules/simpletest/src/RandomGeneratorTrait.php
   @@ -0,0 +1,120 @@
   +trait RandomGeneratorTrait {
   

   Nice use of traits here and elsewhere.

3. +++ b/core/modules/simpletest/src/Tests/KernelTestBaseTest.php
   @@ -8,6 +8,7 @@
   +use org\bovigo\vfs\vfsStream;
   

   Side-note: everytime I read "org\bovigo" I think of "Oingo Boingo". Anyone else?

4. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
   @@ -21,10 +21,13 @@ class ModuleHandlerTest extends KernelTestBase {
   +    // @todo ModuleInstaller calls system_rebuild_module_data which is part of
   +    // system.module, see https://www.drupal.org/node/2208429
   

   If you reroll, this second line should be indented 2 more spaces.

5. +++ b/core/scripts/run-tests.sh
   @@ -709,7 +709,7 @@ function simpletest_script_command($test_id, $test_class) {
   -  if (strpos($test_class, 'Drupal\\Tests\\') === 0) {
   +  if (is_subclass_of($test_class, '\PHPUnit_Framework_TestCase')) {
   

   +1

6. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -0,0 +1,234 @@
   +class KernelTestBaseTest extends KernelTestBase {
   

   TestTestTest

7. +++ b/core/tests/Drupal/Tests/Core/Render/ElementInfoManagerTest.php
   @@ -136,7 +136,6 @@ public function providerTestGetInfo() {
   -        '#defaults_loaded' => TRUE,
   

   Weird that you remove one, but not the other four instances of that...

8. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1176 @@
   +  protected function enableModules(array $modules) {
   

   I know this is the same name as regular KTB, but it's really installModules()

9. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1176 @@
   +      trigger_error('KernelTestBase::enableModules() should not be called from setUp(). Use the $modules property instead.', E_DEPRECATED);
   

   Nice!

10. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1176 @@
    +   * @deprecated 8.0.x:8.0.0
    

    Ooh what is this syntax?

11. +++ b/core/tests/bootstrap.php
    @@ -49,37 +59,39 @@ function drupal_phpunit_contrib_extension_directory_roots() {
      * Registers the namespace for each extension directory with the autoloader.
      *
    - * @param Composer\Autoload\ClassLoader $loader
    - *   The supplied autoloader.
      * @param array $dirs
      *   An associative array of extension directories, keyed by extension name.
      */
    -function drupal_phpunit_register_extension_dirs(Composer\Autoload\ClassLoader $loader, $dirs) {
    +function drupal_phpunit_get_extension_namespaces($dirs) {
    ...
    +  return $namespaces;
    

    Missing @return docs

Also, shouldn't we put a note on \Drupal\simpletest\KernelTestBase pointing to the new one, and perhaps deprecating it?

Still needs a CR.

Addressing my own review with @dawehner at the DrupalCon LA sprint.

4) Fixed
7) This is a bad change. Without that flag, an element could be run through getInfo() multiple times (for example in FormBuilder and Renderer), and I've restored this behavior.
10) Used standard syntax instead
11) Fixed

Since these are reverts and docs fixes only from me, I feel comfortable RTBC-ing this. @dawehner is writing the CR right now.

Added a change record, tim, you have the honor for the final patch

Quick reroll

1. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   @@ -7,6 +7,8 @@
   +use Symfony\Component\DependencyInjection\ContainerInterface;
   

   Not used

2. +++ b/core/modules/simpletest/src/KernelTestBase.php
   @@ -33,6 +33,9 @@
   + * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.2.x. Use
   + *   \Drupal\Tests\KernelTestBase instead.
   

   Is this discussed anywhere on issue? I think it is an open question which LTS version of Drupal 8 we should have all the Simpletest based kernel test base tests converted. How tricky do we imagine this will be?

3. +++ b/core/modules/simpletest/src/Tests/KernelTestBaseTest.php
   @@ -8,6 +8,7 @@
   +use org\bovigo\vfs\vfsStream;
   

   Not used

4. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1178 @@
   +use Drupal\Component\Utility\String;
   

   Does not exist

5. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1178 @@
   +use Psr\Log\LoggerInterface;
   +use Psr\Log\LoggerTrait;
   +use Psr\Log\LogLevel;
   +use Symfony\Component\DependencyInjection\Dumper\PhpDumper;
   

   None of this is used

6. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1178 @@
   +  protected $runTestInSeparateProcess = TRUE;
   

   Nice - I didn't know this was possible - we should have a followup to do the same for BrowserTestBase. And @dawehner thank you for leaving this in. Regardless of the debate about whether this is correct the fact that run-tests.sh runs every test in a separate process means that at least this is making the PHPUnit runner behave the same way.

7. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1178 @@
   +  /**
   +   * BC alias for register().
   +   *
   +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
   +   *
   +   * @see \Drupal\Tests\KernelTestBase::register()
   +   */
   +  public function containerBuild(ContainerBuilder $container) {
   +  }
   ...
   +  /**
   +   * BC alias for setSetting().
   +   *
   +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
   +   *
   +   * @see \Drupal\Tests\KernelTestBase::setSetting()
   +   */
   +  protected function settingsSet($name, $value) {
   ...
   +  /**
   +   * BC: Automatically resolve former KernelTestBase class properties.
   +   *
   +   * Test authors should follow the provided instructions and adjust their tests
   +   * accordingly.
   +   *
   +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
   +   */
   +  public function __get($name) {
   ...
   +  /**
   +   * BC: Trigger warnings/exceptions for former KernelTestBase class properties.
   +   *
   +   * This is just an extra safety net. Legacy tests are only reading the former
   +   * KernelTestBase properties. Drupal core is not aware of any tests that are
   +   * trying to set or change them.
   +   *
   +   * Test authors should follow the provided instructions and adjust their tests
   +   * accordingly.
   +   *
   +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
   +   */
   +  public function __set($name, $value) {
   

   Is there a followup already existing?

8. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1178 @@
   +  /**
   +   * Converts a list of possible parameters into a stack of permutations.
   +   *
   +   * Takes a list of parameters containing possible values, and converts all of
   +   * them into a list of items containing every possible permutation.
   +   *
   +   * Example:
   +   * @code
   +   * $parameters = array(
   +   *   'one' => array(0, 1),
   +   *   'two' => array(2, 3),
   +   * );
   +   * $permutations = KernelTestBase::generatePermutations($parameters);
   +   * // Result:
   +   * $permutations == array(
   +   *   array('one' => 0, 'two' => 2),
   +   *   array('one' => 1, 'two' => 2),
   +   *   array('one' => 0, 'two' => 3),
   +   *   array('one' => 1, 'two' => 3),
   +   * )
   +   * @endcode
   +   *
   +   * @param array $parameters
   +   *   An associative array of parameters, keyed by parameter name, and whose
   +   *   values are arrays of parameter values.
   +   *
   +   * @return array
   +   *   A list of permutations, which is an array of arrays. Each inner array
   +   *   contains the full list of parameters that have been passed, but with a
   +   *   single value only.
   +   */
   +  public static function generatePermutations(array $parameters) {
   +    $all_permutations = array(array());
   +    foreach ($parameters as $parameter => $values) {
   +      $new_permutations = array();
   +      // Iterate over all values of the parameter.
   +      foreach ($values as $value) {
   +        // Iterate over all existing permutations.
   +        foreach ($all_permutations as $permutation) {
   +          // Add the new parameter value to existing permutations.
   +          $new_permutations[] = $permutation + array($parameter => $value);
   +        }
   +      }
   +      // Replace the old permutations with the new permutations.
   +      $all_permutations = $new_permutations;
   +    }
   +    return $all_permutations;
   +  }
   

   Untested and unused.

Addressing 1, 3, 4, and 5.

About 8: It seems this method is copied verbatim from \Drupal\simpletest\TestBase. I couldn't find a test for the method in that class as well. If I am not wrong, since this class is meant to be an equivalent, I am not entirely sure of removing this method. I guess it still needs tests. I am still switching to Needs review to trigger the testbot. Feel free to switch back.

Thank you @hussainweb for fixing the review!

Thank you @alexpott for reviewing this patch!

Is this discussed anywhere on issue? I think it is an open question which LTS version of Drupal 8 we should have all the Simpletest based kernel test base tests converted. How tricky do we imagine this will be?

Well yeah I don't care about the concrete version, but you can always push it to a higher version in the future, can't you? You threw out that particular version number ...

How tricky do we imagine this will be?

Well, have a look at the earlier versions of the patch, there are some stuff needed but not too much.

Nice - I didn't know this was possible - we should have a followup to do the same for BrowserTestBase. And @dawehner thank you for leaving this in. Regardless of the debate about whether this is correct the fact that run-tests.sh runs every test in a separate process means that at least this is making the PHPUnit runner behave the same way.

Yeah, let's simply tell people how they can speed up their tests locally by factor 3.

Is there a followup already existing?

No :P

About 8: It seems this method is copied verbatim from \Drupal\simpletest\TestBase. I couldn't find a test for the method in that class as well. If I am not wrong, since this class is meant to be an equivalent, I am not entirely sure of removing this method. I guess it still needs tests. I am still switching to Needs review to trigger the testbot. Feel free to switch back.

You could move it to a a common trait. But yeah I don't think that this issue should be responsible for adding test coverage for it.

Nice - I didn't know this was possible - we should have a followup to do the same for BrowserTestBase. And @dawehner thank you for leaving this in. Regardless of the debate about whether this is correct the fact that run-tests.sh runs every test in a separate process means that at least this is making the PHPUnit runner behave the same way.

There we go: #2494735: add $runTestInSeparateProcess = TRUE for BrowserTestBase

Before we do get this in, we should be addressing some of chx' concerns on PHPUnit as a test runner in general - especially regarding debuggability (#2469731-5: Document when to use BrowserTestBase).

Some have been related to how mocks work - which is less important for integration tests.

But one strong point was how assert* works and I agree with that.

Could we have one common base class that both our UnitTestCase and KernelTestBase NG base classes derive from and then create some issues to increase the debuggability of assert*() by providing our own similar functions as overrides?

Apologies if this is already done, have not checked the patch too much, yet.

Could we open an upstream issue if PHPUnit could change to native namespacing (PSR-0 / PSR4). It is currently severely polluting our class map ...

I am just ensuring those arguments are not forgotten before we deprecate the simpletest way.

@hussainweb
Do you want to address the other points according to my comment?

Before we do get this in, we should be addressing some of chx' concerns on PHPUnit as a test runner in general - especially regarding debuggability (#2469731-5: Document when to use BrowserTestBase).

Feel free to open up an actual issue for that. IMHO this does not belong here and can be done independently from it.

Could we open an upstream issue if PHPUnit could change to native namespacing (PSR-0 / PSR4). It is currently severely polluting our class map ...

Regarding that point there seems to be an issue: https://github.com/sebastianbergmann/phpunit/issues/1137 ... It seems to be an issue for PHPUnit 5.0.x

#145: I checked now:

What does belong here is that we don't extend KernelTestBase from \Unit_Test_Case, but from our own base class.

If we can change the "parent" class later and introduce an intermediate class, then no problem, just ensuring it won't block us in the future.

Opened #2495607: Improve PHPUnit debuggability and performance I think we might be able to get quite some performance improvements from this, too :).

What does belong here is that we don't extend KernelTestBase from \Unit_Test_Case, but from our own base class.

Well for sure, bringing in an intermediate subclass is by no means an API change.
Can we please now focus back and get the patch in?

Is there a followup already existing?

Created one: #2496109: Replace all usages to deprecated methods on KernelTestBase(NG)

Untested and unused.

Let's remove it for now and add it back, when we convert the tests

I think the feedback of alexpott got addressed, so back to RTBC?

The only thing that wasn't 100% resolved was the promise of "will be removed before Drupal 8.2.x".
This is our first time referring to 8.[1-9].x in the code base, I think it's a realistic goal.

I don't think saying "will be removed before Drupal 8.y.x" is a fair thing to do :) and no real reason to wait until 9.0.x.

1. +++ b/core/includes/install.inc
   @@ -211,7 +211,7 @@ function drupal_rewrite_settings($settings = array(), $settings_file = NULL) {
   -  $contents = file_get_contents(DRUPAL_ROOT . '/' . $settings_file);
   +  $contents = file_get_contents($settings_file);
   
   @@ -316,7 +316,7 @@ function drupal_rewrite_settings($settings = array(), $settings_file = NULL) {
   -    if (file_put_contents(DRUPAL_ROOT . '/' . $settings_file, $buffer) === FALSE) {
   +    if (file_put_contents($settings_file, $buffer) === FALSE) {
   
   +++ b/core/lib/Drupal/Core/PhpStorage/PhpStorageFactory.php
   @@ -52,7 +52,7 @@ static function get($name) {
   -      $configuration['directory'] = DRUPAL_ROOT . '/' . PublicStream::basePath() . '/php';
   +      $configuration['directory'] = PublicStream::basePath() . '/php';
   

   Bye bye DRUPAL_ROOT <3

2. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -200,19 +200,23 @@ public function decode($raw) {
   -    // \GlobIterator on Windows requires an absolute path.
   -    $files = new \GlobIterator(realpath($dir) . '/' . $prefix . '*' . $extension);
   

   There are still 3 usages of glob iterator in core.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php
   @@ -49,9 +49,10 @@ public function __sleep() {
   -    $container = \Drupal::getContainer();
   -    foreach ($this->_serviceIds as $key => $service_id) {
   -      $this->$key = $container->get($service_id);
   +    if ($container = \Drupal::getContainer()) {
   +      foreach ($this->_serviceIds as $key => $service_id) {
   +        $this->$key = $container->get($service_id);
   +      }
        }
   

   This change looks extremely dodgy and dangerous change.

4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -467,6 +471,14 @@ public function getContainer() {
   +  public function setContainer(ContainerInterface $container = NULL) {
   +    $this->container = $container;
   +    return $this;
   +  }
   

   I think we should prevent changing the container once it is set no or the kernel is booted?

5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -559,7 +571,7 @@ public function discoverServiceProviders() {
   -        if (class_exists($class)) {
   +        if ((is_object($class) && ($class instanceof ServiceProviderInterface || $class instanceof ServiceModifierInterface)) || class_exists($class)) {
   

   Lets optimise for the regular class_exists($class) code path by putting it first.

6. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -127,6 +127,14 @@ protected function getLocalPath($uri = NULL) {
   +    // a proxy. realpath() is (obviously) not supported by vfsStream.
   

   (obviously) can be removed. If it so obvious why comment? :)

7. +++ b/core/modules/simpletest/src/RandomGeneratorTrait.php
   @@ -0,0 +1,120 @@
   +trait RandomGeneratorTrait {
   

   Yay! Can we get a followup to use this in BrowserTestBase please?

8. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -29,6 +29,9 @@
      use SessionTestTrait;
    
   +
   

   Lots and lots of space! Also you've implemented the Trait but not removed the methods - weird.

9. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
   @@ -21,10 +21,13 @@ class ModuleHandlerTest extends KernelTestBase {
   -  public static $modules = array('system');
   ...
   +    // @todo ModuleInstaller calls system_rebuild_module_data which is part of
   +    //   system.module, see https://www.drupal.org/node/2208429.
   +    include_once $this->root . '/core/modules/system/system.module';
   

   Why can't we still just enable the system module?

10. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
    @@ -0,0 +1,234 @@
    +  /**
    +   * @covers ::getCompiledContainerBuilder
    +   */
    +  public function testCompiledContainer() {
    +    $this->enableModules(['system', 'user']);
    +    $this->installConfig('user');
    +  }
    +
    +  /**
    +   * @covers ::getCompiledContainerBuilder
    +   * @depends testCompiledContainer
    +   */
    +  public function testCompiledContainerIsDestructed() {
    +    $this->enableModules(['system', 'user']);
    +    $this->installConfig('user');
    +  }
    

    Tests with no assertions fail with strict PHPUnit testing

11. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1124 @@
    + * @todo Extend ::setRequirementsFromAnnotation() and ::checkRequirements() to
    + *   account for '@requires module'.
    

    d.o issue?

12. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1124 @@
    +    // Change the current dir to DRUPAL_ROOT.
    +    chdir(__DIR__ . '/../../../../');
    ...
    +    $this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));
    

    Is this two strategies for the same thing?

13. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1124 @@
    +    // Uses a random ID, since created test files may be processed by file
    +    // discovery/parser services that are using a static cache to avoid parsing
    +    // the identical files multiple times.
    +    $suffix = mt_rand(100000, 999999);
    ...
    +    $this->databasePrefix = 'simpletest' . $suffix;
    +    drupal_valid_test_ua($this->databasePrefix);
    

    Why is this necessary - and clashes will happen so what's will go wrong? In TestBaseTest we ensure that this is unique.

14. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1124 @@
    +  /**
    +   * Parses a database URL into a connection info array.
    +   *
    +   * @param string $db_url
    +   *   The database URL.
    +   *
    +   * @return array
    +   *   The database connection info.
    +   */
    +  protected function parseDbUrl($db_url) {
    +    $info = parse_url($db_url);
    +    if (!isset($info['scheme'], $info['host'], $info['path'])) {
    +      throw new \InvalidArgumentException('Invalid dburl, . Minimum requirement: driver://host/database');
    +    }
    +    $info += [
    +      'user' => '',
    +      'pass' => '',
    +      'fragment' => '',
    +    ];
    +    if ($info['path'][0] === '/') {
    +      $info['path'] = substr($info['path'], 1);
    +    }
    +    if ($info['scheme'] === 'sqlite' && $info['path'][0] !== '/') {
    +      $info['path'] = $this->root . '/' . $info['path'];
    +    }
    +    $db_info = [
    +      'driver' => $info['scheme'],
    +      'username' => $info['user'],
    +      'password' => $info['pass'],
    +      'host' => $info['host'],
    +      'database' => $info['path'],
    +      'prefix' => [
    +        'default' => $info['fragment'],
    +      ],
    +    ];
    +    if (isset($info['port'])) {
    +      $db_info['port'] = $info['port'];
    +    }
    +    return $db_info;
    +  }
    

    This feels like this is duplicated code. Yep we can use Database::convertDbUrlToConnectionInfo() - no? Ah it looks like that is already being used - so this can be removed :)

15. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1124 @@
    +  /**
    +   * Returns the Database connection info to be used for this test.
    +   *
    +   * This method only exists for tests of the Database component itself, because
    +   * they require multiple database connections. Each SQLite :memory: connection
    +   * creates a new/separate database in memory. A shared-memory SQLite file URI
    +   * triggers PHP open_basedir/allow_url_fopen/allow_url_include restrictions.
    +   * Due to that, Database tests are running against a SQLite database that is
    +   * located in an actual file in the system's temporary directory.
    +   *
    +   * Other tests should not override this method.
    +   *
    +   * @return array
    +   *   A Database connection info array.
    +   *
    +   * @internal
    +   */
    +  protected function getDatabaseConnectionInfo() {
    +    // If the test is run with argument dburl then use it.
    +    $db_url = getenv('SIMPLETEST_DB');
    +    if (!empty($db_url)) {
    +      $database = Database::convertDbUrlToConnectionInfo($db_url, $this->root);
    +      Database::addConnectionInfo('default', 'default', $database);
    +    }
    +
    +    // Clone the current connection and replace the current prefix.
    +    $connection_info = Database::getConnectionInfo('default');
    +    if (is_null($connection_info)) {
    +      throw new \InvalidArgumentException('There is no database connection so no tests can be run. You must provide a SIMPLETEST_DB environment variable, like "sqlite://localhost//tmp/test.sqlite", to run PHPUnit based functional tests outside of run-tests.sh.');
    +    }
    +    else {
    +      Database::renameConnection('default', 'simpletest_original_default');
    +      foreach ($connection_info as $target => $value) {
    +        // Replace the full table prefix definition to ensure that no table
    +        // prefixes of the test runner leak into the test.
    +        $connection_info[$target]['prefix'] = array(
    +          'default' => $value['prefix']['default'] . $this->databasePrefix,
    +        );
    +      }
    +    }
    +    return $connection_info;
    +  }
    

    This code already exists in BrowserTestBase. Maybe KernelTestBase and BrowserTestBase should share a common base class... what with the random stuff there seems to be a case for this.

16. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1124 @@
    +  /**
    +   * BC alias for register().
    +   *
    +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
    +   *
    +   * @see \Drupal\Tests\KernelTestBase::register()
    +   */
    +  public function containerBuild(ContainerBuilder $container) {
    +  }
    

    Shall we trigger a warning ala Symfony to help users migrate?

17. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1124 @@
    +  /**
    +   * Returns the modules to enable for this test.
    +   *
    +   * @param string $class
    +   *   The fully-qualified class name of this test.
    +   *
    +   * @return array
    +   */
    +  private static function getModulesToEnable($class) {
    +    $modules = array();
    +    while ($class) {
    +      if (property_exists($class, 'modules')) {
    +        // Only add the modules, if the $modules property was not inherited.
    +        $rp = new \ReflectionProperty($class, 'modules');
    +        if ($rp->class == $class) {
    +          $modules[$class] = $class::$modules;
    +        }
    +      }
    +      $class = get_parent_class($class);
    +    }
    +    // Modules have been collected in reverse class hierarchy order; modules
    +    // defined by base classes should be sorted first. Then, merge the results
    +    // together.
    +    $modules = array_reverse($modules);
    +    return call_user_func_array('array_merge_recursive', $modules);
    +  }
    

    Something else that could be shared with BrowserTestBase - it does not currently have this functionality.

18. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1124 @@
    +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
    +   */
    +  public function __get($name) {
    ...
    +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
    +   */
    +  public function __set($name, $value) {
    

    Hmmm really? Should we be supporting these - and if so until when if it is 8.0.0 then well they should never exist.

19. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
    @@ -0,0 +1,1124 @@
    +      // @comment it in again.
    +      // $this->triggerDeprecated(sprintf("KernelTestBase::\$%s no longer exists. Use the regular API method to retrieve it instead (e.g., Settings).", $name));
    

    Let's do this then.

#152.4: Agree, if we need a new container in the kernel, we should boot a new kernel instead and use that, IMHO.

There are still 3 usages of glob iterator in core.

Removed the other instances.

This change looks extremely dodgy and dangerous change.

Let's revert that and see how much breaks.

Lets optimise for the regular class_exists($class) code path by putting it first.

Sure, let's do that.

(obviously) can be removed. If it so obvious why comment? :)

Totally fair point. I hate when people use straightforward, obviously, and what not.

+++ b/core/modules/simpletest/src/RandomGeneratorTrait.php
@@ -0,0 +1,120 @@
+trait RandomGeneratorTrait {
Yay! Can we get a followup to use this in BrowserTestBase please?

There we go: #2499199: Use RandomGeneratorTrait in BrowserTestBase

Lots and lots of space! Also you've implemented the Trait but not removed the methods - weird.

Probably caused by some random merge misstake ...

Is this two strategies for the same thing?

Good point, let's move that into a protected static method.

This feels like this is duplicated code. Yep we can use Database::convertDbUrlToConnectionInfo() - no? Ah it looks like that is already being used - so this can be removed :)

Oh yeah that API function was added rather recently.

This code already exists in BrowserTestBase. Maybe KernelTestBase and BrowserTestBase should share a common base class... what with the random stuff there seems to be a case for this.

Let's try to not introduce too much hierarchy but instead use a Trait as well ...

This code already exists in BrowserTestBase. Maybe KernelTestBase and BrowserTestBase should share a common base class... what with the random stuff there seems to be a case for this.

Decided to go with a follow up for that. For example DRUPAL_ROOT vs. $this->root etc. ... #2499207: Unify BrowserTestBase::changeDatabasePrefix() and KernelTestbase::getDatabaseConnectionInfo() into a DataBaseTestTrait

Shall we trigger a warning ala Symfony to help users migrate?

+1

Something else that could be shared with BrowserTestBase - it does not currently have this functionality.

Hmmm really? Should we be supporting these - and if so until when if it is 8.0.0 then well they should never exist.

Well, see it as BC layer as well ... i think this will make it easier to convert the existing tests ... :)

Let's fix it.

Back to green.

+++ b/core/lib/Drupal/Core/Config/InstallStorage.php
@@ -197,10 +197,17 @@ public function getComponentNames(array $list) {
+        // glob() directly calls into libc glob(), which is not aware of PHP stream
+        // wrappers. Same for \GlobIterator (which additionally requires an absolute

@@ -217,10 +224,17 @@ public function getCoreNames() {
+      // glob() directly calls into libc glob(), which is not aware of PHP stream
+      // wrappers. Same for \GlobIterator (which additionally requires an absolute

Nits more then 80 chars.

There we go.

All is well at https://github.com/jibran/drupal/pull/1. Found #2454439-112: [META] Support PHP 7 while testing ;-).
Let's get @alexpott's feedback on this.

1. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1087 @@
   +    // @todo Remove this BC layer.
   +    $defined_class = new \ReflectionMethod($this, 'containerBuild');
   +    $defined_class = $defined_class->getDeclaringClass()->name;
   +
   +    if (__CLASS__ !== $defined_class) {
   +      $this->containerBuild($container);
   +      trigger_error('Use ::register() instead', E_USER_DEPRECATED);
   +    }
   ...
   +  /**
   +   * BC alias for register().
   +   *
   +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
   +   *
   +   * @see \Drupal\Tests\KernelTestBase::register()
   +   */
   +  public function containerBuild(ContainerBuilder $container) {
   +  }
   

   Is this BC layer actually worth it? We're saying that we're going to remove before 8.0.0 but nothing is going to use it by then...

2. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1087 @@
   +  /**
   +   * BC alias for setSetting().
   +   *
   +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
   +   *
   +   * @see \Drupal\Tests\KernelTestBase::setSetting()
   +   */
   +  protected function settingsSet($name, $value) {
   +    trigger_error(sprintf("KernelTestBase::%s() is deprecated. Use setSetting() instead.", __FUNCTION__), E_DEPRECATED);
   +    $this->setSetting($name, $value);
   +  }
   

   Same for this...

3. +++ b/core/tests/Drupal/Tests/KernelTestBase.php
   @@ -0,0 +1,1087 @@
   +  /**
   +   * BC: Automatically resolve former KernelTestBase class properties.
   +   *
   +   * Test authors should follow the provided instructions and adjust their tests
   +   * accordingly.
   +   *
   +   * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0.
   +   */
   

   And this... perhaps it makes sense to leave them in till 9.0 - i dunno. The only thing i do know is that adding it now and removing before 8.0.0 makes no sense.

Is this BC layer actually worth it? We're saying that we're going to remove before 8.0.0 but nothing is going to use it by then...

Well, it makes it easier to convert existing tests because we throw some helpful information ... What about not promising anything via @deprecated and always throw an \Exception and then remove it at some point before 8.0.x?

I think removing this all together is a good idea we can update the change record to mention these changes.

Removed the BC layers, as alex requested ...

And this... perhaps it makes sense to leave them in till 9.0 - i dunno. The only thing i do know is that adding it now and removing before 8.0.0 makes no sense.

Went with 8.2.x to be more in parallel with the old KernelTestBase

I think removing this all together is a good idea we can update the change record to mention these changes.

Adapted the PR

Also fixed the failing test ... thank you @jibran for the resending :)

Thanks for the fixes patch looks good now.

Just some ordinary rerolling.

git ac https://www.drupal.org/files/issues/2304461-174.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 85243  100 85243    0     0  62453      0  0:00:01  0:00:01 --:--:-- 64578
error: patch failed: core/modules/system/src/Tests/PhpStorage/PhpStorageFactoryTest.php:5
error: core/modules/system/src/Tests/PhpStorage/PhpStorageFactoryTest.php: patch does not apply