Convert system module's kernel tests to NG

Note, I'd recommend to consider moving any where we do not actually test system *module* stuff but components to move out of the module and into core/tests/....

I've done the same with the config component tests that are currently in the config module.

Yeah that one is super weird.

Maybe we can make public:// work, see #31 in #2684141: Convert migrate kernel tests to KernelTestBaseTNG, not sure if the same applies here, though.

1. +++ b/core/modules/system/tests/src/Kernel/File/DirectoryTest.php
   @@ -5,7 +5,7 @@
    
   -namespace Drupal\system\Tests\File;
   +namespace Drupal\Tests\system\Kernel\File;
   

   I think most of those tests are about testing FileSystem, which is in the core namespace or functions in include files.

   IMHO, those would be OK to move as well. entity tests for example also rely on system (and test modules in system.module).

2. +++ b/core/modules/system/tests/src/Kernel/File/DirectoryTest.php
   @@ -82,7 +82,7 @@ function testFileCheckDirectoryHandling() {
          $this->assertFalse(file_prepare_directory($directory, 0), 'Error reported for a non-writeable directory.', 'File');
    
          // Test directory permission modification.
   -      $this->settingsSet('file_chmod_directory', 0777);
   +      $this->setSetting('file_chmod_directory', 0777);
          $this->assertTrue(file_prepare_directory($directory, FILE_MODIFY_PERMISSIONS), 'No error reported when making directory writeable.', 'File');
   

   the new assertTrue() only has two arguments, the group concept is gone. Not sure if we want to address that here since it doesn't really hurt and I fear that there are a lot of calls like that. Back then, we used that group thing quite a lot.

   In fact, we even still have asserts with t() calls in here.

3. +++ b/core/modules/system/tests/src/Kernel/File/StreamWrapperTest.php
   @@ -41,14 +41,12 @@ class StreamWrapperTest extends FileTestBase {
    
      function setUp() {
   +    parent::setUp();
   +
        // Add file_private_path setting.
   -    $settings = Settings::getAll();
        $request = Request::create('/');;
        $site_path = DrupalKernel::findSitePath($request);
   -    $settings['file_private_path'] = $site_path . '/private';
   -    new Settings($settings + Settings::getAll());
   -
   -    parent::setUp();
   +    $this->setSetting('file_private_path', $site_path . '/private');
      }
   

   Hm, we might have done that deliberately before setUp(), not sure.

4. +++ b/core/modules/system/tests/src/Kernel/KeyValueStore/DatabaseStorageExpirableTest.php
   @@ -5,10 +5,11 @@
   -namespace Drupal\system\Tests\KeyValueStore;
   +namespace Drupal\Tests\system\Kernel\KeyValueStore;
    
    use Drupal\Core\DependencyInjection\ContainerBuilder;
    use Drupal\Core\KeyValueStore\KeyValueFactory;
   +use Drupal\KernelTests\Core\KeyValueStore\StorageTestBase;
   

   Why only move some of those classes?

   Note that this one overrides containerBuild(), which is now called just register(). Not sure why it's not failing, maybe it's not required and we can drop it? Or we might not actually be testing what we think we are, since it defaults to memory?

5. +++ b/core/modules/system/tests/src/Kernel/Routing/RouteProviderTest.php
   @@ -91,8 +91,8 @@ protected function setUp() {
       */
   -  public function containerBuild(ContainerBuilder $container) {
   -    parent::containerBuild($container); // TODO: Change the autogenerated stub
   +  public function register(ContainerBuilder $container) {
   +    parent::register($container); // TODO: Change the autogenerated stub
   

   lets drop that bogus todo while we touch that line.

6. +++ b/core/modules/system/tests/src/Kernel/Routing/RouteProviderTest.php
   @@ -214,7 +215,8 @@ function testOutlinePathMatchTrailingSlash() {
        foreach ($routes as $route) {
   -      $this->assertEqual($route->compile()->getPatternOutline(), '/path/%/one', 'Found path has correct pattern');
   +      $this->assertEqual($route->compile()
   +        ->getPatternOutline(), '/path/%/one', 'Found path has correct pattern');
   

   looks like the typical automatic phpstorm changes, which seem unrelated and just make the patch bigger.

Let's see whether this fixes all of the failures ...

In general I think the tests which should be kept in system should be just the ones which test functionality shipped by system module directly, like a custom controller or service. All the other ones belong totally

There we go.

+++ b/core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php
@@ -249,13 +251,13 @@ function testOutlinePathMatchDefaults() {
-    }
-    catch (ResourceNotFoundException $e) {
+    } catch (ResourceNotFoundException $e) {

unrelated change? all changes in this file are not needed? coding standards: catch must be oin a new line.

Otherwise looks good.

Thanks for the review klausi!

No comment

Here are two files I forgot to move.

#2665992: @file is not required for classes, interfaces and traits was committed and breaks this like hell.

It does. But git is awesome, try "git merge -X theirs 8.1.x". Worked fine for me in #2456477: Convert deprecated \Drupal\simpletest\KernelTestBase tests to KernelTestBaseNG.

Can do it, but not sure how much @dawehner already did on this since the last patch.

That was a bit too optimistic, you still have to fix the namespace declarations then.

This was not at all a painful.

git merge -X theirs 8.1.x

You actually run git merge -X ours and then run the fixer again.

Found one remaining one: \Drupal\system\Tests\Cache\CacheContextOptimizationTest.

\Drupal\system\Tests\Cache\GenericCacheBackendUnitTestBase should get a @deprecated note.

I also found a left-over in views and some that weren't properly updated, will take care of that in #2456477: Convert deprecated \Drupal\simpletest\KernelTestBase tests to KernelTestBaseNG.

Thank you berdir!

Found an issue: #2712419: ModuleInstaller::install() doesn't order the installed modules ... here are some additional bugfixes

Talked with alex, and we discussed to split off the problematic patches.

+++ b/core/tests/Drupal/KernelTests/Core/Routing/ContentNegotiationRoutingTest.php
@@ -17,23 +17,13 @@ class ContentNegotiationRoutingTest extends KernelTestBase {
+  public function register(ContainerBuilder $container) {
+    parent::register($container);
 
     // \Drupal\simpletest\KernelTestBase::containerBuild() removes the alias path
     // processor.

Comment does not match the new function name register().

Otherwise looks good to me.

Thank you klausi!

Let's get this in.

+++ b/core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php
@@ -202,7 +202,7 @@ function testSettings() {
     $endToken = '}';
     $start = strpos($rendered_js, $startToken);
     $end = strrpos($rendered_js, $endToken);
-    $json  = Unicode::substr($rendered_js, $start, $end - $start + 1);
+    $json  = Unicode::substr((string) $rendered_js, $start, $end - $start + 1);

Why is the cast necessary when it's not for the strpos/strrpos calls above? If it really is necessary needs a comment.

Well, I understand it kinda, but not completely.\Drupal\Component\Utility\Unicode::substr breaks completely without mb disabled when you pass in a Markup object, see $c = ord($text[$bytes]);

Just a random test failure ...

Haven't reviewed yet but just so it is not forgotten there is one thing to fix on commit...

FILE: ...rupal/KernelTests/Core/Cache/GenericCacheBackendUnitTestBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 92 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
    |       |     "NULL" but found "null"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY

Fixed #52.

Thank you jibran!

#49 OK but why not cast before the strpos() calls. The added comment helps but it doesn't explain why it's necessary here and not later.

This is what I meant.

Totally works for me. Thank you!

Added a couple of followups: #2731253: Call \Drupal\Component\Utility\Unicode::check in kernel tests #2731253: Call \Drupal\Component\Utility\Unicode::check in kernel tests

My change was tiny so I don't think it disqualifies me from committing this to 8.2.x - done that now.

Is it time to remove \Drupal\simpletest\KernelTestBase now?

No, there are still other base classes using it, that we need to convert, for example PluginBaseTest in Views.

$ ag simpletest\\\\KernelTestBase
modules/views/src/Tests/Plugin/PluginBaseTest.php
12:use Drupal\simpletest\KernelTestBase;

modules/views/src/Tests/ViewKernelTestBase.php
6:use Drupal\simpletest\KernelTestBase;

modules/system/src/Tests/DrupalKernel/DrupalKernelTest.php
7:use Drupal\simpletest\KernelTestBase;

modules/system/src/Tests/Entity/EntityUnitTestBase.php
5:use Drupal\simpletest\KernelTestBase;

modules/system/src/Tests/Cache/GenericCacheBackendUnitTestBase.php
7:use Drupal\simpletest\KernelTestBase;

modules/simpletest/src/TestBase.php
24: * \Drupal\simpletest\WebTestBase or \Drupal\simpletest\KernelTestBase.

modules/simpletest/src/Tests/KernelTestBaseTest.php
7:use Drupal\simpletest\KernelTestBase;

tests/Drupal/KernelTests/KernelTestBase.php
1194:      // @see \Drupal\simpletest\KernelTestBase::containerBuild()

All of those are unused base classes that we kept on purpose until we remove Drupal\simpletest\KernelTestBase. And the old KernelTestBase test class. So yes, we can open an issue to remove it now.

Ah cool, PluginBaseTest in Views is not deprecated, that's why I thought it is still used. Let's also @deprecate all of those unused base classes then.

oh, PluginBaseTest is not even a test base class, it is still just an old kernel test. So no, we cannot remove Drupal\simpletest\KernelTestBase because not all kernel tests have been converted.

Ah, looks like we missed that one, I think we can convert that in the same issue ,should be simple enough?

Ok, opened #2734663: Update deprecation message for old KernelTestBase in simpletest.