Updated: Comment #65

Problem/Motivation

Standard methods of running PHPUnit-based tests for contrib modules do not work.

PHPUnit finds all the test classes within /modules and /sites/*/modules, but none of the classes in those folders are loaded by the autoloader.

Subsequently, running any PHPUnit tests on contrib modules will fail when those tests attempt to load classes from namespaces that have not been registered (including the module's own). This goes for both the QA infrastructure on drupal.org as well as standard PHPUnit command line invocations (like cd core; ./vendor/bin/phpunit and IDE integrations.

Proposed resolution

Modify /core/tests/bootstrap.php so that it finds and registers all the relevant namespaces outside of core.

Remaining tasks

User interface changes

None

API changes

None

Original report by Mile23

I copied the file directory structure and namespacing conventions from Views into my contrib module.

I did the following:

cd core
./vendor/bin/phpunit

PHPUnit is unable to find my tests. I have no idea whether the autoloader is broken or I'm doing something wrong.

Is there a clear explanation somewhere for how to setup a PHPUnit-based test for a contrib module in Drupal 8?

BTW: There is no component setting for 'testing' in the issue system.

CommentFileSizeAuthor
#78 drupal-phpunit_bootstrap_contrib-2025883-78.patch4.09 KBiamEAP
#78 drupal-phpunit_bootstrap_contrib-2025883-78.interdiff.txt1.01 KBiamEAP
#72 drupal-phpunit_bootstrap_contrib-2025883-72.patch4.08 KBiamEAP
#65 drupal-phpunit_bootstrap_contrib-2025883-65.patch4.41 KBiamEAP
#65 drupal-phpunit_bootstrap_contrib-2025883-65.interdiff.txt1.73 KBiamEAP
#63 drupal-phpunit_bootstrap_contrib-2025883-63.patch4.51 KBiamEAP
#63 drupal-phpunit_bootstrap_contrib-2025883-63.interdiff.txt5.22 KBiamEAP
#57 drupal-phpunit_bootstrap_contrib-2025883-55.patch4.07 KBMile23
#57 drupal-phpunit_bootstrap_contrib-2025883-55.interdiff.txt576 bytesMile23
#52 drupal-phpunit_bootstrap_contrib-2025883-52.patch4.01 KBiamEAP
#52 drupal-phpunit_bootstrap_contrib-2025883-52.interdiff.txt1.47 KBiamEAP
#49 drupal-phpunit_bootstrap_contrib-2025883-49.patch3.93 KBBerdir
#49 drupal-phpunit_bootstrap_contrib-2025883-49-interdiff.txt1.37 KBBerdir
#47 drupal-phpunit_bootstrap_contrib-2025883-47.patch3.83 KBiamEAP
#47 drupal-phpunit_bootstrap_contrib-2025883-47.interdiff.txt528 bytesiamEAP
#43 drupal-phpunit_bootstrap_contrib-2025883-43.patch3.36 KBiamEAP
#41 drupal-phpunit_bootstrap_contrib-2025883-41.patch3.22 KBiamEAP
#41 drupal-phpunit_bootstrap_contrib-2025883-41.interdiff.txt1.81 KBiamEAP
#40 drupal-phpunit_bootstrap_contrib-2025883-39.patch2.7 KBiamEAP
#40 drupal-phpunit_bootstrap_contrib-2025883-39.interdiff.txt657 bytesiamEAP
#33 drupal-phpunit_bootstrap_contrib-2025883-33.patch2.85 KBmsonnabaum
#30 drupal-phpunit_bootstrap_contrib-2025883-30.patch3.29 KBiamEAP
#30 drupal-phpunit_bootstrap_contrib-2025883-30.interdiff.txt630 bytesiamEAP
#18 drupal_phpunit-autoload_verify-do-not-test.patch5.06 KBMile23
#14 drupal-phpunit_bootstrap_contrib-2025883-14.patch3.21 KBMile23
#9 drupal-phpunit_bootstrap_contrib-2025883-9.patch3.23 KBMile23
#5 drupal-phpunit_bootstrap_autoload_contrib-2025883-5.patch1.36 KBiamEAP
#3 2025883-phpunit_config-no-test.patch620 bytesMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

Title: How do you set up and run phpunit tests for contrib modules? » PHPUnit's bootstrap.php does not register module namespaces out of /core
Category: support » bug

yeap, that never occured before and we should definitely fix that cause it makes phpunit addition worthless for contrib :/

Mile23’s picture

Priority: Normal » Major
Issue tags: +qa

Any other issue out there that has traffic on this problem?

I'd really like to write a PHPUnit testing example for the Examples project...

Mile23’s picture

Issue tags: +PSR-0
FileSize
620 bytes

PHPUnit can find tests in contrib this way, but it's a horrible hack and doesn't solve the PSR-0 problem.

I think a real solution will be to write out a new classmap file on module install/disable.

iamEAP’s picture

I was able to have a test auto-loaded in contrib without a problem at... /modules/my_module/tests/Drupal/my_module/Tests/MyTest.php

iamEAP’s picture

Status: Active » Needs review
FileSize
1.36 KB

Nevermind, I understand now. How about something like this?

iamEAP’s picture

Priority: Major » Critical

And, yep, this is going to crop up in any contrib module looking to use PHPunit. For example: #2047321: Add PHPUnit tests for StatPluginMethodBase and a relevant test: https://qa.drupal.org/pifr/test/583293

Gonna go ahead and argue this is a critical bug, particularly because contrib modules that use unit tests cannot be upgraded.

dawehner’s picture

+++ b/core/tests/bootstrap.phpundefined
@@ -6,17 +6,20 @@
+foreach (array(__DIR__ . '/../modules', __DIR__ . '/../../modules') as $location) {

It feels like using https://drupal.org/node/2050289 would be way better, as it also covers things like modules in sites/foo/modules etc.

iamEAP’s picture

May be so, though phpunit.xml.dist only explicitly supports modules/foo and sites/foo/modules outside of core anyway. May be odd or undesirable to to load anything else.

Mile23’s picture

Title: PHPUnit's bootstrap.php does not register module namespaces out of /core » Drupal's PHPUnit bootstrap.php does not register module namespaces out of /core
FileSize
3.23 KB

For PHPUnit we want as few dependencies as possible. Using the Drupal YAML parser as per #2050289: Add class to make yaml discovery reusable is the opposite of that, particularly for the autoloader. Ideally, we want to run unit tests without loading *any* Drupal.

phpunit.xml.dist only tells PHPUnit which directories to scan for test cases. It's super easy to change this to meet our needs.

What has to happen is that core/tests/bootstrap.php has to autoload all of the classes in the Drupal installation, including contrib. To do this, we have to scan through all the folders to find directories to try and autoload.

And that's what this patch does. :-)

Unfortunately, it's not all that testable as it sits.

It assumes that any directory with a /lib subdirectory is a module, and then tells the loader to load that as a namespace.

Status: Needs review » Needs work

The last submitted patch, drupal-phpunit_bootstrap_contrib-2025883-9.patch, failed testing.

iamEAP’s picture

Aside from the fail, a few questions.

+++ b/core/phpunit.xml.distundefined
@@ -3,14 +3,15 @@
+    <testsuite name="Drupal Contributed Module Unit Test Suite">

Is there any benefit to breaking off contrib tests in their own test suite? Will testbot still be able to run them?

+++ b/core/tests/bootstrap.phpundefined
@@ -1,20 +1,55 @@
+function _drupal_phpunit_scan_autoload_directories($loader, $given_path) {

Do we care about polluting the global context with single-use procedural functions?

+++ b/core/tests/bootstrap.phpundefined
@@ -1,20 +1,55 @@
+$autoload_these_paths = array(

Ditto for global variable.

dawehner’s picture

For PHPUnit we want as few dependencies as possible. Using the Drupal YAML parser as per #2050289: Add class to make yaml discovery reusable is the opposite of that, particularly for the autoloader. Ideally, we want to run unit tests without loading *any* Drupal.

So you basically have nothing when it comes to phpunit?

Mile23’s picture

@iamEAP: "Is there any benefit to breaking off contrib tests in their own test suite?"

My thinking: If you only want to run contrib tests you'd be able to. PHPUnit has --group and a --filter arguments to run whichever suite you want. It might make sense to establish a naming convention for @group annotations, as an alternative. http://phpunit.de/manual/3.7/en/appendixes.annotations.html#appendixes.a...

@iamEAP: "Do we care about polluting the global context with single-use procedural functions [and variables]?"

This bootstrap is only loaded during testing. Obviously we want to minimize globals, but the recursive function needs to call itself, and the array seemed like it needed a little better readability. You're right to question it. The original bootstrap file also introduces the $loader global, which should probably be renamed something like $_bootstrap_phpunit_loader.

@dawehner: "So you basically have nothing when it comes to phpunit?"

Ideally, yes. You want PHPUnit tests to run very quickly and with as few dependencies as possible, so you can be in charge of how dependencies are met. And you want to be able to run them any time while you're developing, even (and especially) without a database. Also: How do you test the YAML parser if you depend on it for testing? :-)

Mile23’s picture

Status: Needs review » Needs work
FileSize
3.21 KB

Let's try this.

It has two global functions, but they encapsulate a bunch of variables that would otherwise be global.

I removed the changes to phpunit.xml.dist, because they were the ones causing the previous testbot failure.

Mile23’s picture

Status: Needs work » Needs review

..needs review.

Mile23’s picture

Issue summary: View changes

Added fact that I'm doing this from the command line.

Mile23’s picture

Updated issue summary.

dawehner’s picture

Status: Needs work » Needs review

Ideally, yes. You want PHPUnit tests to run very quickly and with as few dependencies as possible, so you can be in charge of how dependencies are met. And you want to be able to run them any time while you're developing, even (and especially) without a database. Also: How do you test the YAML parser if you depend on it for testing? :-)

This is fine when you test the class in the core case, on which the behavior of the discovery is not important as the core tests are not in a module. Additional we can't really write tests for this new code at all.

Mile23’s picture

This is a patch which demonstrates that the patch in #14 works.

It creates some namespaces in /module and /sites/all, and then runs a test which attempts to verify that those namespaces loaded.

How to:

Apply #14, then apply this one, then do:

cd core
./vendor/bin/phpunit

Does anyone have a way to mock this behavior, so the test doesn't require files to be added to those directories?

iamEAP’s picture

Tested #14 with the PHPUnit tests I've written for my D8 contrib module; worked like a charm there. I'm not sure we necessarily need tests for this, especially as more and more modules are ported to / written for D8; the test will be unnecessary as more modules rely on automated testing on d.o.

@mile23 adequately covered my concerns from #11 in #13; pursuing any further global variable/function cleanup would render the code unreadable. Further, doing so in this particular, purely test-based context, seems overly academic.

Given the above, #14 gets a +1 from me. A review or two more would be appropriate.

fubhy’s picture

Note: It also doesn't work for nested modules (e.g. /core/modules/foo_module/bar_module).

Mile23’s picture

Well #14 doesn't change the behavior for core modules, and it runs the same number of core tests as without the patch.

If bar_module is a module for testing foo_module, put it under foo_module/tests/. Look to Views for guidance.

If not.... The reason is in phpunit.xml.dist. The only place we're telling PHPUnit to look for core module tests to run is: <directory>./modules/*/tests/</directory>, which is appropriate. It recurses through that directory to find tests, which of course leaves out ./modules/*/bar_module/tests/.

I assume this setup was the result of a long discussion in a long-closed issue that I can't find. :-)

This is all different from the autoloader that's looking for namespaces so we can refer to classes within tests.

iamEAP’s picture

Issue tags: -PHPUnit, -qa, -PSR-0

Hmm... Seems problematic if, for example, Rules could run its unit tests, but rules_i18n, a submodule of Rules, could not.

iamEAP’s picture

Issue tags: +PHPUnit, +qa, +PSR-0

I have no idea why these tags were removed with that last comment.

Mile23’s picture

Rules isn't a core module.

Put it under ./modules/rules/ or ./sites/*/modules/rules.

In fact, that is exactly the use case this patch fixes, demonstrated by #18. I'm trying to write a phpunit_example module for the Examples project, which would be a submodule. (And I'll finish, once someone commits #14 :-)

iamEAP’s picture

Right, but I mean does #14 solve the case of unit tests for rules_i18n? e.g.

./modules/rules/rules_i18n or ./sites/*/modules/rules/rules_i18n

Mile23’s picture

It should solve that case. But apply the patch and try it out with your module setup and see.

iamEAP’s picture

Status: Needs review » Needs work

PHPUnit was unable to find ./modules/foo/foo_sub/tests/Drupal/foo_sub/Tests/FooSubTest.php. I don't know if that's explicitly supported, but I'm definitely able to do it at ./modules/foo/tests/Drupal/foo/Tests/FooTest.php.

Also, if we want to narrow down the definition a little bit for what a "module" is, at least in terms of autoloading, we may want to look for a foo.info.yml in addition to a lib directory.

iamEAP’s picture

Issue summary: View changes

Updated issue using issue summary template.

Mile23’s picture

Issue tags: -PHPUnit, -qa, -PSR-0

That's definitely a problem with misconfigured phpunit.xml.dist. I made this issue about it: #2056607: /core/phpunit.xml.dist only finds top-level contrib modules

Mile23’s picture

Issue tags: +PHPUnit, +qa, +PSR-0

Grr. Re-add tags.

iamEAP’s picture

Got it. #28 makes sense. Also verified ./modules/foo/foo_sub/lib/Drupal/foo_sub/FooSub.php is auto-loaded when running PHPUnit tests with #14.

Here's #14 with the extra check for the module.info.yml file.

Status: Needs review » Needs work

The last submitted patch, drupal-phpunit_bootstrap_contrib-2025883-30.patch, failed testing.

iamEAP’s picture

Status: Needs work » Needs review

Well, I'm embarrassed. I should really think before I patch... Sorry for the noise!

Let's go back to reviewing #14.

msonnabaum’s picture

I actually agree that we should be looking for info.yml, that's a much safer way to go about this.

I ended up just rewriting this, but it works in my testing and is a bit easier to grok IMO.

agentrickard’s picture

Works here; just need to cross-reference with #2056607: /core/phpunit.xml.dist only finds top-level contrib modules

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

#33 looks much better than mine. :-)

Also I can run ./vendor/bin/phpunit and it works, either with or without #2056607: /core/phpunit.xml.dist only finds top-level contrib modules patch #11.

iamEAP’s picture

Status: Reviewed & tested by the community » Needs work

Patch fails for me if I have a module symlink'd in the modules directory rather than actually there.

agentrickard’s picture

Why is that case this code's problem?

agentrickard’s picture

From iRC: "Common workflow for me when I'm working on contrib modules (as a workaround to having to deal with git submodules)."

Objection withdrawn.

dawehner’s picture

  1. +++ b/core/tests/bootstrap.php
    @@ -1,21 +1,66 @@
    +function drupal_phpunit_find_module_directories($scan_directory) {
    ...
    +function drupal_phpunit_find_site_module_directories($sites_path) {
    ...
    + * @param ComposerAutoloader $loader
    

    Yes nobody will ever use them but please let's also follow the documentation standards: http://drupal.org/node/1354 which means for example full namespace for parameters.

  2. +++ b/core/tests/bootstrap.php
    @@ -1,21 +1,66 @@
    +function drupal_phpunit_register_module_dirs($loader, $dirs) {
    

    If possible we should also typehint the loader here.

iamEAP’s picture

#33 + Symlinks.

iamEAP’s picture

iamEAP’s picture

We can't seem to reproduce the TestBot fail over here.

In the meantime...

@@ -1,21 +1,82 @@
+  $dir = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($scan_directory, RecursiveDirectoryIterator::FOLLOW_SYMLINKS));

Should be \RecursiveDirectoryIterator::FOLLOW_SYMLINKS

@@ -1,21 +1,82 @@
+    $lib_path = $dir . '/lib';
+    if (is_dir($lib_path)) {
+      $loader->add('Drupal\\' . basename($dir), $lib_path);
+    }

Mile23 says we should also be checking for and adding a /tests directory.

iamEAP’s picture

#41 + comments from #42

Status: Needs review » Needs work

The last submitted patch, drupal-phpunit_bootstrap_contrib-2025883-43.patch, failed testing.

Berdir’s picture

./modules/simpletest/tests/modules/phpunit_test doesn't have a info.yml, I don't think that can work anywhere? :)

msonnabaum’s picture

Right, I had a blank file in my patch. It just needs to be readded (maybe with some content…).

iamEAP’s picture

Whoops, did not notice that when I was patching. Added back here + some .info.yml content.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Woot.

#47 works with or without #2056607: /core/phpunit.xml.dist only finds top-level contrib modules #11, finding the namespaces I care about, and probably other ones, as well. Applying the patch from that other issue yields PHPUnit contrib happiness.

Thanks, iamEAP and everyone.

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.37 KB
3.93 KB

The patch still makes the same wrong assumption that directory name = module name as The YamlDiscovery class.. The .info.yml filename defines the module name, not the directory. See #2067809: YamlDiscovery uses basename of directory instead of module name. The directory rename there results in an explosion of the unit tests.

The attached patch fixes this but doesn't add explicit tests for it. Not sure if we have to, as the other issue will take care of that.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

nice catch, thanks

Mile23’s picture

+1 on RTBC.

It still finds my contrib modules.

iamEAP’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.47 KB
4.01 KB

Looks good. Some tiny documentation nitpicks: this patch is just adding those changes in #49 to the inline documentation. Purely documentation.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Yay specific documentation.

catch’s picture

Status: Reviewed & tested by the community » Needs review

It doesn't look like this handles modules inside the profiles folder - should that be added as well?

iamEAP’s picture

Status: Needs review » Needs work

Hum... I suppose it should.

Mile23’s picture

On it. How about themes?

Mile23’s picture

Added /profiles and /themes.

ParisLiakos’s picture

i dont think themes need it..if a theme needs unit tests...well you are doing something wrong..

iamEAP’s picture

Status: Needs review » Needs work

Going off of this doc: https://drupal.org/node/306267

We would also have to add support for sites/*/profiles/*/modules and sites/*/profiles/*/themes.

iamEAP’s picture

And re: #58, if core registers / autoloads classes from themes, I don't think we should prevent theme maintainers from writing unit tests for those classes, even if we can't think of legitimate use-cases.

Mile23’s picture

re: #59: I think if we start the recursion down sites/* it will find sites/profiles/*/modules and */themes. (Which is how #57 and antecedents work.)

And +1 on #60. If autoloading gets super-slow, then revisit the decision.

iamEAP’s picture

@Mile23: currently, we aren't actually looking recursively down sites/*, we only look for directories of the form sites/*/modules, and only then search each of those paths recursively for *.info.yml files.

iamEAP’s picture

Here's something of a radical departure from #57.

Dumping the word "module" for "extension," since we're now autoloading classes for themes and profiles. Also, if we're autoloading contrib themes/profiles, we should autoload core themes/profiles as well.

Taking advantage of scandir()'s ".." item to catch the root /modules, /profiles, /themes directories.

catch’s picture

Status: Needs review » Needs work

Please don't add themes here, that needs its own issue at the very least.

iamEAP’s picture

I might argue that, since theme classes are already auto-loaded by Core, the deficiency/bug for modules being addressed here is the exact same for themes, and thus, a secondary issue may be redundant.

That being said, I care more about the ability to upgrade/port my modules to D8 and have the unit tests actually work and run on d.o instead of pushing on that idea. As such, I'll open up a follow-up for further discussion.

In the meantime, here's #63 - themes.

iamEAP’s picture

Issue summary: View changes

Added reference to #2056607

ParisLiakos’s picture

opened #2070581: Themes should not register namespaces for stop registering namespaces for themes

Mile23’s picture

+++ b/core/tests/bootstrap.phpundefined
@@ -1,23 +1,90 @@
+$loader->add('Drupal\Core', __DIR__ . '/../../core/lib');
+$loader->add('Drupal\Component', __DIR__ . '/../../core/lib');

It turns out Composer does this for us, thanks to composer.json and /vendor/autoload.php. Probably faster.

Eric_A’s picture

+/**
+ * Finds all valid extension directories recursively within a given directory.
+ *
+ * @param string $scan_directory
+ *   The directory that should be recursively scanned.
+ * @return array
+ *   An associative array of extension directories found within the scanned
+ *   directory, keyed by extension name.
+ */
+function drupal_phpunit_find_extension_directories($scan_directory) {
+  $extensions = array();
+  $dir = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($scan_directory, \RecursiveDirectoryIterator::FOLLOW_SYMLINKS));
+  foreach ($dir as $d) {
+    if (strpos($d->getPathname(), 'info.yml') !== FALSE) {
+      // Cut off ".info.yml" from the filename for use as the extension name.
+      $extensions[substr($d->getFilename(), 0, -9)] = $d->getPathInfo()->getRealPath();
+    }
+  }
+
+  return $extensions;
+}

This will scan '/^(CVS|lib|templates|css|js)$/'.

Should we not just copy code from SystemListing::scanDirectory / file_scan_directory?

iamEAP’s picture

This will scan '/^(CVS|lib|templates|css|js)$/'.

I think that's at least partially intentional; there's nothing stopping you from throwing a sub-module in a directory by any of those names.

sdboyer’s picture

Status: Needs review » Needs work
Issue tags: +PHPUnit, +autoloader, +qa, +PSR-0

The last submitted patch, drupal-phpunit_bootstrap_contrib-2025883-65.patch, failed testing.

iamEAP’s picture

Status: Needs work » Needs review
FileSize
4.08 KB

Re-roll of #65.

iamEAP’s picture

mmilano’s picture

I was trying to get a custom module's tests working with the latest commit: b13a82c (as of 9/20/13 11:30pm pdt)

Module location: modules/mymodule

I was having a hard time finding docs, so I wasn't sure what the standard directory structure was for tests.

My initial thought was: modules/mymodule/lib/Drupal/mymodule/Tests

My tests did not pick up with or without patch #72.

After poking around core more, I moved my tests to: modules/mymodule/tests/Drupal/mymodule/Tests

This worked, both with and without patch #72.

Those results make me think I'm testing this issue wrong, but I am happy to be running tests on my module now. :)

donquixote’s picture

#11, #13

@iamEAP: "Do we care about polluting the global context with single-use procedural functions [and variables]?"

This bootstrap is only loaded during testing. Obviously we want to minimize globals, but the recursive function needs to call itself, and the array seemed like it needed a little better readability. You're right to question it. The original bootstrap file also introduces the $loader global, which should probably be renamed something like $_bootstrap_phpunit_loader.

#18

@mile23 adequately covered my concerns from #11 in #13; pursuing any further global variable/function cleanup would render the code unreadable. Further, doing so in this particular, purely test-based context, seems overly academic.

What about moving some of this code into a static class method somewhere in Drupal\\Core\\.. ?
These are already available for autoload as soon as we include core/vendor/autoload.php.
And I don't think it is going to be that unreadable.

It would be similar to the AutoloaderInit class generated by Composer.

Another option would be to wrap this procedural code into a namespace.
(something which we don't do anywhere else, so maybe we won't do it here either)

Mile23’s picture

tstoeckler’s picture

Tried the patch and it works.

Code looks good as well.

If we don't want to pollute the global namespace, maybe we could just use local functions. I.e.

$directory_scanner = function($directory) {
  ...
}

and so forth. What do you guys think?

Other than that I only found one small nitpick:

+++ b/core/tests/bootstrap.php
@@ -1,20 +1,87 @@
+  foreach ($dir as $d) {

$d is not very descriptive.

iamEAP’s picture

Thanks for the review @tstoeckler! I have a feeling code readability could suffer were we to reorganize the code in such a way that we reduced net global namespace pollution to zero. See #13 for what I believe is a satisfactory answer to similar concerns I had originally. Also note the vast readability improvement @msonnabaum introduced in #33 that I think we might lose were we to go down that rabbit hole.

Regarding the second point, duly noted! I've attached a patch to address this.

Regarding #76 @Mile23, I've subscribed to those issues, though I don't think any of them should necessarily stop this one from getting in. I think it's better to start supporting PHPUnit tests in contrib as early as possible and iterate/refactor as needed than wait much longer. Would love to get this RTBC ASAP.

Status: Needs review » Needs work
Issue tags: -PHPUnit, -autoloader, -qa, -PSR-0

The last submitted patch, drupal-phpunit_bootstrap_contrib-2025883-78.patch, failed testing.

iamEAP’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit, +autoloader, +qa, +PSR-0
Mile23’s picture

Agreed on iteration.

Applies cleanly for me and PHPUnit is happy with it from the command line. I'll RTBC when the test passes, and then we can direct our attention to #2056607: /core/phpunit.xml.dist only finds top-level contrib modules

It would be great if there were a comprehensive way to test it, but #2095037: Add vfsStream as a dependency in composer.json. looks like a very remote possibility.

Eric_A’s picture

I think that's at least partially intentional; there's nothing stopping you from throwing a sub-module in a directory by any of those names.

@iamEAP, can you elaborate a little more? The impression I got from looking at SystemListing::scanDirectory / file_scan_directory is that those names will just not work.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

PHPUnit's bootstrap has two tasks:

1) Load all the namespaces that can be tested by PHPUnit.

2) Load all the namespaces that are needed to complete the test. This doesn't include the PHPUnit test case classes themselves, since PHPUnit will find and load them anyway, but there's no need to work very hard to exclude them.

The faster and more flexibly those two things can happen, the better.

PHPUnit doesn't really care about the structure of modules or other Drupalisms; PHPUnit only wants namespaces. The reason we care about the structure of modules is so we can find the PSR file structures. It's unlikely that someone has an .info.yml file alongside a lib/ directory inside a css/ directory. But if it's there, we should go ahead and load it, because that's a PSR file structure, and that's the kind of thing we're tasked with loading as per #1 above.

Also, copypasta from SystemListing would likely need to be changed anyway after #2083547: PSR-4: Putting it all together

<bobfossil>And that's why I RTBC.</bobfossil>

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f995135 and pushed to 8.x. Thanks!

Mile23’s picture

Yay! Thanks alexpott. :-)

Now, about that #2056607: /core/phpunit.xml.dist only finds top-level contrib modules .....

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Cleaning up issue and noting the new related issue about registering and autoloading theme namespaces.