drupal_phpunit_find_extension_directories() uses infinite recursion ⇒ more directories = slower tests

Duplicate of #3223881: Speed up test discovery with \RecursiveCallbackFilterIterator?

edit: and you even came up with basically the same approach :)

+++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveGenericExtensionFilterIterator.php
@@ -0,0 +1,113 @@
+ * RecursiveExtensionFilterIterator::$skippedFolders property.

this class name now needs updating

This looks good to me - I'm happy either way.

I've been running my own phpunit.xml for at least 3 years to bypass this problem (and we have an even more optimized one on client projects where we know where tests will live). That's probably why this hasn't been fixed, because phpunit.xml.dist can be freely modified as phpunit.xml - so thank you for also coming back to try and improve this for everyone.

Crediting participants in the other issue and marking it as a duplicate

I only read the comments and have some suggestions.

1. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -3,20 +3,11 @@
   + * Filters a RecursiveDirectoryIterator to discover extensions in root folders.
   

   This would be clearer if 'root' was 'Drupal root'.

2. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -3,20 +3,11 @@
   + * filter implementation hard-codes a range of assumptions. See the
   

   If this is saying see some documentation should there be an @see below to that class. At least, I was looking for it when I read this.

3. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -130,12 +75,16 @@ public function getChildren() {
   +    // If this is a subdirectory of a base search path, only recurse into the
   

   I have trouble following this paragraph. Maybe it is just me. But it appears to be a duplicate of the comment that is below the if statement. So there are not two copies of this comment, one above the if and one inside.

4. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -143,26 +92,12 @@ public function accept() {
   +    // Otherwise, respect the parent implementation's conclusion.
   

   Let's not use possessive on implementation. 'Otherwise, respect the implementation of the parent.'

5. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveGenericExtensionFilterIterator.php
   @@ -0,0 +1,113 @@
   +      // 'config' directories are special-cased here, because every extension
   

   Can we have the first sentence start with a capital letter and remove the hyphen in special-cased. And there are extensions that do not have a config directory.

   'The 'config' directories are a special case because extensions may have a directory named 'config'.

6. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveGenericExtensionFilterIterator.php
   @@ -0,0 +1,113 @@
   +      // because core happens to have a directory of an actual module that is
   +      // named 'config'. By explicitly testing for that case, we can skip all
   

   Clarify the name of the module and the machine name.

   "because the Configuration Manager module in core has the machine name 'config'."

7. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveGenericExtensionFilterIterator.php
   @@ -0,0 +1,113 @@
   +      // (whereas it must be located directly in a modules directory).
   

   Not sure what this phrase is telling me. It 'it' the core config module?

8. +++ b/core/tests/bootstrap.php
   @@ -22,15 +23,22 @@
   +  // symlinks (to allow extensions to be linked from elsewhere), and return
   

   Does this phrase really need to be in parenthesis?

edit: grammar fixes

There is a lot of variation in DrupalCI runs. I also wonder if disk is even the issue here, the instances have plenty of RAM and have checked out the entire repository shortly before running the tests; to me this would imply that all the directories should be in cache and so while we are saving a bit of CPU time here, it is probably not all that much in terms of wall clock time compared to running the actual test suite.

However as this *does* markedly speed up runs on developer machines where the whole project is not necessarily in RAM already, this is still a good thing to fix.

Maybe this should be deferred to a followup but it feels like we should be able to combine a bit more of the test bootstrap and the iterator:

RecursiveExtensionFilterIterator stores this list:

  protected $allowedExtensionTypes = [
    'profiles',
    'modules',
    'themes',
  ];

drupal_phpunit_contrib_extension_directory_roots() builds a lot of lists containing these items:

  $paths = [
    $root . '/core/modules',
    $root . '/core/profiles',
    $root . '/core/themes',
    $root . '/modules',
    $root . '/profiles',
    $root . '/themes',
  ];
  $sites_path = $root . '/sites';
  // Note this also checks sites/../modules and sites/../profiles.
  foreach (scandir($sites_path) as $site) {
    if ($site[0] === '.' || $site === 'simpletest') {
      continue;
    }
    $path = "$sites_path/$site";
    $paths[] = is_dir("$path/modules") ? realpath("$path/modules") : NULL;
    $paths[] = is_dir("$path/profiles") ? realpath("$path/profiles") : NULL;
    $paths[] = is_dir("$path/themes") ? realpath("$path/themes") : NULL;
  }

I wonder if drupal_phpunit_contrib_extension_directory_roots() should start returning the top level directories instead and let RecursiveExtensionFilterIterator handle the modules, profiles and themes subdirectories?

I added some extra logging inside the iterators and the final loop in drupal_phpunit_find_extension_directories(). Before this patch there were 22,156 possible extensions naively returned by the iterator to consider. With the patch applied, there are only 547 entries which exactly matches the set of .info.yml files.

Testing this locally on a core checkout alone there is still an significant improvement. Before the patch:

$ ddev exec vendor/bin/phpunit -c core core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php 
PHPUnit 8.5.21 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\ckeditor5\Kernel\ValidatorsTest
..............................                                    30 / 30 (100%)

Time: 19.46 seconds, Memory: 4.00 MB

OK (30 tests, 50 assertions)

After the patch:

$ ddev exec vendor/bin/phpunit -c core core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php 
PHPUnit 8.5.21 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\ckeditor5\Kernel\ValidatorsTest
..............................                                    30 / 30 (100%)

Time: 16.61 seconds, Memory: 4.00 MB

OK (30 tests, 50 assertions)

This patch will speed up test runs for any developer using PHPUnit with Drupal. There is nothing left to do here, #30 can be tackled in a followup as it's a cleanup rather than any further performance improvement and so to me this is RTBC.

The improvements to local testing look nice. I think though we should ask ourselves about the naming / class structure. I'm concerned about \Drupal\Core\Extension\Discovery\RecursiveGenericExtensionFilterIterator missing

      // If this is a subdirectory of a base search path, only recurse into the
      // fixed list of expected extension type directory names. Required for
      // scanning the top-level/root directory; without this condition, we would
      // recurse into the whole filesystem tree that possibly contains other
      // files aside from Drupal.
      if ($this->current()->getSubPath() == '') {
        return in_array($this->current()->getFilename(), $this->allowedExtensionTypes, TRUE);
      }

I think that we can do this all with less new code and different code paths (eventually).

If drupal_phpunit_contrib_extension_directory_roots returned:

[
  $root . '/core',
  $root,
  $root . '/sites' . $site, // It'll have to find all...
]

Then we would need two Iterators.

Also I think we can improve things for contrib and custom by doing what ExtensionDiscovery does and passing $ignore_directories into the \Drupal\Core\Extension\Discovery\RecursiveExtensionFilterIterator::__construct... so we skip node_modules by default.

    // Allow directories specified in settings.php to be ignored. You can use
    // this to not check for files in common special-purpose directories. For
    // example, node_modules and bower_components. Ignoring irrelevant
    // directories is a performance boost.
    $ignore_directories = Settings::get('file_scan_ignore_directories', []);

Something like this.

No interdiff because it's a different approach.

Fixing coding standards.

Reading this issue summary is great! So much detailed information.
I would love to see performance differences between #26 and #36 because they are different approaches. So story in the IS is probably not exactly right anymore if we decided to follow that approach?

Tested #37 with 9.4 - works great.

The speed improvement on large codebase with multiple `node_modules` directories is significant (30s vs 10s).

One issue is that when running Unit tests settings are not picked up and this line $ignore_directories = Settings::get('file_scan_ignore_directories', []); will always return empty array. But maybe I ran it incorrectly. Manually populating that return value with ['node_modules'] did the trick.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-rolled #37 for 9.5.x with the following changes:
1. Fixed $ignore_directories = Settings::get('file_scan_ignore_directories', []); not working as settings has not been initialised yet.
2. Updated deprecation messages to point to 9.6.x

When testing custom modules or themes using Drupal Core's PHPUnit configuration file, core/phpunit.xml.dist, note that the core/tests/bootstrap.php file scans all extension roots (core, modules, themes, etc.), and there is currently no way to opt out of this behavior. While we may want to use Drupal Core's bootstrap file to reuse PHPUnit behavior when testing custom modules or themes within real sites, we may not want to use the same targets.

In practice, the current implementation of core/tests/bootstrap.php can lead to longer test execution times that scale proportionally with the number of installed modules. For instance, if you have a site with 100 contributed modules and one custom module with 1 unit test, running that test will scan through core and 100s of contributed modules to then only run one test in your custom module.

And if you run these tests in a Docker container, the performance degradation can be even worse due to PHP's constant checking for directory and file existence while performing such a lookup.

UPDATE: Found https://www.drupal.org/project/drupal/issues/2943172

We've been running the version from #3223881: Speed up test discovery with \RecursiveCallbackFilterIterator with our phpunit.xml referencing a custom bootstrap.php for ~4 years. You just need to update it from time to time as the major phpunit version changes.

@alex.skrypnyk thanks for finding the other issue where I was also involved, or made an attempt to make a related change...

+ $ignore_directories = Settings::get('file_scan_ignore_directories', []);

I did the some, but @Lendude might have a valid point on the other issue

Also, are we really sure we want to re-use an existing setting? Does 'file_scan_ignore_directories' always match the pattern we want to use here? The current use case is to ignore dirs where we look for modules, do we really want to add 'and where we look for tests' to that? Sure, there might be a lot of overlap, but is this always 100%?

(Source https://www.drupal.org/project/drupal/issues/2943172#comment-13216764)

I noticed that it recurses through .git directories too - that wouldn't be as bad as node_modules but, still seems like a waste

Let's try and revive this, opened an MR with the simplest change that should work.

Added a static method to ExtensionDiscovery so we can reuse some code.

This is a good start, I wonder if this should be in the extension system since it's about extension discovery.

I think we'd want to patch this in contrib before we release this to make sure tests work properly still.

Any recommendation for testing this one?

Is the summary still accurate?