Problem/Motivation
PHP 7.2 is not supported by out current version of PHPUnit. There is no PHPUnit version which supports PHP 5.5 and PHP 7.2 - see https://phpunit.de/. In fact the version of PHPUnit we are using is unsupported.
Proposed resolution
Use PHPUnit 6 for testing on PHP 7.2 but remain on PHPUnit 4 for 5.5, 5.6, 7.0 and 7.1. This is similar to what Symfony are doing to resolve this issue but Drupal has the added complexity that we have a composer.lock and people have come to expect that PHPUnit is around when you install Drupal with development dependencies.
Component tests are updated to use different exception methods depending PHPUnit version because they don't inherit the drupal base class. This is okay because they are our components.
Mocks that try to mock methods that don't exist are updated.
Other options to get PHPUnit testing on 7.2 are:
- Ditch PHP 5.5 and PHP 5.6 testing and move PHPUnit 6 - lots of API breaks and whilst #2795317: Allow PHPUnit 6+ support for object mocking helps somethings just look quite tricky to achieve - maybe class aliases for things which no longer exist will work but it is likely to be an interesting ride. Downsides are we'd be flying blind in PHP 5 land for the next year whilst it is still supported.
- Go the Symfony solution and remove phpunit from composer.josn. Then install different PHPUnit versions depending on the PHP version. This comes with the cost of maintaining things like the legacy listeners seen in #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes and all the complexity of multiple test paths seen above. But if a contrib or custom test stays on the same PHP version they don't have to change their tests and then they can choose when to upgrade.
- Fork PHPUnit 4 and make it work on all the PHP versions Drupal 8 is going to support. Downside is the we're likely to have to make further changes for future PHP versions and we're then got a test system to support again
Remaining tasks
Decide if the composer hack is the way to go.
User interface changes
None
API changes
There should be no API breaks for existing tests. There are things that work on PHPUnit 4 but actually are just bugs - for example mocking methods that do not exist.
Data model changes
Original report
If you run tests with PHP7.2 you see warnings like:
PHP Deprecated: The each() function is deprecated. This message will be suppressed on further calls in /Users/alex/.composer/vendor/phpunit/phpunit/src/Util/Getopt.php on line 38
After applying #2853503: Remove all assert('string') calls from Drupal core because deprecated in PHP 7.2 if I run a test like
phpunit -c ./core core/tests/Drupal/KernelTests/Core/Batch/BatchKernelTest.php
PHP Deprecated: The each() function is deprecated. This message will be suppressed on further calls in /Users/alex/.composer/vendor/phpunit/phpunit/src/Util/Getopt.php on line 38
Deprecated: The each() function is deprecated. This message will be suppressed on further calls in /Users/alex/.composer/vendor/phpunit/phpunit/src/Util/Getopt.php on line 38
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.
Testing Drupal\KernelTests\Core\Batch\BatchKernelTest
.
Time: 712 ms, Memory: 6.00MB
OK (1 test, 6 assertions)
It passes so all looks okay(ish)... at least it passes. But on testbot we're seeing:
Batch.Drupal\KernelTests\Core\Batch\BatchKernelTest
✗
Drupal\KernelTests\Core\Batch\BatchKernelTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-94.xml:
PHPunit Test failed to complete; Error: PHPUnit 4.8.36 by Sebastian Bergmann and contributors.
Testing Drupal\KernelTests\Core\Batch\BatchKernelTest
.
Time: 615 ms, Memory: 4.00MB
OK (1 test, 6 assertions)
Other deprecation notices (1)
The each() function is deprecated. This message will be suppressed on further calls: 1x
Comment | File | Size | Author |
---|---|---|---|
#94 | 2927806-94.patch | 76.91 KB | alexpott |
#94 | 90-94-interdiff.txt | 15.95 KB | alexpott |
#90 | 2927806-90.patch | 66.52 KB | alexpott |
#90 | 81-90-interdiff.txt | 5.64 KB | alexpott |
#87 | 2927806-81.patch | 67.04 KB | alexpott |
Comments
Comment #2
alexpottComment #3
catchBumping this to critical since it prevents a passing test suite on PHP 7.2.
Comment #4
mondrakeThat'd be the most elegant solution... would require a change to composer.json to be sth like
... which would allow PHP 5.5 to fallback on PHPUnit 4.8.36 and the later PHP releases to use PHPUnit 5.7.25.
But I do not know about the implications. Especially since we are composer installing based on a pre-defined composer.lock.
Comment #5
Wim Leers#4: Interesting! Can you post a patch that does that?
Comment #6
alexpottWell the lock file is going to cause us problems. Because of that regardless of how we change the composer.json we're not going to be able to let composer fix this for us. Symfony don't even have phpunit in their dev dependencies they manage everything via this script -
https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/PhpUni...
Comment #7
mondrake#5 just for a reference, take a look at https://github.com/PhenX/php-font-lib/pull/66, how that changes the composer.json, and how the TravisCI runs behave for different PHP versions. The point in that one though is that there is no composer.lock, so composer is 'free' to select the most appropriate PHPUnit version based on PHPUnit constraints.
Comment #8
mondrakeHere's a patch for composer.json.
Most probably, it won't do anything helpful, since on testbots we're running
composer install
, and we getThis bit is the key one: Installing dependencies (including require-dev) from lock file
If we could add a 'composer require phpunit/phpunit:5.*' to the testbot script after the composer install, then theoretically with the composer.json allowing PHPUnit 5.7 we should be able to update the PHPUnit version dynamically per test run.
Comment #9
alexpott@mondrake a possible way to do that is with a composer script that does a composer command to do exactly that - like checks PHP version and current PHPUnit version and if they are 7.2 and 4.8 then does a require for the new PHPUnit... could be a bit recursive but interesting to see if it works.
Comment #10
mondrake@alexpott yeah... but that's above my head. I'll pass to the next person here :)
Comment #11
alexpottThis needs to be
"phpunit/phpunit": ">=4.8.35 <5 || ^5.7",
And then in the root composer.json we need to add something like:
But new second command needs to be written in some way that does not cause recursion - as it does in the version above.
Comment #12
mondrakeHow about
"phpunit/phpunit": "^4.8.35 || ^5.7"
?Comment #13
mondrakeA do-not-test patch for #12, will do some testing on TravisCI
Comment #14
mondrakeSo... theory works.
I'm running a TravisCI build to run tests for the Imagemagick module.
removal of assert('string'), plus the patch in #2915820: [PHP 7.2] FormValidator: Parameter must be an array or an object that implements Countableform validator countablethen installation with Drupal console runs fine, PHPUnit is updated to 5.7.25, and tests execute.
However, the each depreaction failure is still there, no clue why...
See script: https://github.com/mondrake/imagemagick/blob/dev-7.2/.travis.yml
See TravisCI build results with 7.2: https://travis-ci.org/mondrake/imagemagick/jobs/310142301
... just sharing, obviously OT again wrt the issue here :)
EDIT - added issue numbers
Comment #15
martin107 CreditAttribution: martin107 as a volunteer commented@mondrake,
A quick quesiotion when you say
Do you mean including the patch from #2915820: [PHP 7.2] FormValidator: Parameter must be an array or an object that implements Countable ? I was not sure.
Comment #16
mondrake@martin107 yes, sorry, I drafted the comment and then forgot to inline the actual issue numbers. Updated.
Comment #17
martin107 CreditAttribution: martin107 as a volunteer commentedAh no problem ... thanks for the prompt reply.
Comment #18
mondrakeDid some more tests locally.
1. The
message also appears in PHPUnit 5.7.25, even if there the call to the the function each() is silenced. Weird. I could not find any way to determine where the warning was raised, i.e. a message similar to the one reported initally in the OP by @alexpott.
2. The test codebase uses the getMock method which is deprecated in PHPUnit 5... so tests fail and therefore more fixes to do before being able to test with PHPUnit 5.
Here I am trying the alternative
suggested by @alexpott in the OP.
Comment #19
Mile23We should move forward on #2795317: Allow PHPUnit 6+ support for object mocking
PHPUnit 5 requires PHP 5.6 or better, but we should make as many moves as possible that direction.
PHPUnit 6.1.0 is where we see the last usage of
each()
: https://github.com/sebastianbergmann/phpunit/blob/104a9bdadf763e3b94b1af...We can't adopt it any time soon, since it's PHP ^7
Unless we drop all PHP 5 versions, Drupal 8 will always show this deprecation error.
That leads us inextricably towards adding
each()
errors to the list of errors to ignore, and dropping PHP 5.5 ASAP so we can begin to move forward on PHPUnit versions.Comment #20
mondrakeSee also https://github.com/sebastianbergmann/phpunit/issues/2887
Comment #21
mondrake... and https://phpunit.de , where we can see that PHP 7.2 is only supported by PHPUnit 6. It seems trying to use Composer to update to PHPUnit 5 like in previous comments here won't be enough.
Comment #22
alexpott@mondrake yep this is exactly what I feared. I think we're going to have to move to Symfony levels of complexity which will mean having a script to run to sort out phpunit based on environment. At least, PHPUnit is not part of the tarball because that would make this impossible. However it does make including it in the lock file a bit problematic. I guess if we leave it at its current version at least Drupal will be composer installable in all PHP versions we currently support.
I think we need to do #2795317: Allow PHPUnit 6+ support for object mocking and then implement some way for people to swap between PHP versions.
This issue adds yet more weight to the importance of #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7 because doing this is going to add some serious complexity.
Comment #23
alexpottAnother consideration that might result in way less work is to fork PHPUnit 4.8.36 and fix these deprecations and any other PHP7.2 issues. Then once #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7 is done and we've actually dropped support move back to an official PHPUnit release.
Comment #24
mondrakeI tried the following:
"phpunit/phpunit": "^4.8.35 || ^5.7 || ^6.1"
in composer.jsonThe good thing with this is that under PHP 7.2 PHPUnit gets now updated to 6.5.2. So even if we composer install 4.8.36 based on the lock file, with a composer json that lets the update freely move it upwards, we can have get dynamically a test specific PHPUnit version based on the PHP version. Similarly,
Then of course now we have failures on PHPUnit 6 due to other things dropped - specifically
However, tests on PHP 5.6 with PHPUnit 5.7.25 run fine with this configuration!
See https://travis-ci.org/mondrake/imagemagick/builds/310939547
Comment #25
timmillwoodCould we package Drupal without dev dependencies, then we wouldn't have the issue? DrupalCI would run composer install getting non Dev dependencies from composer.lock, then whatever versions of Dev dependencies that are appropriate.
Comment #26
Mile23Re #23: Drupal fork of PHPUnit? You mean like we did with Simpletest? :-)
We should just have phpunit-bridge ignore the
each()
errors, because that's all we can do as long as our PHP platform requirements don't match PHPUnit 6.1 or newer. Give it a @todo to an issue about updating the PHPUnit version at some point in the future.#2795317: Allow PHPUnit 6+ support for object mocking will give us a shim to allow for PHPUnit 5. Once we have that passing, we can then have a follow-up to allow PHPUnit 5 as a version constraint, even though it's not compatible with PHP 5.5.
Our composer.lock file will hold us at PHPUnit 4 for most purposes, but if we have something like #2874198: Create and run dependency regression tests for core we can then exercise all the different PHP/PHPUnit version permutations using
composer update
.Comment #27
alexpott@Mile23
Nope I mean a fork of PHPUnit using Github and maintained under https://github.com/drupal and we already have clear plans to drop PHP5 support in the future so we know exactly when we'll be able to move to a move modern PHPUnit.
Comment #28
alexpott@mondrake #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes needs a quick review then PHPUnit 6 becomes simpler.
Comment #29
mondrake#28: the patch in #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes fixes what I reported in #24 for PHPUnit 6.
However this just moves the bar ahead...
With this configuration
"phpunit/phpunit": "^4.8.35 || ^5.7 || ^6.1"
in composer.json#2915820: [PHP 7.2] FormValidator: Parameter must be an array or an object that implements Countablewas committednow on PHPUnit 6 I get the following
and
It seems like the old class naming 'PHPUnit_Framework_****' should be replaced by namespaces all across the test codebase :(
Doing
grep 'PHPUnit_' | grep -v '*'
there's a huge list :(
Comment #30
alexpott@mondrake thanks for #29 - there's a lot of complexity in dealing with all of that. Symfony is doing things like this:
So code of ours like:
Might become something like:
Which is not particularly fun to maintain and also to explain to people that whilst they have a green test on their local environment they also need to have an eye on this other environment that is going to use a different code path. This is why I get tempted to think we should consider forking PHPUnit until we can select a future version. A big problem with changing PHPUnit version on a minor release is that you could argue that this is an API break - i.e. people will have written tests for contrib and custom that will break unless they are fixed. Super tricky.
For those interested see the PHPUnit support matrix - https://phpunit.de/ We are already pushing PHPUnit 4 in ways it is not intended. Officially it does not support PHP7 or PHP7.1 and even if we move to PHPUnit 5 support for that actually ends on February 2, 2018. Also looking at the speed nature of PHPUnit's roadmap it is likely we're going to hit similar problems in the future where there are new PHP versions not supported by our chosen version of PHPUnit and the versions of PHPUnit that do support the new PHP versions are API breaks.
So what are our options:
Whilst it is tempting to consider options 1 and 2 in light of #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7 I think this issue is the other way around. Breaking our test API ensures that contrib / custom will have to do work and tests are already expensive to maintain so I can imagine a lot of frustration if we did this. Also it means that we're just putting of the problem until it strike again in the future. Which seems likely. I worry about the cost of both 3 and 4. Whilst 4 is tempting as a quick fix to get around the each() problem we currently face we're just taking on maintaining a test library again and eventually, as @Mile23 has pointed out, that'll be painful. So whilst it pains me to think about all the complexity for core development I think we need the ability to using different versions of PHPUnit on different PHP Versions. That way our test API is dependent on PHP Version and shouldn't change. So we leave ourselves on PHPUnit 4 for PHP 7.1 because if you've got a test suite running on that we shouldn't break it. But we should work out a way of installing PHPUnit 6 on the PHP 7.2 servers and therefore core tests will have to cope with both APIs. ugh.
Comment #31
greg.1.anderson CreditAttribution: greg.1.anderson commentedRegarding the problem of managing multiple composer.lock files when testing multiple test scenarios, I made a lightweight utility that makes that easy. See https://github.com/greg-1-anderson/composer-test-scenarios for details.
Comment #32
alexpottSo here's a way to get PHPUnit 6 onto the bots whilst still having the same composer.lock file. So this would allow us to have PHPUnit 6 on 7.2 and PHPUnit 4 on 5.5 to 7.1... I've made it switch on 7.1 because it is just easier for testing since 7.2 is so broken in Drupal HEAD atm. So we need to combine this with #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes and then see if we can fix all the things to pass on both PHP 7.0 (where we will be using PHPUnit 4) and PHP 7.1 where we will be using PHPUnit 6.
/me now understands why PHPUnit is not in the dev dependencies of symfony/symfony.
Comment #33
alexpottSo let's see how this does. We have #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes and #2795317: Allow PHPUnit 6+ support for object mocking and so aliases in bootstrap.php plus some additions to Phpunit5FCTrait to support setExpectedException on PHPUnit 6. Interestingly this has revealed a tricky bug on \Drupal\Tests\Phpunit5FCTrait::getMock() and doing something like
$route_collection = $this->getMock('Symfony\Component\Routing\RouteCollection', NULL);
as seen in \Drupal\Tests\Core\EventSubscriber\SpecialAttributesRouteSubscriberTest() which I can't get to pass :(Comment #34
alexpott@greg.1.anderson thanks for the link - looks interesting and certainly something we need to do in the future for all our dependencies. The platform switch looks very useful.
Comment #35
alexpottMade a slight mistake for the PHPUnit 4 code path.
Comment #36
alexpottArgghh the result printer is still getting in the way on PHPUnit 6... a more elegant solution is probably possible - atm code duplication FTW.
Comment #37
alexpottYep we can remove all the duplicate code with a trait.
Comment #39
mondrakeGreat progress @alexpott!
From the sandbox on GitHub: https://github.com/mondrake/d8-php72
PHPUnit version ending up running the tests:
Then:
Single test
phpunit tests/Drupal/KernelTests/Core/Batch/BatchKernelTest.php --verbose
=> All GREEN in all releases/all PHPUnit versions :)Group test
phpunit --group Database --verbose
=> Runs on 5.5, 5.6, 7.0 (PHPUnit 4.8.36); fails on 7.1 and 7.2 (PHPUnit 6.5.2) withFatal error: Class 'PHPUnit_Framework_TestSuite' not found in /home/travis/drupal8/core/tests/TestSuites/TestSuiteBase.php on line 10
See https://travis-ci.org/mondrake/d8-php72/builds/311729882
Comment #40
alexpottSome more progress - will still fail on 7.1 but less hopefully.
Comment #41
mondrake@composer upgrade phpunit/phpunit --with-dependencies --no-progress would look better on the testbot log.
Comment #42
mondrakeCan we up it to ^4.8.36 which is the actual version in use on the bots (and in composer.lock)?
Also in core/tests/TestSuites/TestSuiteBase.php:
abstract class TestSuiteBase extends \PHPUnit_Framework_TestSuite {
can we
instead? That seems to be covered by the FC layer of PHPUnit 4.8.36.
Comment #44
alexpottMore progress.
Incorporated #41 and #42.2 but I didn't update the constraint in composer.json - that's not necessary we've got the lock file for that and none of the changes made require PHPUnit 4.8.36 over 4.8.35.
I think we're going to need to extend the concept of skipped deprecations to the unit test level. Currently these deprecations are handled by
Symfony\Bridge\PhpUnit\SymfonyTestsListener
so they don't get our skipping feature which was necessary to get kernel tests and browser test support for deprecations. But PHPUnit 6 has added a great new feature that if you mock a method with an @deprecated annotation it triggers a silenced error if you call the mocked method - so you get stuff likeThe Drupal\Core\Theme\ActiveTheme::getStyleSheetsRemove method is deprecated (in Drupal 8.0.0, will be removed before Drupal 9.0.0.)
when you runDrupal\Tests\Core\Asset\AssetResolverTest
. This obviously is awesome because it is helping uncover yet more tech debt. The problem is we can't simply mark the test as@legacy
and add an@expectedDeprecation
because this deprecation is not fired in PHPUnit 4.Comment #45
mondrake@alexpott I also triggered a PHP 5.5 test as I think this is breaking on that version now :(
Separate note - looks like this one will be a single elephant to digest in the end... shall #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes and #2795317: Allow PHPUnit 6+ support for object mocking be closed as dupes and this one renamed as 'PHPUnit 6 compatibility layer for PHP 7.2'?
Comment #46
alexpott@mondrake re the issue scoping I'm not sure - in some ways doing #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes and #2795317: Allow PHPUnit 6+ support for object mocking to lay the ground work for this one might be nice as whilst in order to know that they are correct we need this patch they can be done without breaking PHPUnit 4 and the existing test infra so it might be good to keep them for now. You are totally right that this issue needs a re-title and an issue summary update based on the comment in #30
Comment #47
mondrakelead to failure under PHPUnit 6 when running a
--group
testhttps://travis-ci.org/mondrake/d8-php72/jobs/311867434
Comment #48
alexpott@mondrake nice find on the --group issue. This is because the listeners are in core/tests/Drupal/Tests/Listeners and we load up all classes under core/tests/Drupal/Tests to work out if they are a test. Doh! This is not where these things should be because they are not tests. Let's see what we can do.
Comment #49
alexpottThink I've got a solution for the PHPUnit 4 vs 6 difference with respect to triggering silenced deprecations from mocked functions that have an @deprecated annotation. We can change the deprecation level to
weak_vendors
which means we ignore deprecation errors when the deprecation is triggered by code called from vendor code. As the mocks are in vendor this prevents them from causing errors.@mondrake any reason you thought PHP 5.5 was broken - it looks okay so far.
Patch attached also fixes the
--group
issue reported earlier.Comment #50
alexpottI'm not sure of the value of this alias since the class doesn't have the same methods and so we had to replace using it with something better.
Comment #51
mondrake#49 because at one point I got a
Fatal error: Access level to Drupal\Tests\Component\DependencyInjection\ContainerTest::setExpectedException() must be public (as in class PHPUnit_Framework_TestCase) in /home/travis/drupal8/core/tests/Drupal/Tests/Component/DependencyInjection/ContainerTest.php on line 1008
with the patch in #40.
See https://travis-ci.org/mondrake/d8-php72/jobs/311861472
But in later patches it's OK. Sorry for the noise.
Comment #52
alexpottre #51 yeah that was broken on PHP 7.0 - it's because I added a private setExpectedException on that component test - works fine on PHPUnit 6 but not on 4 cause the method exists in the parent there.
Comment #53
mondrake#49: wow! Both a
--group Database
and a one-shottests/Drupal/KernelTests/Core/Batch/BatchKernelTest.php
tests run smooth on all PHP versions now on the TravisCI sandbox (including PHP 7.2)https://travis-ci.org/mondrake/d8-php72/builds/311916038
Let's see what DrupalCI has to say about it.
Great job @alexpott!
Comment #54
greg.1.anderson CreditAttribution: greg.1.anderson commentedRe: #31 / #34, that project does not help solve any problems, it just helps test them. Since you've got things cleanly selecting the right dependencies on a per-php-version basis, it's not needed here. It's mostly good for libraries that are used by multiple clients.
Great job here on the solution for dependency selection based on the php version. I think that as the php ecosystem gets more complicated, the need to do this sort of thing is going to become more common. Perhaps what is needed in the future is a Composer feature that would allow us to declare conflicts based on the php version.
Comment #55
alexpottComment #56
mondrakeI made a combined patch for #49 and #2853503-48: Remove all assert('string') calls from Drupal core because deprecated in PHP 7.2, and triggered a PHP 7.2 test on it at #2874378-43: Ignore, testing issue.
Edit - result is PHP 7.2 & MySQL 5.5 22,566 pass, 381 fail
👍
Getting closer!
Comment #57
alexpottMerging in latest changes from #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes and #2795317: Allow PHPUnit 6+ support for object mocking - whilst those patches require this one to be properly tested I think it is worth landing those first so that the scope of this patch is only about the test changes and the changes to composer to install PHPUnit 6. Makes it way easier to review.
Comment #58
mondrakeLet's postpone this then?
Comment #59
alexpottSo with this and
I think we might nearly be green on PHP7.2
Here are the patches I applied to make this patch:
Comment #60
alexpottSo some new fixes to:
#2928846: [PHP 7.2] count() parameter must be an array or an object that implements Countable
#2929477: Update jcalderonzumba/mink-phantomjs-driver
Comment #61
alexpottFound another little bit for #2928846: [PHP 7.2] count() parameter must be an array or an object that implements Countable
Comment #62
mondrakePass! Bravo @alexpott 👏👍
Comment #63
mondrakeSince we skip PHPUnit 5 altogether -
phpunit.xml.dist
hascheckForUnintentionallyCoveredCode
is deprecated in favor ofbeStrictAboutCoversAnnotation
in PHPUnit 5.2.0 and was eventually removed in PHPUnit 6.0.Since we'll be jumping from PHPUnit 4.8.36 to 6.5.x, this setting will become stale with no warnings unless we remember to do something about it (add a @todo?)
Comment #64
larowlan#2795317: Allow PHPUnit 6+ support for object mocking is in
Comment #65
jibranI applied the patch and ran composer install on PHP 7.2 and saw the following error.
Also composer.lock file was changed. Are we not going to track the changes of composer.lock as a result of
"drupal-phpunit-upgrade": "@composer upgrade phpunit/phpunit --with-dependencies --no-progress",
?Comment #66
larowlanCorrect, because we don't want to pin lower php versions to the newer phpunit and php platform requirememts
Comment #67
jibranHere is a reroll of #61. Both issues mentioned in #57 are already committed to 8.5.x.
Comment #68
jibranOh! it includes #2923015: [PHP 7.2] Incompatible method declarations and #2928846: [PHP 7.2] count() parameter must be an array or an object that implements Countable now so back to postpone.
Comment #69
mondrake@jibran actually those two are fixing PHP 7.2 code issues, and are independent from this one which is about enabling running tests on PHPUnit 6 for PHP 7.2+.
Reroll shoud start from the patch in #57, maybe if @alexpott still has a separate branch for this it would be quicker for him
Comment #70
alexpottYeah I'll handle the re-rolls on this one when I have a sec.
Comment #71
alexpottSo we're still blocked on:
Here's a patch rerolled and a do-not-test patch with just the changes necessary here.
Comment #72
Mile23Reviewing the do-not-test patch in #71
This looks vaguely infinite-loop-y to me. Could it be post-install? We don't need post-update. https://getcomposer.org/doc/articles/scripts.md#event-names
Versions don't match. Or maybe they do and the logic isn't obvious.
It would be great if there could be a class constant for PHPUnit version, for ease of maintenance, but no biggie.
Also, we might consider limiting to PHPUnit <7 which is in dev and who knows what it will bring.
Two possibilities:
1) The user ran the script out of context.
2) This script is running from composer install --no-dev.
We don't really need an error in those circumstances.
Comment #73
alexpottThanks for the review @Mile23.
"phpunit/phpunit": "^4.8.35 || ^6.1",
will never install PHPUnit 7. I'm not a fan of class constants here because the scope of this information is only this method. It is not meant to be accessed from outside.I messed up #71 and didn't include \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException().
Comment #75
alexpottHere's the do not test patch to review.
Comment #76
alexpottStarted the issue summary update.
Comment #77
alexpottRerolled cause #2928846: [PHP 7.2] count() parameter must be an array or an object that implements Countable landed.
So now we have this patch and #2923015: [PHP 7.2] Incompatible method declarations (https://www.drupal.org/files/issues/2923015-42_0.patch).
Comment #78
mondrake@alexpott I launched a PHP 7 test on the uncombined patch, to see if that runs without regression.
Do we need to wait for #2923015: [PHP 7.2] Incompatible method declarations here? AFAICS this one, if in, will set the infrastructure to allow testing on PHP 7.2 in general (and for contrib too), whereas the other is the 7.2 fix for core runtime.
Is the change for version check + the todo in bootstrap.php the only things left out?
Some nits + is the expectException compatibility worth being moved to a trait somewhere?
... and 6 or above
PHPUnit 6+
s/use/using
s/PHPIUnit 6/PHPUnit 6+
this is repeating - how about moving the if/else to a trait?
PHPUnit 6+
Comment #79
Mile23What I meant in #72.2 was: This references version 6.0 for PHPUnit. We want 6.1. So instead of
(version_compare($current, '6.0') < 0)
how about(version_compare($current, '6.1') < 0)
?Upgrade aliases to update.
Comment #80
alexpottBroke out the TourTestBase changes into their own patch because to do this properly we have to add a test. Currently the web test version of TourTestBase class has no usages and that means making changes is risky but adding a test is beyond the scope here. So created #2932044: Remove \PHPUnit_Util_XML::cssSelect() from \Drupal\tour\Tests\TourTestBase
Comment #81
alexpottThanks for the reviews @mondrake and @Mile23.
Re #78
1. Addressed in #80
2. I don't think we should be making claims about PHPUnit 7 or other versions. We don't know what the API will look like.
3. Fixed
4. As point 2
5. That's not possible. These are component tests and are not using the Drupal's test class and there is no shared place to put a trait that all the components can use. In many ways I think this is preferable to our trait approach in regular Drupal because then everything is not coupled together.
6. As point 2
Re #79
1. Well I think that the PHPUnit constraint should be 6.0 - we use composer.json to restrict versions way too often with no reasoning. Changed composer.json. At the moment we're getting
6.5.5
so I'm really not sure what the 1 was about. Maybe @mondrake had a reason?2. Fixed
Also have to rename LegacyMessengerTest because any test with Legacy in the name generates deprecation notices that PHPUnit 6 can catch but PHPUnit 4 for some reason does not.
@mondrake - I think you are right this issue is no longer blocked on anything - we can forward with it we just need #2923015: [PHP 7.2] Incompatible method declarations and #2932044: Remove \PHPUnit_Util_XML::cssSelect() from \Drupal\tour\Tests\TourTestBase to land for a green PHP 7.2 test. But this change should not break PHP =< 7.1.
Comment #82
alexpottNew combined patch including https://www.drupal.org/files/issues/2923015-56.patch and https://www.drupal.org/files/issues/2932044-2.patch so we can see if PHP 7.2 is green.
Comment #83
alexpottFixed #2932044: Remove \PHPUnit_Util_XML::cssSelect() from \Drupal\tour\Tests\TourTestBase using https://www.drupal.org/files/issues/2932044-3.patch
Comment #84
mondrake#81:
the reason for PHPunit 6.1 is that that's the first version where PHPUnit itself dropped all calls to PHP 7.2 deprecated
each()
function. See #19. If we (hypothetically) used PHPUnit 6.0.x then most tests will fail on the same reason why we are trying to get this patch in :)Comment #85
Mile23Re #81:
Well that was the premise... If we care about 6.1 as minimum then we should care in both places :-)
And the reason we care is #19: PHPUnit 6.1.0 is where we see the last usage of
each()
, so we should enforce that so PHP 7.2 doesn't yell at us for deprecation reasons: https://github.com/sebastianbergmann/phpunit/blob/104a9bdadf763e3b94b1af...Edit... Uh, I should read all the way through before I reply. :-)
Comment #86
alexpottRe the PHPUnit 6.1 thing so we're incompatible with PHPUnit 6.0 - I make the same point as before composer.json should be about incompatibilities. We're not incompatible with PHPUnit 6.0 it is just that we don't want to use it for testing on PHP 7.2 and we will never do that. If you have a project which is locked to PHPUnit 6.0 and you include Drupal (or one of our components) then you shouldn't be force to upgrade.
Comment #87
alexpottRe-uploading #81 because that's the patch to review and I think finally we're in a place to go forward.
Comment #88
Mile23Our components don't have dev dependencies, and dev dependencies are only accounted for the root project anyway. So if you have some other project that uses some bit of Drupal then you're not going to expect to run Drupal's tests, and you're welcome to deal with
each()
deprecations in whatever way you see fit.If your contrib module requires a different PHPUnit version than core, then you're already out of bounds for what core can support. We extended our test frameworks to be FC/BC with PHPUnit 4, 5, and 6, so that shouldn't be a problem.
It's possible that we'd inadvertently merge a dev dependency that needs PHPUnit <6.1, and we'd want that to show up as explicit during build-time instead of test-run-time, so we can know it was a dependency rather than deprecations creeping into our code.
So therefore:
6.1 please. :-)
Comment #89
mondrakeAs long as any of the \PHPUnit_Framework_* does not exist, then we would need all of the aliases anyway. Do we need all of the separate if statements?
... also: what about #63?
Comment #90
alexpottRe #63 The
checkForUnintentionallyCoveredCode=false
was added in #2105583: Add some sane strictness to phpunit tests to catch risky tests as the default for both is false the best thing is to remove. It is not adding anything anyway.TODO set checkForUnintentionallyCoveredCode="true" once https://www.drupal.org/node/2626832 is resolved.
. We can totally add it and work out how to deal with PHPUnit 4 and 6 differences in that issue rather than try to solve it here - unnecessarily.Addressed #88 and #89's other points. Whilst I disagree with making our composer.json overly specific and 6.1 is just another example of this I don't care enough to argue it out as in the long run it is not that important given we're on 6.5 already. Also, with respect to this issue I'm edging towards the opinion that PHPUnit et al shouldn't even be in the composer.json file and we should take a further leaf out of Symfony's book on this one.
Comment #91
mondrake👍 thanks @alexpott, RTBC
Comment #92
alexpottJust related the other PHP 7.2 issue that fixes most of the remaining errors.
Comment #93
catchPHPIUNIT.
I understand not wanting to make a trait for this, since it'd mean probably a new component + dependency for each of the components needing the code. However in some tests the helper method is being copied over, in others it's inlined (even with 3-4 calls in the same class). I think we should copy the exact same method to each class that needs it (even when there's a one-to-one usage), this will then make it easier to remove again when we require PHP 6 and means that the need for it is documented everywhere it's used. Would be worth doing an @see to the trait/method for those c&ped helpers too.
Comment #94
alexpottThanks for the review @catch.
\Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException
. The reason I think this is better is because there is less obfuscation when reading the tests and we're not relying on core/tests/bootstrap.php for stuff like:Comment #95
mondrake#93 addressed.
Once we have green tests on PHP 7.2, how about making 7.2 the default for patch testing on the bots? It will have the strictest PHP syntax checking, will benefit from latest updates of PHPUnit, and will force people e.g. to care about expectedException vs. expectException
Comment #96
alexpottThe other advantage of #94 over adding a setExpectedException bc layer to component tests is that when we remove PHPUnit 4 support the patch is just removals.
Comment #98
catchCreated a child issue to remove the inline bc from component tests when phpunit 4/5 is fully dropped.
Committed 03fd77c and pushed to 8.5.x. Thanks!
Comment #99
alexpottCreated #2932606: Use PHPUnit 6 for PHP 7.0 / 7.1 testing to discuss moving all PHP 7 tests to PHPUnit 6
Comment #100
larowlanwas this intended?
Comment #101
larowlannvm, @tim.plunkett pointed out
Comment #102
neclimdulThis change doesn't seem documented in the discussion. Its causing phpunit runner to break.
Comment #103
alexpott@neclimdul yeah we need another issue for that. It's because we run each PHPUnit test individually. But the @trigger_error in Drupal\deprecation_test\Deprecation\FixtureDeprecatedClass is thrown by including the code.
\Drupal\Tests\Core\Listeners\DrupalStandardsListenerDeprecationTest
needs to be changed to use it's own piece of deprecated code. Therefore when you run all the tests via PHPUnit's runner you get a different result. This said I have never understood the point of a site running core's unit tests and doing anything with the results. These are not any other projects tests they are Drupal core's. Kernel tests fine because there are some tests that run everything including contrib and custom modules but unit tests do not and if they do then they should not. But this is also back to the end of question of responsibilities and @neclimdul and I fundamentally disagree.Comment #104
alexpott@neclimdul I opened #2933991: Deprecation tests fail when all PHPUnit tests are run via PHPUnit
Comment #105
alexpottTagging for the release notes and highlights as using PHPUnit 6 is a pretty major upgrade and also supporting PHP7.2. Writing a change record too.
Comment #107
idebr CreditAttribution: idebr at ezCompany commentedThis comment now has an incorrect instruction how to disable deprecation testing. I have added a followup to correct this at #2962736: phpunit.xml.dist has an incorrect instruction on how to disable deprecation errors