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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano created an issue. See original summary.

Xano’s picture

Status: Active » Needs review
FileSize
2.14 KB

This 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.

Mile23’s picture

This is really two issues: phpunit.xml.dist cleanup plus introduce regression.

Because the file structure of contributed modules is not predefined, we have to jump through hoops to support them

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 under sites/ and not so strict under modules/.

BC might be broken, depending on how run-tests.sh and Drupal CI run tests.

run-tests.sh doesn't enter into it. It has its own rules for scanning for files that are unrelated to the excludes in phpunit.xml.dist. DrupalCI uses run-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?

Xano’s picture

Category: Feature request » Bug report

This 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.

This is really two issues: phpunit.xml.dist cleanup plus introduce regression.

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.

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/.

I meant the file structure of projects is not defined.

To put a finer point on it: Both run-tests.sh and SimpleTest UI disagree with the solution offered in this issue.

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.

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.

Again, we're the only project that does this. If you think this is inconsistent, please provide arguments as to why.

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?

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.

Mile23’s picture

Issue tags: +PHPUnit

Again, we're the only project that does this. If you think this is inconsistent, please provide arguments as to why.

Like 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 duplicating core/tests/bootstrap.php so we can specify different search directories. Anyone wanting to run contrib tests would then do phpunit -c core/contrib.phpunit.xml.

Is that unreasonable?

Xano’s picture

This 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.

making it impossible for me to do the things I want to.

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 running phpunit without any configuration in the module's root directory. It's what every other PHP package out there does.

Adding a contrib.phpunit.xml to separate concerns makes the most sense to me,

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.

dawehner’s picture

What we could do is to ship with an example module.phpunit.xml.example file, so people could adapt them easily.

Xano’s picture

An 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.

Mile23’s picture

This 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.

I'm not seeing that. Are you saying that running core tests results in fails because of tests present in vendor/? Core phpunit.xml excludes vendor/, so it should never run those tests. In fact, your patch removes those exclusions:

+++ b/core/phpunit.xml.dist
@@ -20,35 +20,14 @@
-      <!-- Exclude Composer's vendor directory so we don't run tests there. -->
-      <exclude>./vendor</exclude>
You can still run whatever you want to using your own testing configuration.

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.

I'm against that, because as mentioned before the best solution is to make modules provide their own phpunit.xml.dist files,

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.

Xano’s picture

I'm not seeing that. Are you saying that running core tests results in fails because of tests present in vendor/? Core phpunit.xml excludes vendor/, so it should never run those tests. In fact, your patch removes those exclusions:

Dependents, not dependencies. Core's test suite must not fail if the tests of contributed modules (which are dependents of core) fail.

That's not a reason.

Which one of the following things I mentioned in #6 is not a reason in your eyes?

  • Better code coverage reports
  • Faster test bootstrap
  • The ease of use of just running phpunit without any configuration in the module's root directory.
  • It's what every other PHP package out there does. (predictability and consistency)
Mile23’s picture

Dependents, not dependencies. Core's test suite must not fail if the tests of contributed modules (which are dependents of core) fail.

Right, 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 using core/phpunit.xml regardless of the changes in this issue. Your end user will write their own tests, type run-tests.sh -all and if your custom bootstrap is all that different the tests will break.

Xano’s picture

Status: Needs work » Needs review

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.

Nobody 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.

Mile23’s picture

This 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.

dawehner’s picture

Well, 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.

Xano’s picture

Tell contrib maintainers to never use the phpunit tool that ships with core.

It 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.

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.

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.

Well, 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.

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?

Mile23’s picture

Status: Needs review » Needs work

"they must make their unit test bootstrap process compatible with run-tests.sh if they want to use the testbot." ... 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.

Your 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.

bojanz’s picture

Coming 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).

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Xano’s picture

So 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.

Mixologic’s picture

Right 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.

Mile23’s picture

Status: Needs work » Needs review

#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:

Here's what I think the principles should be:

  1. PHPUnit's test runner, run-tests.sh and the Simpletest UI should all work in as similar a way as possible.
  2. It has to be possible to only run tests of a certain type. Lumping unit, functional and functional javascript tests together is a bad idea because they have different test infrastructure requirements.
  3. Users should be able to get all a module's tests to run with run-test.sh --module MODULE_NAME
  4. Running with run-test.sh --directory MODULE_DIRECTORY and ./vendor/bin/phpunit -c ./core/phpunit.xml.dist MODULE_DIRECTORY should result in running all the same PHPUnit based tests. (run-tests should also run the simpletest tests) These should also be the same tests when using the --module option.
  5. Running with ./vendor/bin/phpunit -c ./core/phpunit.xml.dist --filter MODULE_NAME should also run all the module's tests

@Xano: 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.

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?

Xano’s picture

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.

I 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.

Mixologic’s picture

Drupal CI still cannot install Composer packages for contributed projects, and any testing tools core does not provide cannot be used, such

Now 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.

Xano’s picture

I 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.

Mixologic’s picture

If 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.

Mile23’s picture

Drupal CI still cannot install Composer packages for contributed projects

Here'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.

Xano’s picture

Note 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.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
larowlan’s picture

Status: Needs review » Closed (won't fix)
Issue tags: +Bug Smash Initiative

the .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.