Convert Module\InstallTest into a Kernel test

It is always nice to improve the speed of our tests!

+++ b/core/modules/system/src/Tests/Module/InstallKernelTest.php
@@ -8,29 +8,29 @@
  */
-class InstallTest extends WebTestBase {
+class InstallKernelTest extends KernelTestBase {
 

What is the reason that we are converting to a deprecated test class? I just tried it out quickly, and it turns out, that the no problem using the modern alternative. I literally just changed the usestatement on top of the class.

Additionally this class is now really just about testing the module installer, so what about naming it ModuleInstallerKernelTest

testGetSchemaAtInstallTime(): Could not get it passing.
testRequiredModuleSchemaVersions: Could not get it passing.
testUninstallPostUpdateFunctions: This actually passed as a kernel test, but it should not have, because it was asserting a condition that is FALSE by default. So the attached patch also adds an assertion to the web test to ensure the coverage is sufficient (and one that will fail in a kernel test by default).

You can get them running by using the following lines:

class InstallTest extends KernelTestBase {

  /**
   * {@inheritdoc}
   */
  public function setUp() {
    parent::setUp();

    // @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';

    \Drupal::service('module_installer')->install(['system', 'user', 'module_test']);
  }

+++ b/core/modules/system/src/Tests/Module/InstallKernelTest.php
@@ -8,29 +8,29 @@
-  public static $modules = array('module_test');
+  public static $modules = array('system', 'user', 'module_test');
 

If we want to test the installation process of modules, so of user and module_test we should use the module installer instead of the kernel test fake installing of modules.

Ok, attached an attempt on resolving the remaining remarks. Don't know if the interdiff makes sense here but added it anyway.

+++ b/core/modules/system/src/Tests/Module/InstallTest.php
@@ -58,36 +46,20 @@ public function testRequiredModuleSchemaVersions() {
   public function testUninstallPostUpdateFunctions() {
+    // First, to avoid false positives, ensure that the post_update function
+    // exists while the module is still installed.
+    $post_update_key_value = \Drupal::keyValue('post_update');
+    $existing_updates = $post_update_key_value->get('existing_updates', []);
+    $this->assertTrue(in_array('module_test_post_update_test', $existing_updates));
+
+    // Uninstall the module.
     \Drupal::service('module_installer')->uninstall(['module_test']);
 
+
+    // Ensure the post update function is no longer listed.
     $post_update_key_value = \Drupal::keyValue('post_update');
     $existing_updates = $post_update_key_value->get('existing_updates', []);
     $this->assertFalse(in_array('module_test_post_update_test', $existing_updates));
   }

Isn't that one also possible as kernel test?

I moved all the tests to use the kerneltestbase

Sorry, it should use the database inside the container.

Last time.. had an unused use statement.

Lovely, just lovely.

+++ b/core/modules/system/src/Tests/Module/ModuleInstallerKernelTest.php
@@ -0,0 +1,117 @@
+
+namespace Drupal\system\Tests\Module;
+
...
+use Drupal\KernelTests\KernelTestBase;
...
+ */
+class ModuleInstallerKernelTest extends KernelTestBase {
+

Note: The Namespace should be \Drupal\Tests\system\Kernel\Module and the path system/tests/src/Kernel/Module ...

Moved the test's location.

I love those changes :)

+++ b/core/modules/system/tests/src/Kernel/Module/ModuleInstallerKernelTest.php
@@ -0,0 +1,117 @@
+    $module_name = 'invalid_module_name_over_the_maximum_allowed_character_length';
+    $message = new FormattableMarkup('Exception thrown when enabling module %name with a name length over the allowed maximum', ['%name' => $module_name]);
+    try {
+      $this->container->get('module_installer')->install([$module_name]);
+      $this->fail($message->__toString());
+    }
+    catch (ExtensionNameLengthException $e) {
+      $this->assertTrue(TRUE, $message->__toString());
+    }
+
+    // Since for the UI, the submit callback uses FALSE, test that too.
+    $message = new FormattableMarkup('Exception thrown when enabling as if via the UI the module %name with a name length over the allowed maximum', ['%name' => $module_name]);
+    try {
+      $this->container->get('module_installer')->install([$module_name], FALSE);
+      $this->fail($message);
+    }
+    catch (ExtensionNameLengthException $e) {
+      $this->assertTrue(TRUE, $message->__toString());
+    }

IMHO this should be splitted up into two test methods using $this->setExceptedException

I had opened this issue in my browser some time ago to review it and now did before refreshing the; page silly me!
To review the diff easier I reshuffled the methods to their original order and used `git diff -M40%` to detect the renaming of the file.

Then I also addressed @dawehners comment in 16, since that allows to remove the need for FormattableMarkup.

I would mark it as RTBC if it wasn't for having changed the last method.

Great patch! This is a review for #14.

Please add also an interdiff.txt on each patch, regardless of how small is the change, so the reviewers can understand better what was changed since the last patch.

1. +++ b/core/modules/system/src/Tests/Module/ModuleInstallerKernelTest.php
   @@ -0,0 +1,117 @@
   +    // @todo ModuleInstaller calls system_rebuild_module_data which is part of
   +    // system.module, see https://www.drupal.org/node/2208429.
   

   @todo following lines need to be indented.

2. +++ b/core/modules/system/src/Tests/Module/ModuleInstallerKernelTest.php
   @@ -0,0 +1,117 @@
   +    \Drupal::service('module_installer')->install([
   ...
   +    \Drupal::service('module_installer')->install(['user'], FALSE);
   ...
   +      $this->container->get('module_installer')->install([$module_name], FALSE);
   ...
   +    \Drupal::service('module_installer')->uninstall(['module_test']);
   

   Hm. I see we are using module_installer service in 5 times, in this class. Wouldn't it be better if we add it as a protected property and we set it in setUp()?

   $this->moduleHandler = $this->container->get('module_handler');
   

3. +++ b/core/modules/system/src/Tests/Module/ModuleInstallerKernelTest.php
   @@ -0,0 +1,117 @@
   +    $this->assertSame($this->config('core.extension')->get('module.user'), 0);
   ...
   +    $this->assertNotSame($this->config('core.extension')->get('module.does_not_exist'), 0);
   ...
   +    $this->assertSame($value, 'varchar');
   

   Usually in PHPUnite tests we are putting the expectation as first argument, even the result is the same. I would invert the order of arguments.

4. +++ b/core/modules/system/src/Tests/Module/ModuleInstallerKernelTest.php
   @@ -0,0 +1,117 @@
   +    try {
   ...
   +      $this->fail($message->__toString());
   +    }
   +    catch (ExtensionNameLengthException $e) {
   +      $this->assertTrue(TRUE, $message->__toString());
   +    }
   ...
   +    try {
   ...
   +      $this->fail($message);
   +    }
   +    catch (ExtensionNameLengthException $e) {
   +      $this->assertTrue(TRUE, $message->__toString());
   +    }
   

   Consider using $this->setExpectedException() instead of try {...} catch() {...} workaround. See https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writ...

5. +++ b/core/modules/system/src/Tests/Module/ModuleInstallerKernelTest.php
   @@ -0,0 +1,117 @@
   +    $this->assertTrue(in_array('module_test_post_update_test', $existing_updates));
   ...
   +    $this->assertTrue(in_array('module_test_post_update_test', $existing_updates));
   

   With phpunit we have a dedicated assertion to be used for testing array subsets:

   $this->assertArraySubset(['module_test_post_update_test'], $existing_updates);
   

   Note: Unfortunately we don't have the same negation assertion.

6. +++ b/core/modules/system/src/Tests/Module/ModuleInstallerKernelTest.php
   @@ -0,0 +1,117 @@
   +    $this->assertTrue($version > 0, 'System module version is > 0.');
   ...
   +    $this->assertTrue($version > 0, 'User module version is > 0.');
   

   Use $this->assertGreaterThan() instead. You can drop the message.

7. +++ b/core/modules/system/src/Tests/Module/ModuleInstallerKernelTest.php
   @@ -0,0 +1,117 @@
   +   * Verify that drupal_get_schema() can be used during module installation.
   

   s/Verify/Verifies

Let's make it even a bit better :)

1. +++ b/core/modules/system/tests/src/Kernel/Module/ModuleInstallerKernelTest.php
   @@ -45,28 +55,17 @@ public function testEnableUserTwice() {
   +    $this->assertTrue($version > 0, 'System module version is > 0.');
   ...
   +    $this->assertTrue($version > 0, 'User module version is > 0.');
   

   Could be assertGreaterThan

2. +++ b/core/modules/system/tests/src/Kernel/Module/ModuleInstallerKernelTest.php
   @@ -45,28 +55,17 @@ public function testEnableUserTwice() {
   +    $this->assertTrue(in_array('module_test_post_update_test', $existing_updates));
   

   What about using $this->assertContains?

3. +++ b/core/modules/system/tests/src/Kernel/Module/ModuleInstallerKernelTest.php
   @@ -89,29 +88,24 @@ public function testUninstallPostUpdateFunctions() {
   +   * @expectedException \Drupal\Core\Extension\ExtensionNameLengthException
   ...
   +   * @expectedException \Drupal\Core\Extension\ExtensionNameLengthException
   

   Note: https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

@dawehner cross-review :)

@claudiu.cristea
That's kind of a luxury problem to have :P

Just updating the status now.

Oh yeah one thing, IMHO the tests could now live inside core/tests instead of core/modules/system as well.

Attached the updates. I hope I created the diffs correctly.

Namespacing...

1. +++ b/core/tests/Drupal/KernelTests/Core/Module/ModuleInstallerKernelTest.php
   @@ -59,13 +68,13 @@ public function testEnableUserTwice() {
   -    $this->assertTrue(in_array('module_test_post_update_test', $existing_updates));
   +    $this->assertArraySubset(['module_test_post_update_test'], $existing_updates);
   
   @@ -76,10 +85,10 @@ public function testUninstallPostUpdateFunctions() {
   -    $this->assertTrue(in_array('module_test_post_update_test', $existing_updates));
   +    $this->assertArraySubset(['module_test_post_update_test'], $existing_updates);
   

   Can't you use assertContains to be more explicit here?

2. +++ b/core/tests/Drupal/KernelTests/Core/Module/ModuleInstallerKernelTest.php
   @@ -89,23 +98,21 @@ public function testUninstallPostUpdateFunctions() {
      public function testModuleNameLengthFalse() {
   

   that method is a bit unclear, what about using testModuleNameLengthWithDependencyCheck?

Great, we're almost done. Few nits yet to be fixed:

1. +++ b/core/tests/Drupal/KernelTests/Core/Module/ModuleInstallerKernelTest.php
   @@ -1,31 +1,50 @@
   +   * @var $moduleInstaller \Drupal\Core\Extension\ModuleInstallerInterface;
   

   Remove $moduleInstaller. In properties docs we only put: @var type-hint.

2. +++ b/core/tests/Drupal/KernelTests/Core/Module/ModuleInstallerKernelTest.php
   @@ -1,31 +1,50 @@
   +  private $moduleInstaller;
   

   This should be 'protected' not 'private'. Private blocks an hypothetical extension class that wants to change the definition (e.g. wants to add a default value).

3. +++ b/core/tests/Drupal/KernelTests/Core/Module/ModuleInstallerKernelTest.php
   @@ -45,21 +68,29 @@ public function testEnableUserTwice() {
   +    $post_update_key_value = \Drupal::keyValue('post_update');
   ...
   +    $this->assertArraySubset(['module_test_post_update_test'], $existing_updates);
   ...
        $this->assertFalse(in_array('module_test_post_update_test', $existing_updates));
   

   Yepp, @dawehner idea from #19.2 is better than mine because assertContains() has also an assertNotContains() counter assertion. Please fix all the assertTrue(in_array()) and assertFalse(in_array()) occurrences.

4. +++ b/core/tests/Drupal/KernelTests/Core/Module/ModuleInstallerKernelTest.php
   @@ -70,24 +101,18 @@ public function testUninstallPostUpdateFunctions() {
   +    $this->setExpectedException(ExtensionNameLengthException::class);
   ...
   +    $this->setExpectedException(ExtensionNameLengthException::class);
   

   Let's use @dawehner's recommendation from #19. Read https://thephp.cc/news/2016/02/questioning-phpunit-best-practices. Sorry to confuse you in #18. So, expectException() seems the best practice in asserting exceptions.

Wow, a second cross-review in the same issue :)

Updated the patches. I keep using setExpectedException as expectException is only available after phpUnit 5.2.

Nice!

+++ b/core/tests/Drupal/KernelTests/Core/Module/ModuleInstallerKernelTest.php
@@ -15,9 +15,9 @@ class ModuleInstallerKernelTest extends KernelTestBase {
+   * @var \Drupal\Core\Extension\ModuleInstallerInterface;

Nitpick: we don't use semicolons in comments :)

1. similarity index 36%
   rename from core/modules/system/src/Tests/Module/InstallTest.php
   

   I would have expected some methods to be removed from this class.

2. +++ b/core/tests/Drupal/KernelTests/Core/Module/ModuleInstallerKernelTest.php
   @@ -1,31 +1,50 @@
   +    // @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';
   

   This test should just rely on system module rather than doing this include.

Reworked as the patch doesn't apply anymore (the web test has moved as functional), so no interdiff. This patch decreases the locally test run from 27.6 to 11.61 seconds.

Replying to #36.2:

This test should just rely on system module rather than doing this include.

@alexpott. no, we cannot rely on system module. The test, specifically ::testRequiredModuleSchemaVersions(), needs the system.module to be installed using the module_installer service. So, we'll have to use the service rather than adding the system.module in the protected static $module array. So we'll have to explicitly include the module main file.

Let's get #2926068: Deprecate system_rebuild_module_data() and remove usages in core done first and then the problem is moot.

@alexpott, ok, but #2926068: Deprecate system_rebuild_module_data() and remove usages in core waits for #3024461: Adopt consistent deprecation format for core and contrib deprecation messages which waits for #2908391: Add a rule for expected format of @deprecated and @trigger_error() text. And I don't think #2926068: Deprecate system_rebuild_module_data() and remove usages in core should wait for #3024461: Adopt consistent deprecation format for core and contrib deprecation messages as that policy is not yet approved.

Blocking on #2926068: Deprecate system_rebuild_module_data() and remove usages in core.

Fixed #44. I've created the patch with git diff --cached -M10%, there's no interdiff.

1. +++ b/core/modules/system/tests/src/Kernel/Module/InstallTest.php
   @@ -1,44 +1,64 @@
   -    // @see module_test_install()
   

   We should keep this @see reference.

2. +++ b/core/modules/system/tests/src/Kernel/Module/InstallTest.php
   @@ -71,24 +99,18 @@ public function testUninstallPostUpdateFunctions() {
   +   * Tests that an exception is thrown when a module name is too long.
   

   This just repeats the name of the previous test function, so they have the same doc block. What is the difference?

3. +++ b/core/modules/system/tests/src/Kernel/Module/InstallTest.php
   @@ -71,24 +99,18 @@ public function testUninstallPostUpdateFunctions() {
   +  public function testModuleNameLengthWithDependencyCheck() {
   

   I think this name is wrong, should this be "testModuleNameLengthWithoutDependencyCheck()"? Which is a bit long, maybe you can come with something better?

- Rerolled against 9.4.x-dev
- Merged latest commits
- Tried to address 1., 2. and 3. in #48

Interdiff isn't too useful, I fear, since it won't detect the renaming of the test class.

Back to NR.

For a reroll into 10.1.x please.

Will do a review after that

Rerolled for 10.1.x

My bad @Spokje, forgot to reload page.

Rerolled.

Looking at the average comment rate in this issue, I'll be back in about one year to reroll for 10.4/10.5 😈

known random JS test failure, ordered retest.

@_pratik_ the patch before #58 was for 10.1 already

Reviewing #58
Looks good for me thanks!

Committed and pushed 8a2f6514b7 to 10.1.x and 02268796de to 10.0.x. Thanks!

Backported to 10.0.x because this is a test-only change. Property typehinting makes this unsuitable for 9.5.x