Problem/Motivation

At present our test discovery scans every folder in some key places to try and find any that contain an .info.yml file and hence could contain a module/theme etc.

Unfortunately, this means we're scanning folders where we know for sure modules can't be put including

  • .git folders - if you work on core and contrib from the same space, you're likely to have a lot of .git folders - one for each contrib project
  • src folder - this is only OO code, and will never hold modules
  • config/install config/schema - for obvious reasons

Steps to reproduce

Start a timer at the top of bootstrap.php
Stop the timer at the end of the file.
Output the time.
For my local setup its 30sec on discovery alone.

Proposed resolution

Add a RecursiveCallbackFilterIterator to the iterator, and filter out folders we know don't contain modules.
We can use some of the list from RecursiveExtensionFilterIterator
With the attached patch discovery goes from 30sec to 5sec on my local.

Remaining tasks

Reviews

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 interdiff_15-18.txt688 bytesspokje
#18 3223881-18.patch1.91 KBspokje
#17 3223881-17.patch2.94 KBspokje
#17 interdiff_15-17.txt761 bytesspokje
#15 interdiff_3-15.txt700 bytesvsujeetkumar
#15 3223881-15.patch2.57 KBvsujeetkumar
#3 3223881.patch2.58 KBlarowlan
#2 3223881.patch2.58 KBlarowlan

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review
StatusFileSize
new 3223881.patch2.58 KB
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
StatusFileSize
new 3223881.patch2.58 KB 
+++ b/core/tests/bootstrap.php
@@ -22,7 +23,53 @@
+      substr($current->getPathName(), -1 * strlen('config/install')) == 'config/schema') {

whoops

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Interesting. The actual .info.yml/module discovery has an exclusion list too, could we share that somehow. Will need to look up where that is.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#1451320: Evaluate Symfony2's Finder Component to simplify file handling
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
+++ b/core/tests/bootstrap.php
@@ -22,7 +23,53 @@
+      'icons',

drumm pointed out that icons is a valid module, so this has to go

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

@Berdir yeah I borrowed the starting point from RecursiveExtensionFilterIterator

tr’s picture

tr commented

Elsewhere we use ExtensionDiscovery, why not here? ExtensionDiscovery already excludes .git among other things. And ExtensionDiscovery already has built-in support for designating directories to ignore. Perhaps improve that if necessary and use it for test discovery as well?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I've been down that road before in #3050881: Use RecursiveExtensionFilterIterator in drupal_phpunit_find_extension_directories for performance

In that issue dawehner asked

I'm wondering conceptually, is it okay to have tests relying on code which will be tested by it?

And that's the same question for ExtensionDiscovery.

The new approach here is only using built-in PHP features so we're not making tests rely on code they're testing.

tr’s picture

tr commented

You can turn that around and ask if it's OK to use untested code like drupal_phpunit_find_extension_directories() to run our tests. What if your code doesn't work as expected - do we have any way of knowing this? The excessive time taken to scan .git directories is something that wouldn't have ever happened if we used the well-tested core methods for extension discovery, as those methods get a lot of scrutiny when running on live sites. Maintaining a separate code base just for tests pretty much ensures that things will be done differently and that improvements in one or the other might not be cross-ported.

I would say it makes the most sense to use *exactly* the same code to find .info.yml files in both core and in the tests.

tr’s picture

tr commented

Also, you didn't mention that when you quoted @dawehner asking:

I'm wondering conceptually, is it okay to have tests relying on code which will be tested by it?

the very next sentence says:

I think it is worth, given the huge speed difference!

So the way I read it, @dawehner is saying that yes, it's OK in this case to use ExtensionDiscovery.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

To be clear, I'm happy with either approach, because I'm tired of waiting 30sec before PHPUnit even boots up.

On all our projects we've been running a version of #3050881: Use RecursiveExtensionFilterIterator in drupal_phpunit_find_extension_directories for performance for at least 2 years.

We just reference a custom bootstrap.php from phpunit.xml. I'm doing the same for my core/contrib folder too, even if it means from time to time I need to cherry-pick in changes from upstream.

But it would be great to get this fixed for everyone.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3108687: Compatibility with/take advantage of code preloading in PHP 7.4, +#3050881: Use RecursiveExtensionFilterIterator in drupal_phpunit_find_extension_directories for performance
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

The timer bits are causing PHPCS to fail.

  13 | ERROR | [x] Namespaced classes/interfaces/traits should be
     |       |     referenced with use statements
  24 | ERROR | [x] Expected 1 blank line before function; 0 found
 230 | ERROR | [x] Namespaced classes/interfaces/traits should be
     |       |     referenced with use statements
vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
StatusFileSize
new 3223881-15.patch2.57 KB
new interdiff_3-15.txt700 bytes

Fixing PHPCS issues, Please have a look.

daffie’s picture

daffie commented
Status: Needs review » Needs work

Testbot is not happy.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
StatusFileSize
new interdiff_15-17.txt761 bytes
new 3223881-17.patch2.94 KB

Trying to please TestBot and addressing #6.

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new 3223881-18.patch1.91 KB
new interdiff_15-18.txt688 bytes

Sending any output to the console breaks the JSON output testing in Drupal\Tests\Scripts\TestSiteApplicationTest.

Since I wanted to see what the difference was between the times for bootstrapping with and without this patch, I took a stab at it here #3191612-47: [Ignore] In space (and/or this issue), no one can hear patches scream

Since the "timing-shizzle" isn't really required to get this patch in IMHO, I've included a patch here without the Timer to see if TestBot is happy with that.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review
daffie’s picture

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

All the code changes look good to me.
I have tested the speed change myself. There is a large speed improvement.
I do not see how we can get a test for this, other than a manual one.
For me it is RTBC.

For the committer: User @drumm also deserves a commit credit.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

There's an 'src' module. No code in it, but unless we introduce a list of reserved words for modules somehow I'm not sure we can do this.

https://www.drupal.org/project/src

catch credited drumm.

catch’s picture

catch
he/him
Primary language English
commented
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Reviewed & tested by the community

The src module is not discoverable in D8+

See https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C...

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Can we add an @see to RecursiveExtensionFilterIterator? Marking needs work for that.

Also I think it's worth exploring just using that directly, but could be a follow-up.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Can do - we had #3050881: Use RecursiveExtensionFilterIterator in drupal_phpunit_find_extension_directories for performance for that, which I closed in favour of this.
I'm happy to close this in favour of that if you'd prefer it - please advise.
With the approach here, there's more code to maintain, but we're not relying on code that were testing in our tests.
The approach there is closer to what core is doing, but as dawehner points out - we're using production code to bootstrap tests, to test production code so there's a bit of a circular dependency.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

@longwave pointed out that I basically duplicated this, see #3249443-10: drupal_phpunit_find_extension_directories() uses infinite recursion ⇒ more directories = slower tests.

I agree with @TR's assessment here: it's better to not duplicate code; it's better to use the same foundational discovery logic for finding extensions inside tests and outside tests.

But … I did have concerns around this too. Which is why I ended up with a hybrid approach: extract the absolute essentials out of RecursiveExtensionFilterIterator into RecursiveGenericExtensionFilterIterator. Make the original class extend the new class, and add the special stuff. Let the test use the extracted (simpler) class.

Currently awaiting test results at #3249443-11: drupal_phpunit_find_extension_directories() uses infinite recursion ⇒ more directories = slower tests, but it looks like this could be making Drupal core test runs ~8% faster.

I really wish I'd found this issue, then I'd have pushed this forward. But now it seems like the most pragmatic way forward here is for you to review what I proposed, @larowlan? I did uncover some facets/further improvements that are not yet mentioned here nor in #3050881: Use RecursiveExtensionFilterIterator in drupal_phpunit_find_extension_directories for performance 😇

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Closed (duplicate)

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