\Drupal\Tests\system\Functional\Module\VersionTest could be a Kernel (or Unit) Test

This was originally added 11 years ago in #211747: Allow specifying version information as part of module dependencies → . I approached this first by doing the least possible to convert from a Functional test to a Kernel test.

Before
Time: 00:44.311, Memory: 12.00 MB

After:
Time: 00:00.909, Memory: 12.00 MB

Before refactoring further I wonder if it's worth just committing this for the massive speed boost and then discussing the following.

I think started investigating how it works and wish I had never seen this. The test has an array of dependency strings. Every odd numbered dependency is valid and every even numbered dependency is invalid. This is stored in state. There is a hook \Drupal\system_test\Hook\SystemTestHooks::systemInfoAlter that shifts the first constraint off the list and alters the info for the module_test module. The test then loops through the dependency strings, loads the modules form which invokes the hook, and checks if the checkbox to enable module_test is disabled or not.

So I began to wonder what it is we're actually trying to test. Is this the syntax of the version strings? Is it that the checkbox is disabled for incompatible modules? In #3 @larowlan mentioned we could probably just add most of these to \Drupal\Tests\Component\Version\ConstraintTest::providerIsCompatible. But when I stepped through the code it seems it might be more appropriate to expand \Drupal\Tests\Core\Extension\DependencyTest::testIsCompatible. The test essentially boils down to the value of $incompatible in this snippet:

// If this module requires other modules, add them to the array.
/** @var \Drupal\Core\Extension\Dependency $dependency_object */
foreach ($module->requires as $dependency => $dependency_object) {
  // @todo Add logic for not displaying hidden modules in
  //   https://drupal.org/node/3117829.
  if ($incompatible = $this->checkDependencyMessage($modules, $dependency, $dependency_object)) {

I also tried using a provider but it's quite a lot slower:

Time: 00:08.720, Memory: 12.00 MB

So I've reverted that, but made some tweaks to remove some hacks like the static variable and the array_shift. It means state is only set in the test and not also in the hook, which makes it easier to understand the even/odd logic.

Seems like a good refactor to me. Still asserting that things are disabled as before. See no issue.

This was originally added 11 years ago in #211747: Allow specifying version information as part of module dependencies.

Had a quick look at the issue, I think it was actually added 16 years ago? Looks like the cvs commit happened in 2009.

Let's definitely open a follow-up to discuss whether we should remove the test altogether or if it can be simplified further.

Committed/pushed to 11.x, thanks!

catch → closed merge request !12542

You're right, it was 16 years ago, it was the last updated date on the issue that was 11 years ago.

Follow up 📌 Decide the fate of Drupal\Tests\system\Kernel\Module\VersionTest Active