Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
So over in #2932606: Use PHPUnit 6 for PHP 7.0 / 7.1 testing and
#2937469: Automatic upgrade to PHPUnit 6 is dangerous
#2927806: Use PHPUnit 6 for testing when PHP version >= 7.2
There are discussions about using phpunit 6 for all drupal7 testing and how we get there.
Currently there's a composer script who's sole purpose is to determine whether to run, and to run composer upgrade phpunit/phpunit --with-depenedencies
This script is being executed automatically when somebody uses run-tests.sh (but not when using phpunit directly, which is common). However, an error message *is* displayed telling devs they have to run the upgrade script.
Now, there are becoming more ways to run tests (nightwatch) that dont end up using run-tests.sh, so in order for the testing build to have the right phpunit, we'll probably need to upgrade it on the testbots too. Having run-tests.sh automagically do build work for the testbots isnt currently the right approach.
We would still need the check to happen for 7.0 as per here (#2932606: Use PHPUnit 6 for PHP 7.0 / 7.1 testing), so that local devs would get a warning if they were working on php7 and up and just did a bare install and went straight to phpunit.
So what we probably need to do then is use the plugin we already have built for this purpose (container_composer) and just upgrade phpunit for all tests.
This means that the testbot tests would defacto run phpunit 6 for php 7 and up.
The only caveat is this: #2937469-15: Automatic upgrade to PHPUnit 6 is dangerous Alex Pott said:
The problem with saying okay let's change DrupalCI to call composer run-script drupal-phpunit-upgrade-check is that this script is only available in Drupal 8.5.x and not in 8.4.x.
So the proposal here isn't to run drupal-phpunit-upgrade-check at all, its just to run composer upgrade phpunit/phpunit --with-depenedencies in the container as part of the build process for drupalci.
Proposed resolution
Add a container_composer task to run update phpunit/phpunit --with-dependencies. We still need the composer install one in order to get a sanity check for the platform.
Later on in the build, run-tests.sh will make sure the proper phpunit is installed, will discover that it is, and not spend time trying to update it.
We're doing this mainly for contrib, because it's likely that core will have it's own drupalci.yml file.
If project maintainers add drupalci.yml files to their project which omit updating phpunit, then run-tests.sh will still perform the update.
Eventually, Drupal 8 will drop support for PHP 5, and the Drupal 8 default build file will not need this step. That will still leave Drupal 7 which might need this step.
Comments
Comment #2
MixologicComment #3
MixologicComment #5
MixologicSo the main question is : what impact would phpunit 6 have on existing contrib modules that are testing with php7/7.1 and not 7.2? Should this only happen for core testing?
Comment #6
Mile23The only reason that would happen is because PHPUnit 6 has version constraints against PHP <7 and core is specified to use it, not that the testbot has this as a policy. A contrib project could require-dev phpunit ~4 and that would become the constraint.
I think this is totally the way to go. We need a composer install phase before the update phase, because otherwise wikimedia merge plugin doesn't work, and that's present in the feature branch so yay.
Core tests have a compatibility trait for differences between PHPUnit 4 and 6, which amounts to making a facade for getMock(). So disruption should be minimal for contrib if they're using core's test base classes. I think it's probably also true that not a lot of contrib has unit tests, so they won't be mocking things in the first place.
I'll resist setting RTBC until a core maintainer has a say.
Comment #7
alexpottFor me ideally this would be part of #2901677: Allow projects to contain their own drupalci.yml file rather part of DrupalCI. Because then we can choose when to move to PHPUnit 6 and under what conditions. At the moment this will move all PHP7 testing onto PHPUnit 6 and make #2932606: Use PHPUnit 6 for PHP 7.0 / 7.1 testing mostly unnecessary. However I agree with @Mile23 concerns for contrib are minimal and anything we discover that makes running both PHPUnit 4 and PHPUnit 6 will have to be addressed whilst we continue to support D8 on PHP 5.5 / 5.6.
I'm also +1 to removing run-tests.sh calling out to composer update so this for me is a move in the right direction. Just not entirely convinced on the implementation. I think in order to progress we need to decide whether this should be part of any build.yml and whether all PHP7 testing should be on PHPUnit 6.
Comment #8
MixologicOkay, so It just occurred to me that we dont *immediately* need this for nightwatch testing, as the problem only exists on drupalci if we're not running any of the simpletest plugins, and only running nightwatch on its own.
As it stands, drupalci is definitely going to be running all the tests, so when the nightwatch stuff gets merged into core, the run-tests.sh update will still work.
Im going to postpone this on #2932606: Use PHPUnit 6 for PHP 7.0 / 7.1 testing as, like Alex said, we gotta make that decision first before we execute on it here.
Comment #9
MixologicPostponed on #2949209: Phase I: Allow drupalci.yml to influence the build
Comment #10
Mile23Adding a specific step to the testbot isn't such a great idea now that we have the per-project build file. See #2932606-22: Use PHPUnit 6 for PHP 7.0 / 7.1 testing
Comment #11
Mile23Added an example of drupalci.yml file that does this from within the core codebase: #2951843-4: Use drupalci.yml to fail test runs early
Comment #12
Mile23