Problem/Motivation

KernelTestBase::installConfig($modules) can be called with a list of modules, but if one of them causes a failure, you don't know which one it was.

A simple improvement would be to catch any exceptions and re-throw them with a message to say which module caused the problem, like this:

      try {
        $this->container->get('config.installer')->installDefaultConfig('module', $module);
      }
      catch (\Exception $e) {
        throw new \Exception(sprintf('Exception when installing config for module %s, message was: %s', $module, $e->getMessage()), 0, $e);
      }

(tagging as Novice, if someone wants to make an MR from the above code before I get round to it)

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3308162-13.patch877 bytesimmaculatexavier
#10 3308162-10.patch982 bytesimmaculatexavier
#7 3308162-7.patch1.21 KBchaubeyji
#7 3308162-7.patch1.21 KBchaubeyji
#5 interdiff_4-5.txt1.08 KBimmaculatexavier
#5 3308162-5.patch27.2 KBimmaculatexavier
#4 interdiff_2-3.txt26.67 KBimmaculatexavier
#4 3308162-3.patch27.2 KBimmaculatexavier
#2 3308162-1.patch989 byteschaubeyji

Comments

joachim created an issue. See original summary.

chaubeyji’s picture

chaubeyji commented
StatusFileSize
new 3308162-1.patch989 bytes

I have created a patch as you suggested, please review.

joachim’s picture

joachim commented

Thanks!

Not quite right though:

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -698,11 +698,12 @@ public function tearDownCloseDatabaseConnection() {
-      if (!$this->container->get('module_handler')->moduleExists($module)) {
-        throw new \LogicException("$module module is not enabled.");

This exception needs to be kept.

immaculatexavier’s picture

immaculatexavier commented
Status: Needs work » Needs review
StatusFileSize
new 3308162-3.patch27.2 KB
new interdiff_2-3.txt26.67 KB

Fixed the custom commands against #2

immaculatexavier’s picture

immaculatexavier commented
StatusFileSize
new 3308162-5.patch27.2 KB
new interdiff_4-5.txt1.08 KB

@jaochim,
Attached patch against #3

joachim’s picture

joachim commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -699,327 +692,333 @@ public function tearDownCloseDatabaseConnection() {
-  /**
-   * Installs database tables from a module schema definition.
-   *
-   * @param string $module
-   *   The name of the module that defines the table's schema.
-   * @param string|array $tables
-   *   The name or an array of the names of the tables to install.
-   *
-   * @throws \LogicException
-   *   If $module is not enabled or the table schema cannot be found.
-   */
-  protected function installSchema($module, $tables) {
-    /** @var \Drupal\Core\Extension\ModuleHandlerInterface $module_handler */
-    $module_handler = $this->container->get('module_handler');
-    // Database connection schema is technically able to create database tables
-    // using any valid specification, for example of a non-enabled module. But
-    // ability to load the module's .install file depends on many other factors.
-    // To prevent differences in test behavior and non-reproducible test
-    // failures, we only allow the schema of explicitly loaded/enabled modules
-    // to be installed.
-    if (!$module_handler->moduleExists($module)) {
-      throw new \LogicException("$module module is not enabled.");

This patch is making tons of other changes!

chaubeyji’s picture

chaubeyji commented
StatusFileSize
new 3308162-7.patch1.21 KB
new 3308162-7.patch1.21 KB

Please try now.

joachim’s picture

joachim commented 
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -698,10 +698,16 @@ public function tearDownCloseDatabaseConnection() {
+      try {
+        if (!$this->container->get('module_handler')->moduleExists($module)) {
+          $this->container->get('config.installer')->installDefaultConfig('module', $module);
+          throw new \LogicException("$module module is not enabled.");
+        }
+        $this->container->get('config.installer')->installDefaultConfig('module', $module);
+      }
+      catch (\Exception $e) {
+        throw new \Exception(sprintf('Exception when installing config for module %s, message was: %s', $module, $e->getMessage()), 0, $e);

No, this is messing with the earlier exception.

joachim’s picture

joachim commented

The only change that is needed is wrapping the call to installDefaultConfig in a try/catch block.

immaculatexavier’s picture

immaculatexavier commented
Status: Needs work » Needs review
StatusFileSize
new 3308162-10.patch982 bytes

I made a patch earlier against #2.
I have now built a patch from the ground up.

Also for a note:
When we add exception below if condition an error throw as below
-------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
-------------------------------------------------------------------
703 | WARNING | Code after the THROW statement on line 702 cannot be executed
704 | WARNING | Code after the THROW statement on line 702 cannot be executed
706 | WARNING | Code after the THROW statement on line 702 cannot be executed
707 | WARNING | Code after the THROW statement on line 702 cannot be executed

-      if (!$this->container->get('module_handler')->moduleExists($module)) {
-        throw new \LogicException("$module module is not enabled.");

So i have removed THROW statement on line 702

@@ -699,7 +699,12 @@ public function tearDownCloseDatabaseConnection() {
   protected function installConfig($modules) {
     foreach ((array) $modules as $module) {
       if (!$this->container->get('module_handler')->moduleExists($module)) {
-        throw new \LogicException("$module module is not enabled.");

Kindly review.

joachim’s picture

joachim commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -699,7 +699,12 @@ public function tearDownCloseDatabaseConnection() {
-        throw new \LogicException("$module module is not enabled.");

This is the same problem as the earlier patch -- the exception thrown when the module isn't enabled has been removed!

catch’s picture

catch
he/him
Primary language English
commented 
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -699,7 +699,12 @@ public function tearDownCloseDatabaseConnection() {
       }
       $this->container->get('config.installer')->installDefaultConfig('module', $module);
     }
 

It's this line that should be replaced with the try/catch that's added in the patch.

immaculatexavier’s picture

immaculatexavier commented
Status: Needs work » Needs review
StatusFileSize
new 3308162-13.patch877 bytes

@joachim , sorry for the misplacement.
@catch, thanks for the resolution.
Attached patch against #12

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

Perfect, thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed cc7f697733 to 10.1.x and a99518b374 to 10.0.x and 93c1a95d57 to 9.5.x. Thanks!

Backported to 9.5.x as this is a test-only improvement with no BC issues.

  • alexpott committed cc7f697 on 10.1.x 
    Issue #3308162 by immaculatexavier, chaubeyji, joachim, catch:...

  • alexpott committed a99518b on 10.0.x 
    Issue #3308162 by immaculatexavier, chaubeyji, joachim, catch:...

  • alexpott committed 93c1a95 on 9.5.x 
    Issue #3308162 by immaculatexavier, chaubeyji, joachim, catch:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.