Problem

  1. Drupal\system\Tests\Module\InstallUninstallTest takes 14 minutes (23%) of the total core test suite time.

  2. That is not acceptable in any way.

Proposed solution

  1. Remove the test without replacement. It is obsolete.

  2. Re-add missing coverage for the few discrete edge-cases to ModuleHandlerTest.

    #2254203: Fix test performance of Drupal\system\Tests\Module\ModuleApiTest

Details

  1. This test is useless, because it is a "wildcard" test — instead of testing discrete functionality/regressions, it doesn't know what to test, so it blatantly tests each and everything in the hope that it will catch "something".
  2. I've studied the test code to see what expectations are actually covered. (see below)
  3. The vast majority has been made obsolete by major code refactoring in Drupal 8 (cf. ModuleHandler).

    This test predates Drupal 8. It mostly tried to address the D7/D6 architecture of modules/extensions, in which

    1. The database schema had to be installed + uninstalled manually.
    2. Variables had to be created + deleted manually.
    3. Both the installation and the uninstallation process consisted of tons of spaghetti code and wasn't centralized yet.
  4. Only a few edge-cases are explicitly covered, mostly pertaining to "required" modules.

Known regression test coverage

  1. Assert that all "required" modules are already installed by default.
  2. Assert that other modules are not installed by default.
  3. Assert that installing a module with dependencies shows a confirmation screen. → duplicates DependencyTest
  4. Assert that installing a module installs all of its dependencies. → duplicates DependencyTest
  5. Assert that a module's database schema is installed.
  6. Assert that a module's config is installed. → duplicates ConfigInstallTest and ConfigInstallWebTest
  7. Assert that a message is logged for each installed module.
  8. Assert that hook_modules_installed() is invoked.
  9. Assert that an installed module can be uninstalled.
  10. Assert that a module's database schema is uninstalled.
  11. Assert that a module's config is uninstalled. → duplicates ConfigInstallTest and ConfigInstallWebTest
  12. Assert that a message is logged for each uninstalled module.
  13. Assert that hook_modules_uninstalled() is invoked.
  14. Assert that "required" modules are not uninstalled. → duplicates ModuleApiTest
  15. Assert that a module can be re-installed after uninstalling it.
  16. Assert that multiple modules can be installed at the same time. → duplicates DependencyTest
  17. Asserts that all modules provided by core can be installed and then modules uninstalled.
CommentFileSizeAuthor
#8 through-the-moon-door.patch8.14 KBjthorson
#1 test.iu_.1.patch8.18 KBsun

Comments

sun’s picture

Status: Active » Needs review
StatusFileSize
new8.18 KB

Status: Needs review » Needs work

The last submitted patch, 1: test.iu_.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

1: test.iu_.1.patch queued for re-testing.

sun’s picture

Just noticed that some additional/duplicated test coverage for "required" modules exists in Drupal\system\Tests\Module\RequiredTest already.

sun’s picture

1: test.iu_.1.patch queued for re-testing.

jthorson’s picture

1: test.iu_.1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: test.iu_.1.patch, failed testing.

jthorson’s picture

Status: Needs work » Needs review
StatusFileSize
new8.14 KB

... Make him fly!

sun’s picture

Status: Needs review » Reviewed & tested by the community

Just because today is today.

xano’s picture

Assigned: sun » xano
Status: Reviewed & tested by the community » Needs work

Not yet. ModuleHandler needs some more test coverage.

xano’s picture

Assigned: xano » Unassigned
Status: Needs work » Reviewed & tested by the community

Oh, there is web test coverage. It's not pretty, but it does the job. Sorry for delaying this.

catch’s picture

Assigned: Unassigned » alexpott

I think alexpott mentioned that this was the only test that caught some relatively recent regressions, assigning so he can comment.

sun’s picture

@alexpott raised concerns about a different issue, #2258247: Remove useless Drupal\config\Tests\ConfigImportAllTest

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

We should prove that we have covered all the regression listed in the issue summary.

The trickiest will be

  • Assert that a module can be re-installed after uninstalling it.
  • Asserts that all modules provided by core can be installed and then modules uninstalled.
jhedstrom’s picture

Status: Needs review » Needs work

Setting at needs work based on #14.

webchick’s picture

Issue tags: +D8 Accelerate Dev Days

In talking to the DrupalCI folks, they say this is a blocker to testbot testing itself, since this test runs for a completely silly amount of time.

If anyone is looking for something fun to do at the sprint tomorrow, here's a good one. :)

mparker17’s picture

Assigned: alexpott » Unassigned

Unassigning so other people can work on it as per #16 and https://twitter.com/webchick/status/588448754358882304

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fgm’s picture

Version: 8.1.x-dev » 8.2.x-dev

Bumping to 8.2.x now that 8.1.x has been released.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Status: Needs work » Closed (won't fix)

This test again proves it is not obsolete... without this test we would not have caught #2485385-137: Move highwater field support to the source plugin, and do not expose its internals on MigrationInterface and worked on a fix in #2776235: Cached autoloader misses cause failures when missed class becomes available.

It is time to close this issue with won't fix as the premise is wrong.