Convert deprecated \Drupal\simpletest\KernelTestBase tests to KernelTestBaseNG

Can we have a change notice for that so that I can fix my contrib modules?

Just learned how to write tables in HTML.

That's a very good skill to have! Wanna hear a story about divs? :D

In this issue?

Either in this issue after the fix or we can have the steps to convert the tests from KernelTestBase to KernelTestBaseNG.

Should we have a child issue per module?

Just making notes - might as well do it in public.

There are classes of tests which extend KernelTestBase indirectly.

Practically speaking what this means is that when converting and testing it looks like all members of the subset are going to be done in one batch.

For example all the tests that extend core/modules/field/src/Tests/FieldUnitTestBase. -- which has 34 child tests

Anyways here is a list of 17 base classes in core.

grep -Rl 'TestBase extends KernelTestBase' *

core/modules/config/src/Tests/Storage/ConfigStorageTestBase.php ... 3 Child tests
core/modules/field/src/Tests/FieldUnitTestBase.php ... 34 Child tests
core/modules/file/src/Tests/FileManagedUnitTestBase.php
core/modules/hal/src/Tests/NormalizerTestBase.php
core/modules/language/src/Tests/LanguageTestBase.php
core/modules/migrate/src/Tests/MigrateTestBase.php
core/modules/quickedit/src/Tests/QuickEditTestBase.php
core/modules/serialization/src/Tests/NormalizerTestBase.php
core/modules/system/src/Tests/Cache/GenericCacheBackendUnitTestBase.php
core/modules/system/src/Tests/Database/DatabaseTestBase.php
core/modules/system/src/Tests/Entity/EntityUnitTestBase.php
core/modules/system/src/Tests/File/FileTestBase.php
core/modules/system/src/Tests/KeyValueStore/StorageTestBase.php
core/modules/system/src/Tests/Path/PathUnitTestBase.php
core/modules/system/src/Tests/Plugin/Discovery/DiscoveryTestBase.php
core/modules/system/src/Tests/Plugin/PluginTestBase.php ...20 child tests
core/modules/views/src/Tests/ViewUnitTestBase.php ... 65 child tests

I think I have found a bug ....

when I started to convert the first of the class ConfigStorageTestBase ... and its 3 sub issues.

So I have just created a quick fix side issue - because I think it will be a blocker to many efforts here.

Is this really something that we should do in 8.0.x (or more specific, before 8.0.0). Not necessarily against it but it seems like something that we should clarify?

Also don't see why this needs a change record, we already have those afaik?

We need to fix #2553533: KernelTestBaseTNG™ is not cleaning up after itself

I want to answer #11 in a constructive way

My perspective is that we indirectly help contrib modules by advancing this issue.

I am thinking of "sock" like modules - dull uninspiring things that prove essential in life.

They have the following features

1) Provide little fields, widgets or whatever plugin that glues a site together.
2) Many module listed in their dependency definition

The new Kernel is going to be really useful in setting up test fixtures for these things.

PS I have started a little experimental issue to try and expose things we need to know early.

@martin107 the CR for the original issue clearly states:

Note: Until Drupal 8.0.x just straight conversions are allowed to land in core itself. What are straight conversions. Straight conversions are conversions
that just change the base class

The issue was only committed under that provision. Please do not make me regret that decision. It is great that you are experimenting with the new test infra and please continue but large scale conversion at this point is off the table.

@alexpott

Please do not make me regret that decision.

No worries - I will take my foot off the accelerator :)

Am I mistaken, or doesn't switching the base class from the old to the new KernelTestBase also necessitate moving the class to a different directory and namespace (namely from src/Tests to tests/src)?
In that case, a straight move of any of the test base classes would be pretty bad for contrib, since modules would then have to choose exactly which version of Drupal they (or, at least, their test suite) should be compatible with. If this is really the case, I guess there should, for a time at least, be two base classes, one based off the old and one based off the new KernelTestBase class.

Otherwise, what is the plan for contrib modules to migrate their tests? Should they just not use any of those base classes and start migrating to using the new KernelTestBase directly?

(Also, please apologize if I have misunderstood the concept of the different Core minor versions – still not sure how they fit in with contrib compatibility.)

Created child issues for the CKEditor & Text Editor kernel tests: #2678532: [PP-1] Convert ckeditor.module's kernel tests to NG + #2678534: Convert editor.module's kernel tests to NG.

Do you want to have links for every single test, or do you want to reformat the table to have one row per module, for those tests that have per-module issues created for them?

+1.

Done.

I converted the rest of the things to that same pattern. Any suggestions on system module? At 150+ test classes, is that too much?

Also, it would be good to hear from a committer if breaking things up by module is what they want. In the past, they've had feedback that too many little patches makes things harder for them. Although in this case, there's enough going on that a per module approach seems rational.

OK, taking a different direction here. We're going to try to do this all in a single patch. If, and only if, things get a little harry using that approach, we can split out certain tests and modules as appropriate.

There are other base classes too. For example, one of the most common ones is EntityUnitTestBase. There are 50 subclasses of that, in all kinds of modules.

We could either update them all together, but actually, especially considering the weird/wrong name there (UnitTestBase), we could also add a new EntityKernelTestBase, deprecate the old one and then gradually move over classes to the new one.

I don't think #26 is a good idea. I think doing it all in one single issue would be a huge patch, i wouldn't want to review that. It's not that trivial, you have to check that the classes are in the right namespace/folder for example, other they might be silently skipped by the testbot.

re #30: the idea in #26 was from the core committers. We can compare numbers of run tests before and after the patch to make sure we aren't silently missing anything. Let's try doing a massive patch first. If that just gets too hard, too quick, then we can try cutting it up a little.

Well, that is going to be painful. Here's as far as I got. It is completed through the config module. 24 down, ~165 left.

To convert using an IDE, look for usages of 'Drupal\simpletest\KernelTestBase', then start shuffling files around and moving namespaces, etc.

Make sure you set up git diff correctly so it tracks those changes as moves. otherwise that is impossible to review.

I also doubt the count idea will work, AFAIK, simpletest and phpunit tests are counted differently..

Note that I started with #2679096: Convert Kernel Tests in Drupal\system\Tests\Entity to phpunit and that alone is quickly a non-trivial thing with unexpected test fails that requires changes outside of tests.

If you start at the beginning then you won't clash with my efforts there for a while.

Starting at the beginning. And here's the better formatted diff.

A few updates. I had opened #2674474: Remove usages of @deprecated Drupal\simpletest\KernelTestBase but it's a duplicate of this issue.

I linked the existing change record to this issue.

I might pick this up when the following issues are in:

#2553655: Convert ViewKernelTestBase to use KernelTestBaseTNG
#2679096: Convert Kernel Tests in Drupal\system\Tests\Entity to phpunit
#2680105: Convert database kernel tests to phpunit

All convert a sizeable chunk of tests already and the first two also fix various problems in KernelTestBase that might cause the tests above to fail. Once those are in, it might be viable to convert the remaining ones together, at least those that aren't causing troubles.

+++ b/core/modules/config/tests/src/Kernel/ConfigEntityStaticCacheTest.php
@@ -2,13 +2,13 @@
  */
 
-namespace Drupal\config\Tests;
+namespace Drupal\Tests\config\Kernel;
 

the config module is only the UI and we only had them in there so we didn't have to stuff them into system.module like many others.

Now that we can, i think we should move them to core/tests/Drupal/KernelTests/Config.

Just a re-roll of #36 for 8.2.x-dev as it was not applying anymore.

Update to run the test.

This fixes the config tests for me.

More tests converted and fixed.

1. +++ b/core/modules/aggregator/tests/src/Kernel/AggregatorTitleTest.php
   @@ -74,7 +76,7 @@ public function testStringFormatter() {
        $build = $aggregator_feed->{$this->fieldName}->view(['type' => 'aggregator_title', 'settings' => ['display_as_link' => FALSE]]);
        $result = $this->render($build);
   -    $this->assertTrue(strpos($result, 'testing title') === 0);
   +    $this->assertTrue(strpos($result, 'testing title') !== FALSE);
   

   render() actually returns a full HTML page now in the base test class. I don't know what the old returned if this string was at the very beginning?

2. +++ b/core/modules/block/tests/src/Kernel/BlockStorageUnitTest.php
   @@ -26,7 +26,7 @@ class BlockStorageUnitTest extends KernelTestBase {
       */
   -  public static $modules = array('block', 'block_test');
   +  public static $modules = array('block', 'block_test', 'system');
    
   

   this used system.module functions which were always loaded before.

3. +++ b/core/modules/config/tests/src/Kernel/ConfigFileContentTest.php
   @@ -121,16 +121,16 @@ function testReadWriteConfig() {
    
        // Read false value.
   -    $this->assertEqual($config->get($false_key), '0', "Boolean FALSE value returned the string '0'.");
   +    $this->assertSame($config->get($false_key), FALSE, "Boolean FALSE value returned the FALSE.");
    
        // Read true value.
   -    $this->assertEqual($config->get($true_key), '1', "Boolean TRUE value returned the string '1'.");
   +    $this->assertSame($config->get($true_key), TRUE, "Boolean TRUE value returned the TRUE.");
    
        // Read null value.
        $this->assertIdentical($config->get('null'), NULL);
    
        // Read false that had been nested in an array value.
   -    $this->assertEqual($config->get($casting_array_false_value_key), '0', "Nested boolean FALSE value returned the string '0'.");
   +    $this->assertSame($config->get($casting_array_false_value_key), FALSE, "Nested boolean FALSE value returned FALSE.");
    
        // Unset a top level value.
        $config->clear($key);
   

   I think this hasn't been TRUE (see what I did there?) for a very long time, but our non-type safe assertions didn't fail. PhpUnit has some special cases where assertEquals() actually isn't a == check. Made it explicit now and fixed the test.

4. +++ b/core/modules/config/tests/src/Kernel/ConfigImporterTest.php
   @@ -677,8 +677,9 @@ public function testInstallProfile() {
      public function testConfigGetConfigDirectory() {
   +    global $config_directories;
        $directory = config_get_config_directory(CONFIG_SYNC_DIRECTORY);
   -    $this->assertEqual($this->configDirectories[CONFIG_SYNC_DIRECTORY], $directory);
   +    $this->assertEqual($config_directories[CONFIG_SYNC_DIRECTORY], $directory);
   

   not exactly sure what to do with this. This tests the function, so I think using a global is fine as that function uses one?

5. +++ b/core/modules/config/tests/src/Kernel/ConfigSchemaTest.php
   @@ -388,7 +388,7 @@ public function testConfigSaveWithSchema() {
        // changed when saved. The 'config_schema_test.ignore' will have been saved
        // during the installation of configuration in the setUp method.
   -    $extension_path = __DIR__ . '/../../tests/config_schema_test/';
   +    $extension_path = __DIR__ . '/../../config_schema_test/';
        $install_storage = new FileStorage($extension_path . InstallStorage::CONFIG_INSTALL_DIRECTORY);
   

   file location changed.

6. +++ b/core/modules/config/tests/src/Kernel/DefaultConfigTest.php
   @@ -23,15 +24,6 @@ class DefaultConfigTest extends KernelTestBase {
   -   * Modules to enable.
   -   *
   -   * Enable the system module so that system_config_schema_info_alter() fires.
   -   *
   -   * @var array
   -   */
   -  public static $modules = array('system', 'config_test');
   

   this function was removed 1 year, 3 month ago in #2235901: Remove custom theme settings from *.info.yml

7. +++ b/core/modules/config/tests/src/Kernel/DefaultConfigTest.php
   @@ -50,8 +42,8 @@ protected function setUp() {
       */
   -  public function containerBuild(ContainerBuilder $container) {
   -    parent::containerBuild($container);
   +  public function register(ContainerBuilder $container) {
   +    parent::register($container);
   

   method was renamed.

8. +++ b/core/modules/config/tests/src/Kernel/Storage/CachedStorageTest.php
   @@ -36,8 +38,7 @@ class CachedStorageTest extends ConfigStorageTestBase {
        // Create a directory.
   -    $dir = $this->publicFilesDirectory . '/config';
   -    mkdir($dir);
   +    $dir = PublicStream::basePath() . '/config';
   

   $this->publicFilesDirectory no longer exists and the folder already exists.

9. +++ b/core/modules/config/tests/src/Kernel/Storage/FileStorageTest.php
   @@ -75,12 +75,6 @@ public function testlistAll() {
   -
   -    // Initialize FileStorage with absolute file path.
   -    $absolute_path = realpath($this->directory);
   -    $storage_absolute_path = new FileStorage($absolute_path);
   -    $config_files = $storage_absolute_path->listAll();
   -    $this->assertIdentical($config_files, $expected_files, 'Absolute path, two config files found.');
   

   vfs:// has no realpath), so we can't test this like this.

   This was added in #2170117: CMI FileStorage fails with absolute paths but the fix there was actually incorrect and implemented in a different way. The old code could never have worked with vfs in the first place. So it kind of is tested now, and also well documented. The current implementation was added by #2304461: KernelTestBaseTNG™

10. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
    @@ -192,6 +192,22 @@
    +   * An array of config object names that are excluded from schema checking.
    +   *
    +   * @var string[]
    +   */
    +  protected static $configSchemaCheckerExclusions = array(
    +    // Following are used to test lack of or partial schema. Where partial
    
    @@ -578,6 +594,7 @@ public function register(ContainerBuilder $container) {
             ->addArgument(new Reference('config.typed'))
    +        ->addArgument($this->getConfigSchemaExclusions())
             ->addTag('event_subscriber');
    
    @@ -598,6 +615,25 @@ public function register(ContainerBuilder $container) {
    +   */
    +  protected function getConfigSchemaExclusions() {
    +    $class = get_class($this);
    

    This was missing and made a lot of config schema tests fail. I suppose we could make a trait, but it's not that much code.

    I'm not sure if we need it to be this flexible. we could just support a single property and remove the method.

Moar conversions.

The old kernel test base does exactly this. but as far as I see, nobody every used this feature (being able to add more) and I honestly don't really see why someone would want to? It just exists to be able to test invalid config schemas.

On every morning, though shall convert some tests.

Skipping feld and migrate, will do them in separate issues I think.

I initially missed @dawehner's note above to skip views for the time being, and when I started converting them noticed what may be a big problem:

When failures occur, PHPUnit serializes the failing test object, then when it unserializes it in \PHPUnit_Util_PHP::processChildResult(), the various view objects and entity objects call their unserialize methods out of context (for instance, see ViewExecutable::unserialize()). This leads to the following exception to be displayed, masking any indication of which test failed:

Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

Opened #2686207: Convert simpletest kernel tests in modules A-I to phpunit, for half of the modules. Have to split somehow.

Also continued with the full patch, attaching here for now. Most is converted except system and migrate tests, will have some test fails.

I added #2687897: Convert system module's kernel tests to NG to focus on just the system module's tests, which as mentioned above have some failures in the direct conversion.

I reviewed the changes this patch makes to the ckeditor, editor and quickedit modules, for which I'm the maintainer. I've only got one tiny remark:

+++ b/core/modules/quickedit/tests/src/Kernel/MetadataGeneratorTest.php
@@ -2,16 +2,17 @@
+use Drupal\Tests\quickedit\Kernel\QuickEditTestBase;

This use statement should not be necessary, it lives in the same namespace.

Thanks for this!

assuming bot agrees

Maybe we should consider not removing the old base tests and then we can commit this to 8.1.x as well with no fears of breaking anyone's tests till 8.2.x when the old KernelTestBase test will be removed.

R core/modules/system/src/Tests/System/TokenReplaceUnitTestBase.php -> core/modules/system/tests/src/Kernel/Token/TokenReplaceKernelTestBase.php
R core/modules/quickedit/src/Tests/QuickEditTestBase.php -> core/modules/quickedit/tests/src/Kernel/QuickEditTestBase.php
R core/modules/language/src/Tests/LanguageTestBase.php -> core/modules/language/tests/src/Kernel/LanguageTestBase.php

@file docblocks got removed...

Merged with "git merge -X theirs 8.1.x". worked fine :)

Ah, I knew that was too easy. That also kept the wrong namespaces line. Should be fixed now.

Found some left-overs.

Patch didn't apply because it was on top of the other issue, I think.

Fix the hopefully last fails.

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -630,6 +634,16 @@ public function register(ContainerBuilder $container) {
   /**
+   * Provides the data for setting the default language on the container.
+   *
+   * @return array
+   *   The data array for the default language.
+   */
+  protected function defaultLanguageData() {
+    return Language::$defaultValues;
+  }

So the point is that tests can override this? How come we had to add this here?

I like the changes in #80 too... No base tests have changed... therefore I think this is eligible for 8.1.x as keeping all the tests aligned is more valuable than having different tests to maintain. Tests themselves are not API.

Committed 5b67fe0 and pushed to 8.1.x and 8.2.x. Thanks!