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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Reading #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.

alexpott’s picture

Hmmm... I meant for the tag to still be here...

catch’s picture

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.

This makes sense to me. Given we're dropping PHP5 in 8.7 we could even leave it a minor release later though too?

alexpott’s picture

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

xjm’s picture

How will the upgrade affect backports?

xjm’s picture

Still needs the tag I guess.

xjm’s picture

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

alexpott’s picture

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

alexpott’s picture

Re #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.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.

jibran’s picture

Status: Active » Needs review
FileSize
864 bytes
alexpott’s picture

@jibran it'd be great to get #2937469: Automatic upgrade to PHPUnit 6 is dangerous done first (and before 8.5.0 too).

Mile23’s picture

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

alexpott’s picture

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

Mixologic’s picture

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

alexpott’s picture

@Mixologic yep #16 is totally the current plan!

Mile23’s picture

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

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

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

IncrediblyKenzi’s picture

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

Mile23’s picture

Before 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

  • catch committed 7d0b699 on 8.6.x
    Issue #2932606 by jibran, alexpott: Use PHPUnit 6 for PHP 7.0 / 7.1...

  • catch committed 9033407 on 8.5.x
    Issue #2932606 by jibran, alexpott: Use PHPUnit 6 for PHP 7.0 / 7.1...
catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed 67b3997 on 8.5.x
    Revert "Issue #2932606 by jibran, alexpott: Use PHPUnit 6 for PHP 7.0 /...
catch’s picture

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

On second thoughts, let's leave this in 8.6.x.

jibran’s picture

Can you please elaborate? It is a test only change. I think @alexpott made it perfectly clear in #9

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.

so why not let everyone adapt early?

alexpott’s picture

Issue tags: +8.6.0 release notes

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

tim.plunkett’s picture

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

jibran’s picture

Status: Fixed » Needs review
Issue tags: +Quickfix
FileSize
2.15 KB

Good catch! Sorry for missing the docs. Here we go.

mpdonadio’s picture

Status: Needs review » Reviewed & tested by the community

Tested #31 locally w/ PHP 7.0.12 via MAMP and got proper error message.

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change notice
Related issues: +#2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes

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

  • catch committed 00a526a on 8.6.x
    Revert "Issue #2932606 by jibran, alexpott: Use PHPUnit 6 for PHP 7.0 /...
catch’s picture

OK I've reverted the 8.6 commit as well - let's put this in with better docs and the change notice ready to go.

mpdonadio’s picture

OK, here is #19 + #31.

Also took stab at CR.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

  • catch committed 0658077 on 8.6.x
    Issue #2932606 by jibran, mpdonadio, alexpott, catch: Use PHPUnit 6 for...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0658077 and pushed to 8.6.x. Thanks!

mpdonadio’s picture

Pubished [#2957906]

ChrisDarke’s picture

Status: Fixed » Needs work

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

jibran’s picture

Status: Needs work » Fixed

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

ChrisDarke’s picture

Maybe 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".

jonathan1055’s picture

@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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

ysamoylenko’s picture

Currently, everything works as expected. Thank you.