Problem/Motivation
In #2927806: Use PHPUnit 6 for testing when PHP version >= 7.2 we moved to PHPUnit 6 for PHP 7.2 (and up) because PHPUnit 4 was totally incompatible with PHP 7.2. In fact, PHPUnit 4 does not support PHP 7 according to the docs - it works for but not by design.
We are also talking about dropping PHP 5 support from Drupal 8 - see #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7. At this point if all of PHP 7 was being tested with PHPUnit 6 we could drop PHPUnit 4 as well.
There are risks of unexpected API changes which might break contrib or custom testing and no one likes fixing tests because of upstream test API changes. That said, in order to work on PHP 7.2 we'll either have to mitigated such breakages or the tests will have to be adapted. An example of such a change would be using \PHPUnit_Util_XML::cssSelect()
- that method exists in PHPUnit 4 but not in 6.
Proposed resolution
Test PHP 7 using PHPUnit 6 and remove PHPUnit 4.
Remaining tasks
Discuss whether we want to do this.
User interface changes
None
API changes
None??? We could leave \Drupal\Tests\PhpunitCompatibilityTrait
in place to minimise any changes necessary.
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff-19-36.txt | 2.15 KB | mpdonadio |
#36 | 2932606-36.patch | 2.63 KB | mpdonadio |
#31 | 2932606-31.patch | 2.15 KB | jibran |
#19 | 2932606-19.patch | 734 bytes | jibran |
Comments
Comment #2
alexpottReading #2842431-156: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7 it looks like the release managers agree that we should drop PHPUnit 4 when 5.5 / 5.6 support is dropped. It is now just a question when should we move PHP 7.0 and 7.1 testing to PHPUnit 6. Which needs release manager input too. I think reading that comment the moving to PHPUnit 6 for all PHP 7 tests early in the 8.6.x cycle makes sense. But leave 8.5.x alone so we give people more chance to find any pitfalls in the BC layers.
Comment #3
alexpottHmmm... I meant for the tag to still be here...
Comment #4
catchThis makes sense to me. Given we're dropping PHP5 in 8.7 we could even leave it a minor release later though too?
Comment #5
alexpottThere are also reasons why we want to move on from PHPUnit 4 - it will make our developer experience better because we'll be able to do #2870145: Set printerClass in phpunit.xml.dist once we no longer use PHPUnit 4.
Comment #6
xjmHow will the upgrade affect backports?
Comment #7
xjmStill needs the tag I guess.
Comment #8
xjmKeep in mind that we must support running PHPUnit 4 throughout 8.6.x's release cycle until the last release prior to 8.7.0, and thereafter for 8.7.x on the nightlies until something is broken enough for us to raise the "unsupported PHP version" update message from a warning to an error. (Because not getting security updates is worse than a fatal on some random page.)
So we won't be able to do things like #2870145: Set printerClass in phpunit.xml.dist anytime soon regardless, because we need to have the test suite run on PHP 4 nightly. We'll have to see what's feasible, but the less fragmentation, the better. I don't think we want two different test suites.
Comment #9
alexpottYep we need PHPUnit 4 as long as we have PHP 5.x testing going on. This issue is not about dropping PHPUnit 4 - it's about moving to PHPUnit 6 for 7.0 and 7.1 which will allow us to drop PHPUnit 4 once we drop support and testing on PHP 5.x.
One side effect of using composer the way we do to update PHPUnit if you are on PHP 7.2 is that it makes committing when you have PHP 7.2 hard if you are using https://github.com/alexpott/d8githooks as that does a composer install during commit to ensure that phpcs etc are running with the correct versions.
Comment #10
alexpottRe #6 it shouldn't affect backports at all - we already are testing using PHPUnit 6 on PHP 7.2 so people already have to produce tests the work on both PHPUnit 4 and PHPUnit 6 - which fortunately is not that tricky.
Comment #12
jibranComment #13
alexpott@jibran it'd be great to get #2937469: Automatic upgrade to PHPUnit 6 is dangerous done first (and before 8.5.0 too).
Comment #14
Mile23Hah. Should have found this issue before I did this: #2937469-22: Automatic upgrade to PHPUnit 6 is dangerous
Basically: Normalize on PHPUnit 6 and make PHP < 7 the special case. Put PHPUnit 6 in composer.lock and then do composer update if PHP < 7.
Comment #15
alexpott#14 won't work :( because then composer install on PHP5 would fail. We need to remove the lock file and that will be super complicated to do.
Comment #16
Mixologic#2932606-14: Use PHPUnit 6 for PHP 7.0 / 7.1 testing wont work, but, perhaps we could change the script from #2937469: Automatic upgrade to PHPUnit 6 is dangerous to be >= php 7.0 instead of just 7.2 ?
Comment #17
alexpott@Mixologic yep #16 is totally the current plan!
Comment #18
Mile23Re: #15, removing the lock file: Obligatory link to #2874198: Create and run dependency regression tests for core which could use some more input. Similar strategies in #2876669-40: Fix dependency version requirement declarations in components to solve dependency checking for Components.
Comment #19
jibranReroll after #2937469: Automatic upgrade to PHPUnit 6 is dangerous
Comment #20
MixologicIm pretty sure this should be RTBC.
I would *really* appreciate a set of eyes on #2947470: Update phpunit as part of build where we upgrade phpunit as part of drupalci's build process.
Comment #21
IncrediblyKenzi CreditAttribution: IncrediblyKenzi as a volunteer commentedNot sure if this is the right place to report it, but I see one issue, which is the bridge between PhpUnit 4 and 6 is throwing a warning on non-mac systems (e.g. case-sensitive filesystems, due to this line):
class_alias('\PHPUnit\Util\XML', '\PHPUnit_Util_XML');
Should be:
class_alias('\PHPUnit\Util\Xml', '\PHPUnit_Util_XML');
I'm working on a patch, but if someone else gets to it before me, feel free.
Edit: Nevermind. I'm an idiot.
Comment #22
Mile23Before DrupalCon Nashville is over, you'll hear about adding a per-project build stage to the testbot.
Core should solve the problem of which version of PHPUnit is appropriate by letting Composer do it in the drupalci.yml file. Accomplish this by adding a
composer update phpunit/phpunit --with-dependencies
to the assessment stage.I don't want to add a patch illustrating how to do this here, since the last patch is RTBC and the build file thing isn't quite fully announced. But I'll put a new one here: #2951843-3: Use drupalci.yml to fail test runs early
Comment #25
catchCommitted/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!
Comment #27
catchOn second thoughts, let's leave this in 8.6.x.
Comment #28
jibranCan you please elaborate? It is a test only change. I think @alexpott made it perfectly clear in #9
so why not let everyone adapt early?
Comment #29
alexpott@jibran it was me who asked @catch to roll this back from 8.5.x - people have CI set ups working on 8.5.x this might suddenly break them because they have to run composer update to get the correct version of PHPUnit. That does feel like a change to make in a bug fix release. Also this change is really just about prepping the ground to be able to drop PHPUnit 5 when we drop PHP 5 which is scheduled for 8.7.0. This change should also be mentioned in the 8.6.0 release notes.
Comment #30
tim.plunkettThis was confusing to me, as the message says
PHPUnit testing framework version 6 or greater is required when running on PHP 7.2 or greater. Run the command 'composer run-script drupal-phpunit-upgrade' in order to fix this.
Yet I'm on 7.1, and it was happily working yesterday.
Maybe update that string then?
Comment #31
jibranGood catch! Sorry for missing the docs. Here we go.
Comment #32
mpdonadioTested #31 locally w/ PHP 7.0.12 via MAMP and got proper error message.
Comment #33
mpdonadioActually, after running into some snags, I think this needs a Change Notice.
We have one for the 7.2 change in 8.5.0, https://www.drupal.org/node/2936045, but we should have one for 8.6.0. One because it is in a different branch, and two people may have to adjust the `
` sections of their local phpunit.xml (see also #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes). The existing CR should probably also be updated to mention this.
Comment #35
catchOK I've reverted the 8.6 commit as well - let's put this in with better docs and the change notice ready to go.
Comment #36
mpdonadioOK, here is #19 + #31.
Also took stab at CR.
Comment #37
jibranThis looks good to me.
Comment #39
catchCommitted 0658077 and pushed to 8.6.x. Thanks!
Comment #40
mpdonadioPubished [#2957906]
Comment #41
ChrisDarke CreditAttribution: ChrisDarke at Hook 42 commentedI followed the steps to run phpunit tests from core with
../vendor/bin/phpunit --testsuite=unit
and I received the message as mentioned above by tim.plunkett. (https://www.drupal.org/project/drupal/issues/2932606#comment-12551786)Upon following the steps I got an update to composer.lock which isn't covered by the patch. Can someone clarify what are the required steps and if there is something missing to close off this issue?
Moving back to "needs work" if there are any recommended changes for the documentation or any additional patch updates.
Comment #42
jibran> Upon following the steps I got an update to composer.lock which isn't covered by the patch.
This is correct, composer.lock file only represents the changes for PHP version 5.5.9 aka
DRUPAL_MINIMUM_PHP
we can't commit composer.lock changes until we drop php5.Comment #43
ChrisDarke CreditAttribution: ChrisDarke at Hook 42 commentedMaybe we can add some information for anyone running across this issue and concerned with what to do with the composer.lock being updated and not wanting to include the changes in any patches, like "run git update-index --assume-unchanged composer.lock to remove the changes from the working tree".
Comment #44
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@mpdonadio from #40
It seems that the syntax [#2957906] does not resolve if the node is not an issue. To save others wondering what that link was meant to be, it's the CR https://www.drupal.org/node/2957906
Comment #46
ysamoylenko CreditAttribution: ysamoylenko at EPAM Systems commentedHello everyone,
The problem still exists for:
D8.6 - PHP 7.1+
D8.5 - PHP 7.2+
Examples:
https://dispatcher.drupalci.org/job/drupal8_contrib_patches/55668/console
https://dispatcher.drupalci.org/job/drupal8_contrib_patches/55651/console
https://dispatcher.drupalci.org/job/drupal8_contrib_patches/55652/console
https://dispatcher.drupalci.org/job/drupal8_contrib_patches/55650/console
Comment #47
ysamoylenko CreditAttribution: ysamoylenko at EPAM Systems commentedCurrently, everything works as expected. Thank you.