Problem/Motivation
./core/phpunit.xml.dist
was written to run unit tests of Drupal core, install profiles, and (contributed) modules in the system. Because the file structure of contributed modules is not predefined, we have to jump through hoops to support them (#2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests), while it is highly unconventional for any software package to run tests for its dependents (#2499239-12: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests).
Proposed resolution
- Remove the ability of
./core/phpunit.xml.dist
to run any tests that are not part of Drupal core. - Require contributed projects to ship their own
phpunit.xml.dist
file if they want their PHPUnit-based tests to be run.
Both of these steps are consistent most other software packages, including Drupal core's own Composer dependencies.
Remaining tasks
T.b.d.
User interface changes
None.
API changes
None. BC might be broken, depending on how run-tests.sh
and Drupal CI run tests.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#2 | drupal_2677122_2.patch | 2.14 KB | Xano |
Comments
Comment #2
XanoThis patch removes all paths to tests belonging to contributed modules, as well as those to
./core/vendor
because all that code was moved to./vendor
a while ago, and to./drush
, because we shouldn't provide such workarounds for contrib in core, and #2056607-11: /core/phpunit.xml.dist only finds top-level contrib modules introduced this workaround for Devel's Drush tests, which we will no longer pick up with this patch anyway.\Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest::testGetListenerPriority()
causes a single unit test failure, because it provides neither expectations nor assertions. This has not caused any issues with the Drupal CI branch tests since it was committed, though, so let's see what happens with this patch before we write a fix.Comment #3
Mile23This is really two issues:
phpunit.xml.dist
cleanup plus introduce regression.This is misleading because the file structure of a given module is defined making it trivial to find tests within the module/extension. It's just the location of modules within directories is not, so we have to search for the modules. Which isn't hard, just potentially slow and also potentially insecure when running tests under
sites/
. #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests seeks to ameliorate this problem for the phpunit tool only, by being strict undersites/
and not so strict undermodules/
.run-tests.sh
doesn't enter into it. It has its own rules for scanning for files that are unrelated to the excludes inphpunit.xml.dist
. DrupalCI usesrun-tests.sh
.run-tests.sh
sensibly scans for (and runs) your contrib module's PHPUnit-based tests at multiple directory levels. So does the SimpleTest UI.To put a finer point on it: Both
run-tests.sh
and SimpleTest UI disagree with the solution offered in this issue.The odd man out is the phpunit tool which behaves inconsistently by accident rather than by design, and this issue wants to make it inconsistent by design, which is why I changed this to a feature request.
Also, if we combine #2301873-8: Use ExtensionDiscovery to register test namespaces for running PHPUnit with #2329453: Ignore front end vendor folders to improve directory search performance we have a performant, multi-depth scan of
modules/
for the phpunit tool, so that objection begins to vanish.It's fine to have a need and then fill that need. I just can't see what the need is here. So what is it?
Comment #4
XanoThis is a bug, because there is no way to run core's PHPUnit tests without dependents' tests interfering with the test run. Contrib tests make my core test runs fail, for instance. This makes QA difficult, and this behavior is inconsistent with any other project I have seen.
Yes and no. This issue aims to include only the tests that are shipped with core, so the clean-up really just removes the configuration that was needed when our rules were less specific.
I meant the file structure of projects is not defined.
Those are irrelevant, as they are either their own systems or built on top of PHPUnit. This issue is about our PHPUnit integration only. Also, neither of those make assumptions as to which tests to run, which
./core/phpunit.xml.dist
does do right now.Again, we're the only project that does this. If you think this is inconsistent, please provide arguments as to why.
Core should not meddle with contrib, and core's PHPUnit integration is inconsistent with other PHP projects and fails if its dependents' tests fail. This is an unnecessary Drupalism.
Comment #5
Mile23Like I said: "run-tests.sh sensibly scans for (and runs) your contrib module's PHPUnit-based tests at multiple directory levels. So does the SimpleTest UI."
....OK, so the issue is that you want to be able to run core tests without also running contrib tests, using the shipped PHPUnit with the shipped bootstrapping.
PHPunit has
--exclude-group
but we don't want to keep track of all module names during dev.Adding a
contrib.phpunit.xml
to separate concerns makes the most sense to me, rather than making it impossible for me to do the things I want to. That would mean essentially duplicatingcore/tests/bootstrap.php
so we can specify different search directories. Anyone wanting to run contrib tests would then dophpunit -c core/contrib.phpunit.xml
.Is that unreasonable?
Comment #6
XanoThis is about the fact that core's unit test suite can fail depending on dependents' unit tests, and that is a bug in our QA workflow. The only reason for failures in core's test suite should be bugs in core itself, not in those of third party packages. We don't include any tests belonging to dependencies or non-module dependents in the suite either, so
phpunit.xml.dist
cannot be used for testing the entire codebase anyway.You can still run whatever you want to using your own testing configuration. There are several reasons for shipping dedicated
phpunit.xml.dist
files with modules, such as better code coverage reports, faster test bootstrap, and the ease of use of just runningphpunit
without any configuration in the module's root directory. It's what every other PHP package out there does.I'm against that, because as mentioned before the best solution is to make modules provide their own
phpunit.xml.dist
files, but if core committers believe this is necessary to relieve the stress on contrib, then this is not a blocker.Comment #7
dawehnerWhat we could do is to ship with an example module.phpunit.xml.example file, so people could adapt them easily.
Comment #8
XanoAn example would be good indeed. I was thinking of providing one, but in the scope of a documentation page under the PHPUnit section. It feels like it's easier for people to update such a page with new best practices and information about different PHPUnit versions (because contrib may need 5 while core stays on 4 for a while, for instance) than it is to update the example in core. I'm not sure, however, and this may just be a personal preference.
Comment #9
Mile23I'm not seeing that. Are you saying that running core tests results in fails because of tests present in
vendor/
? Core phpunit.xml excludesvendor/
, so it should never run those tests. In fact, your patch removes those exclusions:Or I could fix core so it works according to my expectations, and you can set up your own configuration however you want, which is what you did anyway.
That's not a reason.
....
I'm still not sure what the problem is, which might be why I think there's a solution that satisfies both of our needs.
Comment #10
XanoDependents, not dependencies. Core's test suite must not fail if the tests of contributed modules (which are dependents of core) fail.
Which one of the following things I mentioned in #6 is not a reason in your eyes?
Comment #11
Mile23Right, so the solution is to either make a 'core' suite or a secondary phpunit.xml for contrib and you can do
-c core/contib.phpunit.xml
. For a while we were tagging core tests as@group core
which would solve the problem, but that got shot down for some reason.Requiring people to create their own
phpunit.xml
per contrib and then setting up travisci or whatever and not using the testbot is too much to ask.Currently
run-tests.sh
will find all your unit tests and then try to run them usingcore/phpunit.xml
regardless of the changes in this issue. Your end user will write their own tests, typerun-tests.sh -all
and if your custom bootstrap is all that different the tests will break.Comment #12
XanoNobody is talking about setting up CI services. We chose to use a particular testing framework, which works by letting packages include their own testing configuration. There is no need to break with those conventions and come up with our own, conflicting ones. Conflicting, because the conventional configuration file does not do what people expect it to do. Remember that PHPUnit is not a Drupalism, and we should be careful to make/keep our integration a Drupalism. Also, if any other PHP package out there can do this, so can modules.
Setting back to needs review, as that's what this issue needs.
Comment #13
Mile23This issue does two things:
1) Tell contrib maintainers to never use the phpunit tool that ships with core.
2) Tell contrib maintainers that even though they shouldn't use the phpunit tool that ships with core, they must make their unit test bootstrap process compatible with run-tests.sh if they want to use the testbot.
That's why I don't like it.
Comment #14
dawehnerWell, IMHO we should provide an example bootstrap file + phpunit.xml(.dist) file a contrib module. I thought agree with @Mile23 that this has quite some impact on all of the contrib tests out there, so I'm wondering whether we should primarily do the documentation of how you can ship with your own ones, but not break the existing contrib modules out there.
Then maybe in 9.0.x we could remove the support for contrib modules.
Comment #15
XanoIt does not. It only tells contrib not to use core's own testing configuration. Contrib is free to choose between using core's PHPUnit binary or pull in their own, although Drupal CI does not yet support the latter, which is an issue for another day.
I'm not sure what you mean by this. The bootstrap is not in scope, but can be identical to core's, which I would say is recommended due to our autoloader, for instance.
I must say I have not been able to find much information about the naming conventions of PHPUnit configuration files. Do you know if there is any?
There will be impact, but seeing as this is internal and for testing only, I wanted to bring the BC break up for discussion. If the consensus is that we cannot break anything, then we provide an additional file that's just for core.
Where do you suggest we put the example file? In a new directory
./modules/examples
? On a handbook page? Elsewhere?Comment #16
Mile23Your patch says that contrib is basically not allowed to use core's phpunit.xml file.
That means that contrib is required to create its own phpunit.xml file.
Which then must figure out how to bootstrap.
So you say it *can* be identical to core's, but it doesn't have to be, and if core changes, then it would have to change to stay identical.
The testbot *will* use core's phpunit.xml file for everything but discovery.
So that means that (given this issue) if you want to use the testbot, you *must* make your phpunit.xml file exactly the same as core's for the same expectations.
So: If this issue were committed, then every contrib user who just wanted to use PHPUnit tests with the testbot, without any exotic CI setup, would have to create their own phpunit.xml file and make sure it tracked core's file. And they'd be doing this for no obvious benefit.
Comment #17
bojanz CreditAttribution: bojanz at Centarro commentedComing from #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests I agree with the assumption of this issue.
Contrib tests should not automatically run when I do "phpunit ." in the drupal root.
Core's phpunit.xml will setup bootstrap and all relevant settings.
A module's unit tests will work without a phpunit.xml file, you only need to add it if you want to define the testgroups (unit/functional/kernel), which is optional and to me fine in an additional step (as long as we provide a clear example).
Comment #19
XanoSo as long as we clearly document how to create custom
phpunit.xml.dist
files, can we agree that this is a good way forward?It may be impossible to make
run-tests.sh
work with this easily since it is meant to be able to run all our tests with a variety of filters applied, meaning it's possible to run PHPUnit-based tests from multiple packages (and therefore configuration files) during the same test run. In my opinion this is a bad idea with our current variety in testing frameworks and any testing infrastructure should run PHPUnit-based tests directly through PHPUnit. I am just not sure what can be done on this other than to take out the broken functionality and update Drupal CI. We probably need Drupal CI maintainers to chip in here.Comment #20
MixologicRight now, drupalCI uses run-tests.sh to run *everything*, and expects the one output file generated by that process as a build artifact on the dispatcher. Drupal.org is expecting to get all the results information from that one build artifact, which it uses to put all the results on d.o.
Allowing drupalCI to run multiple job types, and have multiple output artifacts, such as run-tests.sh as well as phpUnit is *definitely* on the 'near term' roadmap.
One issue that phpUnit has always had, that run-tests.sh does not have, is that PHPUnit does not gracefully handle fatal errors, meaning the only information we'd be able to put on drupal.org is "PHPUnit tests failed to complete, probably due to a fatal error, somewhere, somehow, we don't know, go figure it out." Whereas with the run-tests.sh acting as a wrapper around PHPUnit we at least get through the whole test and are able to report something back. (Though Im not sure we're providing much more information unless you read the console logs)
Additionally, run-tests lets us maximize our hardware by running tests concurrently, so we'd need phpunit support for that (maybe, since they're faster it might be unnecessary, but they aren't necessarily fast).
So there is a couple of considerations because the CI infra has to do a whole lot more than just run the tests, but we do want to support this.
Comment #21
Mile23#2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests is actually fixed and committed, despite being 'needs review' (it got committed while I was still writing tests...)
@alexpott laid out some guideline behaviors for Drupal's test runners here: #2605664-123: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits (That's #123). The basics:
It seems like we'd want uniform behavior between core and contrib, so that when the testbot runs we only need one configuration. Nothing is stopping any contrib project from having its own phpunit.xml and running travis or whatever, but then that project should not turn on automated testing on d.o, because it's ambiguous what that will mean for the testbot. We might even have the testbot refuse to test contrib with a phpunit.xml file.
So is this issue closed won't fix?
Comment #22
XanoI don't know why we want uniform testing behavior for several thousands of packages, especially since our current QA standards are not extremely high and severely limit contrib (Drupal CI still cannot install Composer packages for contributed projects, and any testing tools core does not provide cannot be used, such ), which is why several (high-profile) contrib projects have moved to Github and tools like Travis CI already. While this is common knowledge, I wanted to illustrate the consequences of our current QA policy, and that focusing on uniform testing behavior is not the best of ideas.
Having said that, let's go back to this issue's topic: Drupal core's test runs including tests from other packages. This was discussed in #2499239-12: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests and #2499239-14: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests as well, as nobody seems to have seen any other package that runs other packages' tests as part of its own. We should fix this.
If, as a consequence of fixing Drupal core's test runs, we run into other problems (which are symptoms of our current testing workflow with side effects), we should fix those. If Drupal CI wants to use default PHPUnit configuration for projects that do not ship with one, then it can. If we want to convince developers to include their own configuration, we should write a handbook page.
Comment #23
MixologicNow that we have the composer façade in place, we can switch drupalCI to building the codebase with composer, and as such, this will not be an issue in the near future.
Comment #24
XanoI value the progress that has been made recently, and I hope I did not come across as unappreciative. Unfortunately it's a known problem we've been struggling to keep our QA processes (both in core and on drupal.org) up to date with current technology. We should accept that fact, and if we can make decisions that prevent Drupalisms (and in doing so prevent having to build customized infra), then I think we should. It helps us get off the island and lowers our workload in the long term by keeping things maintainable.
Comment #25
MixologicIf there existed any current CI technology that was capable of testing core, we would use it, but no such "current technology" exists. We don't build customized infra lightly, as we're all too cognizant of the costs associated with the maintenance burden. But thats a conversation for another thread.
Comment #26
Mile23Here's the issue: #2597778-51: Install composer dependencies for contrib projects before running tests
We have a working solution.
It just needs someone other than me or @jthorson to work on it.
Comment #27
XanoNote that in #22 I mentioned Composer as one of several examples of limitations of our testing workflow.
We still have PHPUnit configuration that covers everyone's entire application instead of just the package it belongs to, and that cannot even be used to compute code coverage statistics, because the fact it belongs to core means it's applied against a huge file structure making it incredibly slow.
I proposed a solution where Drupal CI uses a default configuration file for any contributed project that does not ship one. What do you think about that? It prevents breaking Drupal CI runs for now until we decide otherwise, yet it cleans up core's configuration nicely and encourages people to ship their own configuration if they want to use anything else than Drupal CI.
Comment #37
larowlanthe .dist in the filename is the 'distributed' version, per phpunit docs, you can copy it to phpunit.xml (no .dist) locally and have it not interfere with version control.
I've been running my own phpunit.xml with a custom boostrap.php for some time (for the same reasons outlined by Xano).
I think we can close this one.