Give the possibility to ignore directories when collecting extensions during test runs

Setting to "needs review" to run the tests.

This is a pretty clean solution for the bug.

Nice, this looks good to me.

It's also working for me, thanks !

This still needs tests.

I created a test for this but I cannot replicate the problem locally. I purposefully introduced a circular dependency in a module but this seems to be dealt with inside PHP. It appears to stop recursing after reaching a certain depth. Memory consumption stays within reasonable limits (4MB). The test takes a lot longer to complete though. Note that I only ran KernelTestBaseTest, it is possible that some other test might fail due to this circular dependency.

Seems the circular references have consequences in other places too :D

It's probably not the best idea to introduce a circular reference inside Drupal core. There might always be some kind of tool used in a deployment procedure that cannot handle them correctly. We should probably generate it during the test, and remove it again in the teardown.

So this issue is a feature request that proposes to add the possibility to filter out extension directories during tests. I think the sole reason to ever want to actually filter out directories is to avoid infinite recursion caused by circular references. This is also the reason mentioned in the issue description.

I think we can approach this from a completely different angle. If we can somehow avoid to follow circular dependencies then this doesn't need manual blacklisting of folders that contain (or are referenced by) the circular symlinks. That means this can become a bugfix.

I've attached an initial idea I had to filter out the circular dependencies by filtering them using a RecursiveFilterIterator. This doesn't work, it is too late in the process, the infinite recursion will still take place and will then be infinitely filtered by this class. In reality (like I mentioned in my previous comment) on my system this recurses quite deep but then stops at some point. There is no infinite recursion taking place. I don't know why but it might be due to running PHP 7.2 or having xdebug enabled.

So instead of filtering we can try to extend the RecursiveDirectoryIterator directly and catch the recursion as it takes place.

This approach seems to work. It obviously needs to be cleaned up. No interdiff since this is a new approach.

@ademarco, @bircher, @pol would this approach be OK to solve the problem without having to manually blacklist paths?

If yes, should we repurpose this issue as a bug report, or create a new issue?

Cleaning round.

OK this needs some work on the tests. I am not sure how we can test this effectively. My best guess is to create the symlink during the test setup and then remove it again on teardown.

I was intrigued that the infinite recursion by following the circular symlinks was interrupted on my system after reaching a certain depth. It appears this is a protection built in to the linux kernel itself. It has a hardcoded limit of 40 levels after which it won't go any deeper. Ref. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/...

Updating the patch.

Updating the second patch.

We have two very different approaches to fix this issue.

@ademarco:
Patch: https://www.drupal.org/files/issues/2018-07-05/2943172-kernel-test-base-...
It tackles the issue by ensuring the 'file_scan_ignore_directories' setting is obeyed.
It's a direct approach to the issue and it goes in the right direction because this setting should be held in consideration when running tests.
We have a test for 'file_scan_ignore_directories' in core/tests/Drupal/KernelTests/Core/File/ScanDirectoryTest.php but it's only testing mocked settings, we could improve this test.

@pfrenssen
Patch: https://www.drupal.org/files/issues/2018-07-05/2943172-22.patch
It ensures that we avoid infinite loops by using an alternative to the PHP native class 'RecursiveDirectoryIterator'. This has some drawbacks in my opinion:

1. Setting file_scan_ignore_directories setting will still be ignored in PHPUnit tests
2. We're adding an helper class 'CircularReferenceResistantRecursiveDirectoryIterator' that is not a Drupal specific thing. I feel we're fixing a problem with the design of 'RecursiveDirectoryIterator' and 'FilesystemIterator' - the later accepts the FOLLOW_SYMLINKS option in which case we may always fall into infinite loops. This is a problem that may be tackled in PHP or in a PHP library that we could use. Addressing this problem inside Drupal doesn't seem optimal.
3. Testing, like @pfrenssen mentioned, is not very straightforward.

My preference goes to @ademarco's approach. It's necessary anyway, it's simplest and easier to test.

BTW I noticed that my approach is not 100% reliable. I would like to believe the approach is sound but unfortunately I had some cases were the recursion still happened on different environments. I am not sure what causes it. I had a look at how this is implemented in PHP and it appears there has been quite some churn and bug fixing going on in the iterators in the recent history (ref. https://github.com/php/php-src/commits/master/ext/spl/spl_iterators.c) so it is possible that this works differently depending on the PHP version.

I have no more time to look into this so feel free to ignore my patch and continue with the approach proposed by @ademarco.

I'm now uploading a patch that includes a test in KernelTestBaseTest, covering the setting "SIMPLETEST_IGNORE_DIRECTORIES".

This will need to be against 8.7.x since this is a feature request. Assigning to me for review. I'm assuming it is OK to take over the assigned status from @ademarco since he hasn't been active here for half a year :)

1. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -495,6 +495,12 @@ abstract class KernelTestBase extends TestCase implements ServiceProviderInterfa
   +    // Set directories to be ignored during filesystem scanning.
   +    $settings = Settings::getAll();
   +    $settings['file_scan_ignore_directories'] = explode(',', getenv('SIMPLETEST_IGNORE_DIRECTORIES'));
   +    new Settings($settings);
   

   This doesn't seem to be the right location to set this setting. This method is supposed to be only returning the extensions for modules. Or it would have been called getExtensionsForModulesAndApplyIgnoreDirectoriesSetting().

   I would move this to the start of the bootKernel() method where the other settings are being applied. We can also use the built in method ::setSetting().

2. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -317,4 +317,22 @@ class KernelTestBaseTest extends KernelTestBase {
   +  /**
   +   * Tests if getExtensionsForModules() obeys
   +   * 'file_scan_ignore_directories' setting.
   +   *
   +   * @covers ::getExtensionsForModules
   +   */
   +  public function testGetExtensionsForModules() {
   +
   +    putenv('SIMPLETEST_IGNORE_DIRECTORIES=user');
   +
   +    $this->expectException(\PHPUnit\Framework\Exception::class);
   +    $this->expectExceptionMessageRegExp('/Unavailable module: \'user\'/');
   +
   +    $reflector = new \ReflectionObject($this);
   +    $method = $reflector->getMethod('getExtensionsForModules');
   +    $method->setAccessible(true);
   +    $method->invoke($this, ['user']);
   +  }
   

   I think it is OK to test it like this, since the getExtensionsForModules() method is responsible for retrieving the extensions. I also think it is OK to make this private method public for testing purposes since this is a very practical way to validate that the problem is fixed without having to refactor KernelTestBase.

   However I have a few remarks about the test:

   1. According to coding standards the first sentence of the docblock should fit on one line.
   2. It seems a bit premature to add the annotation @covers ::getExtensionsForModules() since this test does not in fact provide full coverage for the functionality offered by that method.
   3. I wouldn't set the environment variable in the test itself, this should be done in the test setUp(). This would require moving this to a dedicated class though.
   4. We should also test that setting file_scan_ignore_directories works. Now it only tests the environment variable.

3. +++ b/core/tests/bootstrap.php
   @@ -22,7 +22,13 @@ use PHPUnit\Runner\Version;
   +  $ignore_directories = explode(',', getenv('SIMPLETEST_IGNORE_DIRECTORIES'));
   

   This is using the environment variable directly instead of reading file_scan_ignore_directories. The environment variable is nice to have, but the main problem as described in the issue summary is that the file_scan_ignore_directories setting is ignored. It still seems to be ignored.

This doesn't seem to be the right location to set this setting. This method is supposed to be only returning the extensions for modules. Or it would have been called getExtensionsForModulesAndApplyIgnoreDirectoriesSetting().

I would move this to the start of the bootKernel() method where the other settings are being applied. We can also use the built in method ::setSetting().

Commenting on my own comment. After giving it some more thought, I am thinking that the right place to read the environment variable and adding it to `file_scan_ignore_directories` is probably in the test bootstrap, before drupal_phpunit_populate_class_loader() is called. This will ensure that this setting will be respected as early as possible.

@pfrenssen thanks for your comments with which I mostly agree.

But I still have some doubts to put here, about third point in #30 and #31.

In /core/tests/bootstrap.php we're not using Drupal\Core\Site\Settings yet. We're still in an early stage of the all process. While I don't see any major problem about starting the Settings here, it seems a big change. Do we really want to go on with that?!

Anyway, drupal_phpunit_find_extension_directories is using the PHP method RecursiveDirectoryIterator so we would still need to configure the filter as it's being done now, the only change would be to read the configuration from the Settings instead of the getenv(), it's a theoretic nuance with no practical impacts.

I would still stick to the approach that we have. You say:

main problem as described in the issue summary is that the file_scan_ignore_directories setting is ignored. It still seems to be ignored.

Well the main problem is that we can't ignore some directories. While file_scan_ignore_directories is a useful setting we can use in KernelTestBase::bootKernel() , in other places we can still simply use the new SIMPLETEST_IGNORE_DIRECTORIES setting.
SIMPLETEST_IGNORE_DIRECTORIES doesn't need to be only used once to set file_scan_ignore_directories. This doesn't go against the vision we had since first moment.

Of course, it would be perfect to have file_scan_ignore_directories set with the value in SIMPLETEST_IGNORE_DIRECTORIES and just use that everywhere. But defining Settings in bootstrap.php seems a big price for this.

I'm now submitting a patch that covers remarks 1 and 2 in #30 from @pfrenssen.

I don't think the current approach is ideal. The goal of this issue is to make sure the file_scan_ignore_directories setting is respected when running tests. The latest patch doesn't fully achieve this, and tries to work around it by introducing a second setting in phpunit.xml. This means we will have two different entry points for ignoring paths. And since one of them is only half working, this will force people to copy any settings they have for file_scan_ignore_directories into SIMPLETEST_IGNORE_DIRECTORIES.

I think the right solution for this problem is to forget about SIMPLETEST_IGNORE_DIRECTORIES completely and just make sure it works with file_scan_ignore_directories in settings.php. If this requires to refactor the test bootstrap so that the discovery of files is done after the settings are initialized, then this is probably the right path to take.

#34 +1, but in addition, it would be great if ignored paths still could be manipulated with an environment variable as well. It could be super useful in Docker based setups.

Can't we just retrieve the list of paths that should be excluded from .gitignore files? We would need a single source of truth and for me, it seems reasonable to re-use .gitignore for this.

drupal/core's .gitignore already excludes node_modules https://github.com/drupal/core/blob/8.7.5/.gitignore#L3 although it does not exclude bower_components that the default.settings.php does https://github.com/drupal/drupal/blob/8.7.5/sites/default/default.settin...

My idea is to merge drupal/core's .gitignore with the root project's .gitignore to get a list of directories that should be excluded both from test discovery and file scanning.

... and there is another place where the list of excluded directories has to be kept in sync manually, the phpcs.xml.dist: https://github.com/drupal/core/blob/8.7.5/phpcs.xml.dist#L12-L16

PHPCS does not support yet to dynamically register excluded paths, but it could be added quite easily, see: https://github.com/squizlabs/PHP_CodeSniffer/issues/2242

Meh, almost forget about that project-level .gitignores usually excludes contrib modules/themes, so we cannot use the blacklist from .gitignore because contrib modules would be discovered by neither test nor extension discovery in that case.

POC patch that bootstraps Drupal for PHPUnit to retrieve the list of file_scan_ignore_directories and exclude them from discovery. Feedbacks are warmly welcomed, probably the current solution could be still optimized.

I did not use the test from #33 because as I saw drupal_phpunit_find_extension_directories() do not get called for kernel tests by drupal_phpunit_populate_class_loader(). So this solution still requires a test coverage.

Removed composer.lock changes from #42.

8.7.x backport

Okay, so I had a realization that you can only add directory names to file_scan_ignore_directories, you cannot add relative paths or path fragments because this is what \Drupal\Core\Extension\Discovery\RecursiveExtensionFilterIterator accepts.

I also bumped into #3050881 that suggests to use RecursiveExtensionFilterIterator to discover tests files in the bootstrap file. Well, the problem with using just the RecursiveExtensionFilterIterator is that it also ignores settings in file_scan_ignore_directories. Although, if we would use the \Drupal\Core\Extension\ExtensionDiscovery to discover directories that could combine the best from both solutions.

Here is a WIP patch for 8.7.x branch that boots Drupal and uses ExtensionDiscovery to discover tests files. Even if it works we have to figure out how to deprecate some functions properly. Let's see what fails...

Okay, next try.

Failed tests:

1) Drupal\Tests\Core\Command\QuickStartTest::testQuickStartCommand
PHPUnit\Framework\Exception: PHP Fatal error:  Cannot redeclare _drupal_shutdown_function_handle_exception() (previously declared in /var/www/html/core/includes/bootstrap.inc:1072) in /var/www/html/core/includes/bootstrap.inc on line 1072

I had to remove an additional include from the related test file for this. The new bootstrap file already includes bootstrap.inc.

1) Drupal\Tests\Core\Extension\ExtensionSerializationTest::testServiceAppRouteUsage
Constant DRUPAL_ROOT is defined.

Well, I have checked git history and I think this assert can be removed because if we remove it the test case will still remain valid.

1) Drupal\Tests\Core\Session\AccountProxyTest::testId
Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

and

1) Drupal\Tests\Core\Session\AccountProxyTest::testSetInitialAccountIdException
Failed asserting that exception of type "Drupal\Core\DependencyInjection\ContainerNotInitializedException" matches expected exception "LogicException". Message was: "\Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container." at

These were the most interesting ones. The problem caused by that drupal_get_user_timezone() did not check whether the container was actually initialized before it tried to retrieve the current user or the config.

1) Drupal\Tests\field_layout\FunctionalJavascript\FieldLayoutTest::testChangingFormatterAndRegion
WebDriver\Exception\CurlExec: Curl error thrown for http POST to http://chromedriver-jenkins-drupal-patches-4289:9515/session/b22700bd3c751cac4f33a5a03cd7afc0/execute with params: {"script":"(function (element) {\n    var event = document.createEvent(\"HTMLEvents\");\n\n    event.initEvent(\"drop\", true, true);\n    event.dataTransfer = {};\n\n    element.dispatchEvent(event);\n}(arguments[0]));","args":[{"ELEMENT":"0.07795512151426687-5"}]}

Well, I guess this has not been caused by my changes.

Added deprecation notices since #47, also here is a 8.8.x patch as well.

Again, it could happen that the container is not initialized when a static service gets used...

Okay, so for both versions ./scripts/test-site.php failed because Drupal\Core\Session\AccountInterface has not been found. This could have been fixed by adding require_once __DIR__ . '/../../autoload.php'; to the file. I saw you made an attempt to avoid this in #2926633 but I could not find a better solution. Open for ideas.

For 8.8.x, the DrupalGetProfileLegacyTest failed because another include $this->root . '/core/includes/bootstrap.inc'; had to be removed from the test.

interdiff_49_52.diff contains the Drupal 8.7.x changes the interdiff_50_52.diff contains the Drupal 8.8.x changes.

One small tweak that is required to solve the actual task we are trying to solve here.

1. +++ b/core/includes/bootstrap.inc
   @@ -566,18 +568,24 @@ function drupal_get_messages($type = NULL, $clear_queue = TRUE) {
   +  catch (ContainerNotInitializedException $e) {
   
   @@ -807,7 +815,12 @@ function drupal_get_profile() {
   +      catch (ConnectionNotDefinedException $e) {
   
   +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -228,7 +229,12 @@ public function scan($type, $include_tests = NULL) {
   +    catch (ContainerNotInitializedException $e) {
   

   these indicate an issue in the test, we shouldn't be changing the runtime code for tests, in these cases typically we setup a mock container in the test

2. +++ b/core/tests/bootstrap.php
   @@ -7,31 +7,49 @@
   +require_once __DIR__ . '/../includes/bootstrap.inc';
   

   I don't think we should be doing this, this pollutes the global scope during bootstrap. Those functions are loaded into memory and can't be unloaded.

3. +++ b/core/tests/bootstrap.php
   @@ -7,31 +7,49 @@
   +  $class_loader = require __DIR__ . '/../../autoload.php';
   +  // Settings must be initialized otherwise
   +  // Settings::get('file_scan_ignore_directories', []) returns an empty array
   +  // in ExtensionDiscovery::scanDirectory().
   +  Settings::initialize($root, 'sites/default', $class_loader);
   

   same, I don't think we should have settings populated in unit tests, this is the wrong approach in my opinion

4. +++ b/core/tests/bootstrap.php
   @@ -7,31 +7,49 @@
   +  $discovery = new ExtensionDiscovery($root, FALSE);
   

   I think this suffers from the same problem @dawehner highlighted in #3050881: Use RecursiveExtensionFilterIterator in drupal_phpunit_find_extension_directories for performance - we're using code under test to drive the tests, which could lead to issues

In my opinion, the solution for your custom projects here is to write your own bootstrap.php, we do the same at PreviousNext, e.g. ours includes elements from #3050881: Use RecursiveExtensionFilterIterator in drupal_phpunit_find_extension_directories for performance but also completely skips scanning contrib and core tests, because we only want to run project-specific tests. You can easily swap this in by using your own phpunit.xml and changing the bootsrap entry.

I'll ping @dawehner and @lendude (phpunit initiative maintainers) for a review.

I agree with @larowlan that doesn't feel like the right solution for core. If the changes in #53 are indeed what is needed to support this feature, then they feel too invasive and we shouldn't do it. Like @larowlan and @ademarco pointed out, there is an option to get this to work using a custom bootstrap.php, which is admittedly less obvious then using an existing setting, but perfectly workable I think (so this is really a DX feature).

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%?

The current proposed fix also doesn't line up with the IS, so that needs an update (I spent some time looking for where the env var was being used, before figuring out it isn't used).

Also, this changes a lot of tests, but doesn't add any coverage.

Thanks for the feedbacks and reviews, I share some of your concerns and I'll provide detailed answers to those tomorrow. I'll also try to update the description to describe in a better way the core problem that we are trying to solve here.
Even if the ExtensionDiscovery + file_scan_ignore_directories approach will not be the one that solves this problem or Drupal core won't support this development model we need a solution now and I see we are not the only ones, so I am leaving this updated patch here for now.

Following the work on #33, would it not be possible to set file_scan_ignore_directories during environment setup? I attach a patch and interdiff with an example

Fixed CS issues.