The test calls \Drupal::service('module_installer')->install(...) and does not check the result to see that the module was installed correctly.

This patch from #1719648: ModuleInstaller::install() should provide more information than simply returning FALSE when a module isn't in the file system shows the installation in fact fails.

CommentFileSizeAuthor
#11 2387669-11.patch1.72 KBrpayanm
#11 2387669-interdiff.txt575 bytesrpayanm
#9 2387669-9.patch1.97 KBrpayanm
#6 1719648.31.drupal.module-install-missing-ancillary-config-tests_6.patch941 bytestadityar
1719648.31.drupal.module-install-missing-ancillary-config-tests.patch941 bytesjoachim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

joachim CreditAttribution: joachim commented
Issue summary: View changes
Related issues: +#1719648: ModuleInstaller::install() should provide more information than simply returning FALSE when a module isn't in the file system
joachim’s picture

joachim CreditAttribution: joachim commented
Status: Active » Needs review

Oops.

Test should fail.

Status: Needs review » Needs work

The last submitted patch, 1719648.31.drupal.module-install-missing-ancillary-config-tests.patch, failed testing.

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented 
  1. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
@@ -132,7 +132,8 @@ function testInstallProfileConfigOverwrite() {
     // Turn on the test module, which will attempt to replace the
     // configuration data. This attempt to replace the active configuration
     // should be ignored.

    This comment doesn't match the code, the test module referred to is empty. This fails because the config_existing_default_config_test.yml is wrongly named, should be config_existing_default_config_test.info.yml. I don't know if this is intentional or not. CMI folks may want to take a look at this.

  2. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
@@ -132,7 +132,8 @@ function testInstallProfileConfigOverwrite() {
-    \Drupal::service('module_installer')->install(array('config_existing_default_config_test'));
+    $status = \Drupal::service('module_installer')->install(array('config_existing_default_config_test'));
+    $this->assertTrue($status, "The module config_existing_default_config_test was installed.");

    Another test in the class (::testIntegrationModuleReinstallation) also does similar ModuleInstaller::install(), maybe we can extend this check to include those.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: +Novice

1. That is wrong, it should be renamed. Tagging Novice to do exactly that, provide a patch with that rename and the test change from the initial patch and then it should be green.

tadityar’s picture

tadityar CreditAttribution: tadityar commented
Status: Needs work » Needs review
FileSize
1719648.31.drupal.module-install-missing-ancillary-config-tests_6.patch941 bytes

Is this the file that should be renamed?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

I can't say, the patch doesn't contain it :)

The last submitted patch, 6: 1719648.31.drupal.module-install-missing-ancillary-config-tests_6.patch, failed testing.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
Status: Needs work » Needs review
FileSize
2387669-9.patch1.97 KB

Trying...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Rename looks correct, have a look at https://www.drupal.org/documentation/git/configure for configuring git diff so that it picks up the rename as a rename and not a delete/add of a new file.

I believe we no longer add explicit hidden: true to modules in tests/modules folders, as they are automatically hidden (or non-existent, actually) when you're not within a test.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm commented
FileSize
2387669-interdiff.txt575 bytes
2387669-11.patch1.72 KB

Config git successfully ! Thank you :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Assigned: Unassigned » alexpott
Status: Needs review » Reviewed & tested by the community

I think this is good to go, we could also check the other install() call, but I think #1719648: ModuleInstaller::install() should provide more information than simply returning FALSE when a module isn't in the file system will take care of that in a more generic and better way.

RTBC, alexpott will probably want to see this, so assigning to him.

joachim’s picture

joachim CreditAttribution: joachim commented

That's correct, #1719648: ModuleInstaller::install() should provide more information than simply returning FALSE when a module isn't in the file system will take care of crashing tests properly when a module install fails ;)

alexpott’s picture

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

Nice find. Committed 8e7f24f and pushed to 8.0.x. Thanks!

  • alexpott committed 8e7f24f on 8.0.x 
    Issue #2387669 by rpayanm, joachim, tadityar: ConfigInstallWebTest is...

Status: Fixed » Closed (fixed)

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