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
Related Issues
- #2056607: /core/phpunit.xml.dist only finds top-level contrib modules
- #2070559: Harmonize PHPUnit and run-tests to both scan for themes.
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.
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedyeap, that never occured before and we should definitely fix that cause it makes phpunit addition worthless for contrib :/
Comment #2
Mile23Any other issue out there that has traffic on this problem?
I'd really like to write a PHPUnit testing example for the Examples project...
Comment #3
Mile23PHPUnit 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.
Comment #4
iamEAP CreditAttribution: iamEAP commentedI was able to have a test auto-loaded in contrib without a problem at... /modules/my_module/tests/Drupal/my_module/Tests/MyTest.php
Comment #5
iamEAP CreditAttribution: iamEAP commentedNevermind, I understand now. How about something like this?
Comment #6
iamEAP CreditAttribution: iamEAP commentedAnd, 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.
Comment #7
dawehnerIt feels like using https://drupal.org/node/2050289 would be way better, as it also covers things like modules in sites/foo/modules etc.
Comment #8
iamEAP CreditAttribution: iamEAP commentedMay be so, though phpunit.xml.dist only explicitly supports
modules/foo
andsites/foo/modules
outside of core anyway. May be odd or undesirable to to load anything else.Comment #9
Mile23For 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.
Comment #11
iamEAP CreditAttribution: iamEAP commentedAside from the fail, a few questions.
Is there any benefit to breaking off contrib tests in their own test suite? Will testbot still be able to run them?
Do we care about polluting the global context with single-use procedural functions?
Ditto for global variable.
Comment #12
dawehnerSo you basically have nothing when it comes to phpunit?
Comment #13
Mile23@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? :-)
Comment #14
Mile23Let'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.
Comment #15
Mile23..needs review.
Comment #15.0
Mile23Added fact that I'm doing this from the command line.
Comment #16
Mile23Updated issue summary.
Comment #17
dawehnerThis 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.
Comment #18
Mile23This 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:
Does anyone have a way to mock this behavior, so the test doesn't require files to be added to those directories?
Comment #19
iamEAP CreditAttribution: iamEAP commentedTested #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.
Comment #20
fubhy CreditAttribution: fubhy commentedNote: It also doesn't work for nested modules (e.g. /core/modules/foo_module/bar_module).
Comment #21
Mile23Well #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.
Comment #22
iamEAP CreditAttribution: iamEAP commentedHmm... Seems problematic if, for example, Rules could run its unit tests, but rules_i18n, a submodule of Rules, could not.
Comment #23
iamEAP CreditAttribution: iamEAP commentedI have no idea why these tags were removed with that last comment.
Comment #24
Mile23Rules 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 :-)
Comment #25
iamEAP CreditAttribution: iamEAP commentedRight, 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
Comment #26
Mile23It should solve that case. But apply the patch and try it out with your module setup and see.
Comment #27
iamEAP CreditAttribution: iamEAP commentedPHPUnit 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.
Comment #27.0
iamEAP CreditAttribution: iamEAP commentedUpdated issue using issue summary template.
Comment #28
Mile23That'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
Comment #29
Mile23Grr. Re-add tags.
Comment #30
iamEAP CreditAttribution: iamEAP commentedGot 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.
Comment #32
iamEAP CreditAttribution: iamEAP commentedWell, I'm embarrassed. I should really think before I patch... Sorry for the noise!
Let's go back to reviewing #14.
Comment #33
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #34
agentrickardWorks here; just need to cross-reference with #2056607: /core/phpunit.xml.dist only finds top-level contrib modules
Comment #35
Mile23#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.Comment #36
iamEAP CreditAttribution: iamEAP commentedPatch fails for me if I have a module symlink'd in the modules directory rather than actually there.
Comment #37
agentrickardWhy is that case this code's problem?
Comment #38
agentrickardFrom iRC: "Common workflow for me when I'm working on contrib modules (as a workaround to having to deal with git submodules)."
Objection withdrawn.
Comment #39
dawehnerYes 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.
If possible we should also typehint the loader here.
Comment #40
iamEAP CreditAttribution: iamEAP commented#33 + Symlinks.
Comment #41
iamEAP CreditAttribution: iamEAP commented#40 + docs and typehinting.
Comment #42
iamEAP CreditAttribution: iamEAP commentedWe can't seem to reproduce the TestBot fail over here.
In the meantime...
Should be \RecursiveDirectoryIterator::FOLLOW_SYMLINKS
Mile23 says we should also be checking for and adding a /tests directory.
Comment #43
iamEAP CreditAttribution: iamEAP commented#41 + comments from #42
Comment #45
Berdir./modules/simpletest/tests/modules/phpunit_test doesn't have a info.yml, I don't think that can work anywhere? :)
Comment #46
msonnabaum CreditAttribution: msonnabaum commentedRight, I had a blank file in my patch. It just needs to be readded (maybe with some content…).
Comment #47
iamEAP CreditAttribution: iamEAP commentedWhoops, did not notice that when I was patching. Added back here + some .info.yml content.
Comment #48
Mile23Woot.
#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.
Comment #49
BerdirThe 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.
Comment #50
ParisLiakos CreditAttribution: ParisLiakos commentednice catch, thanks
Comment #51
Mile23+1 on RTBC.
It still finds my contrib modules.
Comment #52
iamEAP CreditAttribution: iamEAP commentedLooks good. Some tiny documentation nitpicks: this patch is just adding those changes in #49 to the inline documentation. Purely documentation.
Comment #53
Mile23Yay specific documentation.
Comment #54
catchIt doesn't look like this handles modules inside the profiles folder - should that be added as well?
Comment #55
iamEAP CreditAttribution: iamEAP commentedHum... I suppose it should.
Comment #56
Mile23On it. How about themes?
Comment #57
Mile23Added /profiles and /themes.
Comment #58
ParisLiakos CreditAttribution: ParisLiakos commentedi dont think themes need it..if a theme needs unit tests...well you are doing something wrong..
Comment #59
iamEAP CreditAttribution: iamEAP commentedGoing off of this doc: https://drupal.org/node/306267
We would also have to add support for
sites/*/profiles/*/modules
andsites/*/profiles/*/themes
.Comment #60
iamEAP CreditAttribution: iamEAP commentedAnd 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.
Comment #61
Mile23re: #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.
Comment #62
iamEAP CreditAttribution: iamEAP commented@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.Comment #63
iamEAP CreditAttribution: iamEAP commentedHere'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.
Comment #64
catchPlease don't add themes here, that needs its own issue at the very least.
Comment #65
iamEAP CreditAttribution: iamEAP commentedI 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.
Comment #65.0
iamEAP CreditAttribution: iamEAP commentedAdded reference to #2056607
Comment #66
ParisLiakos CreditAttribution: ParisLiakos commentedopened #2070581: Themes should not register namespaces for stop registering namespaces for themes
Comment #67
Mile23It turns out Composer does this for us, thanks to composer.json and /vendor/autoload.php. Probably faster.
Comment #68
Eric_A CreditAttribution: Eric_A commentedThis will scan '/^(CVS|lib|templates|css|js)$/'.
Should we not just copy code from
SystemListing::scanDirectory
/file_scan_directory
?Comment #69
iamEAP CreditAttribution: iamEAP commentedI 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.
Comment #70
sdboyer CreditAttribution: sdboyer commented#65: drupal-phpunit_bootstrap_contrib-2025883-65.patch queued for re-testing.
Comment #72
iamEAP CreditAttribution: iamEAP commentedRe-roll of #65.
Comment #73
iamEAP CreditAttribution: iamEAP commented#72: drupal-phpunit_bootstrap_contrib-2025883-72.patch queued for re-testing.
Comment #74
mmilano CreditAttribution: mmilano commentedI 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. :)
Comment #75
donquixote CreditAttribution: donquixote commented#11, #13
#18
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)
Comment #76
Mile23A few heads-up situations:
#2083547: PSR-4: Putting it all together
#2094843: Port PHPUnit test classes in (module dir)/tests/ to PSR-4
#2095037: Add vfsStream as a dependency in composer.json.
Mostly here for iamEAP. :-)
Comment #77
tstoecklerTried 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.
and so forth. What do you guys think?
Other than that I only found one small nitpick:
$d is not very descriptive.
Comment #78
iamEAP CreditAttribution: iamEAP commentedThanks 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.
Comment #80
iamEAP CreditAttribution: iamEAP commented#78: drupal-phpunit_bootstrap_contrib-2025883-78.patch queued for re-testing.
Comment #81
Mile23Agreed 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.
Comment #82
Eric_A CreditAttribution: Eric_A commented@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.Comment #83
Mile23PHPUnit'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>
Comment #84
alexpottCommitted f995135 and pushed to 8.x. Thanks!
Comment #85
Mile23Yay! Thanks alexpott. :-)
Now, about that #2056607: /core/phpunit.xml.dist only finds top-level contrib modules .....
Comment #86.0
(not verified) CreditAttribution: commentedCleaning up issue and noting the new related issue about registering and autoloading theme namespaces.