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:

  1. 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.
  2. 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.
  3. 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
CommentFileSizeAuthor
#94 2927806-94.patch76.91 KBalexpott
#94 90-94-interdiff.txt15.95 KBalexpott
#90 2927806-90.patch66.52 KBalexpott
#90 81-90-interdiff.txt5.64 KBalexpott
#87 2927806-81.patch67.04 KBalexpott
#83 2927806-81.combined.patch93.99 KBalexpott
#82 2927806-81.combined.patch93.97 KBalexpott
#81 2927806-81.patch67.04 KBalexpott
#81 77-81-interdiff.txt4.63 KBalexpott
#77 2927806-77.do-not-test.patch66.99 KBalexpott
#77 2927806-77.combined.patch90.28 KBalexpott
#75 2927806-73.do-not-test.patch68.38 KBalexpott
#73 2927806-73.combined.patch95.88 KBalexpott
#73 71-73-interdiff.txt5.56 KBalexpott
#71 2927806-71.do-not-test.patch66.72 KBalexpott
#71 2927806-71.combined.patch94.22 KBalexpott
#67 2927806-66.patch93.94 KBjibran
#61 2927806.all-teh-things3.patch130.59 KBalexpott
#60 2927806.all-teh-things2.patch130.14 KBalexpott
#59 2927806.all-teh-things.patch128.45 KBalexpott
#57 2927806-57.patch101.35 KBalexpott
#57 49-57-interdiff.txt14.21 KBalexpott
#49 2927806-49.patch100.86 KBalexpott
#49 44-49-interdiff.txt4.71 KBalexpott
#44 2927806-44.patch96.52 KBalexpott
#44 40-44-interdiff.txt31.2 KBalexpott
#40 2927806-40.patch67.59 KBalexpott
#40 37-40-interdiff.txt30.87 KBalexpott
#37 2927806-37.patch39.46 KBalexpott
#37 36-37-interdiff.txt7.3 KBalexpott
#36 2927806-36.patch39.14 KBalexpott
#36 35-36-interdiff.txt5.91 KBalexpott
#35 2927806-35.patch33.23 KBalexpott
#35 33-35-interdiff.txt1.14 KBalexpott
#33 2927806-33.patch33 KBalexpott
#32 2927806-31.patch2.85 KBalexpott
#24 2927806-24-do-not-test.patch522 bytesmondrake
#18 2927806-18.patch1.13 KBmondrake
#13 2927806-13-do-not-test.patch514 bytesmondrake
#8 2927806-8.patch515 bytesmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

catch’s picture

Priority: Normal » Critical

Bumping this to critical since it prevents a passing test suite on PHP 7.2.

mondrake’s picture

do what Symfony is doing and swap around which PHPUnit is being used depending on PHP version

That'd be the most elegant solution... would require a change to composer.json to be sth like

     "require-dev": {
          ...
 -        "phpunit/phpunit": ">=4.8.35 <5",
 +        "phpunit/phpunit": ">=4.8.35 || ^5.7"
          ...
      }

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

Wim Leers’s picture

#4: Interesting! Can you post a patch that does that?

alexpott’s picture

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

mondrake’s picture

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

mondrake’s picture

Status: Active » Needs review
FileSize
515 bytes

Here's a patch for composer.json.

Most probably, it won't do anything helpful, since on testbots we're running composer install, and we get

23:50:55 ----------------   Starting container_composer   ----------------
23:50:55 Directory created at /var/lib/drupalci/workspace/jenkins-php-7.2-apache_mysql5.5-1/ancillary/container_composer
23:50:55 Running Composer within the environment.
23:50:55 COMPOSER_ALLOW_SUPERUSER=TRUE /usr/local/bin/composer install --prefer-dist --no-suggest --no-progress --working-dir /var/www/html
23:50:55 Loading composer repositories with package information
23:50:55 Installing dependencies (including require-dev) from lock file
23:50:55 Nothing to install or update
23:50:55 Generating autoload files
23:50:55 > Drupal\Core\Composer\Composer::preAutoloadDump
23:50:55 > Drupal\Core\Composer\Composer::ensureHtaccess
23:50:56 ---------------- Finished container_composer in 0.72047591209412 ---------------- 

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

alexpott’s picture

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

mondrake’s picture

@alexpott yeah... but that's above my head. I'll pass to the next person here :)

alexpott’s picture

+++ b/core/composer.json
@@ -44,7 +44,7 @@
+        "phpunit/phpunit": ">=4.8.35 || ^5.7",

This needs to be "phpunit/phpunit": ">=4.8.35 <5 || ^5.7",

And then in the root composer.json we need to add something like:

        "post-autoload-dump": [
          "Drupal\\Core\\Composer\\Composer::ensureHtaccess",
          "$COMPOSER_BINARY require phpunit/phpunit:5.* --update-with-dependencies"
        ],

But new second command needs to be written in some way that does not cause recursion - as it does in the version above.

mondrake’s picture

How about

"phpunit/phpunit": "^4.8.35 || ^5.7"?

mondrake’s picture

FileSize
514 bytes

A do-not-test patch for #12, will do some testing on TravisCI

mondrake’s picture

So... theory works.

I'm running a TravisCI build to run tests for the Imagemagick module.

then 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

martin107’s picture

@mondrake,

A quick quesiotion when you say

plus the form validator countable

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.

mondrake’s picture

@martin107 yes, sorry, I drafted the comment and then forgot to inline the actual issue numbers. Updated.

martin107’s picture

Ah no problem ... thanks for the prompt reply.

mondrake’s picture

FileSize
1.13 KB

Did some more tests locally.

1. The

Other deprecation notices (1)

The each() function is deprecated. This message will be suppressed on further calls: 1x

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

add "The each() function is deprecated..." to the list of skipped deprecations

suggested by @alexpott in the OP.

Mile23’s picture

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

mondrake’s picture

mondrake’s picture

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

alexpott’s picture

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

alexpott’s picture

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

mondrake’s picture

FileSize
522 bytes

I tried the following:

The 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,

  • PHP 5.6 => PHPUnit updates to 5.7.25
  • PHP 7.0 => PHPUnit updates to 6.5.2
  • PHP 7.1 => PHPUnit updates to 6.5.2

Then of course now we have failures on PHPUnit 6 due to other things dropped - specifically

PHP Fatal error:  Class 'PHPUnit_Framework_BaseTestListener' not found in /home/travis/drupal8/core/tests/Drupal/Tests/Listeners/DeprecationListener.php on line 12

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

timmillwood’s picture

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

Mile23’s picture

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

alexpott’s picture

@Mile23

Drupal fork of PHPUnit? You mean like we did with Simpletest? :-)

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.

alexpott’s picture

mondrake’s picture

#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

now on PHPUnit 6 I get the following

Fatal error: Declaration of Drupal\Tests\Listeners\DeprecationListener::endTest(PHPUnit_Framework_Test $test, $time) must be compatible with PHPUnit\Framework\TestListener::endTest(PHPUnit\Framework\Test $test, $time) in /home/travis/drupal8/core/tests/Drupal/Tests/Listeners/DeprecationListener.php on line 14

and

Fatal error: Class 'PHPUnit_Framework_TestSuite' not found in /home/travis/drupal8/core/tests/TestSuites/TestSuiteBase.php on line 10

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

core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php:    $this->setExpectedException(\PHPUnit_Framework_AssertionFailedError::class);
core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php:    $this->setExpectedException(\PHPUnit_Framework_AssertionFailedError::class);
core/modules/image/tests/src/Kernel/ImageItemTest.php:      $this->assertInstanceOf(\PHPUnit_Framework_Error_Warning::class, $exception->getPrevious());
core/modules/simpletest/src/TestDiscovery.php:use PHPUnit_Util_Test;
core/modules/simpletest/src/TestDiscovery.php:    $annotations = PHPUnit_Util_Test::parseTestMethodAnnotations($class->getName())['class'];
core/modules/simpletest/src/TestDiscovery.php:    // @see PHPUnit_Util_Test::getRequirements()
core/modules/simpletest/tests/fixtures/phpunit_error.xml:        <error type="PHPUnit_Framework_Error_Notice">Drupal\Tests\Core\Extension\ModuleHandlerUnitTest::testloadInclude
core/modules/simpletest/tests/fixtures/phpunit_error.xml:          <failure type="PHPUnit_Framework_ExpectationFailedException">Drupal\Tests\Core\Route\RoleAccessCheckTest::testRoleAccess with data set #0 ('role_test_1', array(Drupal\user\Entity\User, Drupal\user\Entity\User))
core/modules/simpletest/tests/fixtures/phpunit_error.xml:          <failure type="PHPUnit_Framework_ExpectationFailedException">Drupal\Tests\Core\Route\RoleAccessCheckTest::testRoleAccess with data set #1 ('role_test_2', array(Drupal\user\Entity\User, Drupal\user\Entity\User))
core/modules/simpletest/tests/fixtures/phpunit_error.xml:          <failure type="PHPUnit_Framework_ExpectationFailedException">Drupal\Tests\Core\Route\RoleAccessCheckTest::testRoleAccess with data set #2 ('role_test_3', array(Drupal\user\Entity\User))
core/modules/simpletest/tests/src/FunctionalJavascript/BrowserWithJavascriptTest.php:    $this->setExpectedException(\PHPUnit_Framework_AssertionFailedError::class);
core/modules/tour/src/Tests/TourTestBase.php:          $elements = \PHPUnit_Util_XML::cssSelect('#' . $tip['data-id'], TRUE, $this->content, TRUE);
core/modules/tour/src/Tests/TourTestBase.php:          $elements = \PHPUnit_Util_XML::cssSelect('.' . $tip['data-class'], TRUE, $this->content, TRUE);
core/modules/tour/tests/src/Functional/TourTestBase.php:          $elements = \PHPUnit_Util_XML::cssSelect('#' . $tip['data-id'], TRUE, $raw_content, TRUE);
core/modules/tour/tests/src/Functional/TourTestBase.php:          $elements = \PHPUnit_Util_XML::cssSelect('.' . $tip['data-class'], TRUE, $raw_content, TRUE);
core/modules/views/tests/src/Kernel/ViewExecutableTest.php:    catch (\PHPUnit_Framework_Error_Warning $e) {
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
core/tests/Drupal/KernelTests/Core/Test/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_SkippedTestError $e) {
core/tests/Drupal/KernelTests/Core/Test/BrowserTestBaseTest.php:    catch (\PHPUnit_Framework_SkippedTestError $e) {
core/tests/Drupal/KernelTests/KernelTestBase.php:        throw new \PHPUnit_Framework_Exception("Unavailable module: '$name'. If this module needs to be downloaded separately, annotate the test class with '@requires module $name'.");
core/tests/Drupal/KernelTests/KernelTestBaseTest.php:    catch (\PHPUnit_Framework_SkippedTestError $e) {
core/tests/Drupal/KernelTests/KernelTestBaseTest.php:    catch (\PHPUnit_Framework_SkippedTestError $e) {
core/tests/Drupal/Tests/Component/Diff/Engine/DiffOpTest.php:    $this->setExpectedException(\PHPUnit_Framework_Error::class);
core/tests/Drupal/Tests/Component/DrupalComponentTest.php:    catch (\PHPUnit_Framework_AssertionFailedError $e) {
core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php:use PHPUnit_Framework_Error_Warning;
core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php:    $this->setExpectedException(PHPUnit_Framework_Error_Warning::class, 'mkdir(): Permission Denied');
core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php:use PHPUnit_Framework_ExpectationFailedException;
core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php:    $this->setExpectedException(PHPUnit_Framework_ExpectationFailedException::class);
core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php:    $this->setExpectedException(PHPUnit_Framework_ExpectationFailedException::class);
core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php:    $this->setExpectedException(PHPUnit_Framework_ExpectationFailedException::class);
core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php:    $this->setExpectedException(PHPUnit_Framework_ExpectationFailedException::class);
core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php:    $this->setExpectedException(PHPUnit_Framework_ExpectationFailedException::class);
core/tests/Drupal/Tests/Core/Config/ConfigTest.php:    $this->setExpectedException(\PHPUnit_Framework_Error_Warning::class);
core/tests/Drupal/Tests/Core/Database/ConditionTest.php:    $this->setExpectedException(\PHPUnit_Framework_Error::class);
core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php:    $this->setExpectedException(\PHPUnit_Framework_Error::class);
core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php:    $this->setExpectedException(\PHPUnit_Framework_Error::class);
core/tests/Drupal/Tests/Core/EventSubscriber/SpecialAttributesRouteSubscriberTest.php:    $this->setExpectedException(\PHPUnit_Framework_Error_Warning::class, 'uses reserved variable names');
core/tests/Drupal/Tests/Core/Menu/ContextualLinkManagerTest.php:      ->with($this->equalTo('contextual_links'), new \PHPUnit_Framework_Constraint_Count(2), $this->equalTo('group1'), $this->equalTo(['key' => 'value']));
core/tests/Drupal/Tests/Core/Plugin/DefaultSingleLazyPluginCollectionTest.php:  protected function setupPluginCollection(\PHPUnit_Framework_MockObject_Matcher_InvokedRecorder $create_count = NULL) {
core/tests/Drupal/Tests/Core/Plugin/LazyPluginCollectionTestBase.php:  protected function setupPluginCollection(\PHPUnit_Framework_MockObject_Matcher_InvokedRecorder $create_count = NULL) {
core/tests/Drupal/Tests/Core/Render/ElementTest.php:    $this->setExpectedException(\PHPUnit_Framework_Error::class, '"foo" is an invalid render array key');
core/tests/Drupal/Tests/Listeners/DeprecationListener.php:class DeprecationListener extends \PHPUnit_Framework_BaseTestListener {
core/tests/Drupal/Tests/Listeners/DeprecationListener.php:  public function endTest(\PHPUnit_Framework_Test $test, $time) {
core/tests/Drupal/Tests/Listeners/DrupalComponentTestListener.php:  public function endTest(\PHPUnit_Framework_Test $test, $time) {
core/tests/Drupal/Tests/Listeners/DrupalComponentTestListener.php:        $error = new \PHPUnit_Framework_AssertionFailedError('Component tests should not extend a core test base class.');
core/tests/Drupal/Tests/Listeners/DrupalStandardsListener.php:    $fail = new \PHPUnit_Framework_AssertionFailedError($message);
core/tests/Drupal/Tests/Listeners/DrupalStandardsListener.php:  public function endTest(\PHPUnit_Framework_Test $test, $time) {
core/tests/Drupal/Tests/Listeners/DrupalStandardsListener.php:    // \PHPUnit_Framework_Test does not have any useful methods of its own for
core/tests/Drupal/Tests/Listeners/DrupalStandardsListener.php:    elseif ($test instanceof \PHPUnit_Framework_TestSuite) {
core/tests/Drupal/Tests/Listeners/HtmlOutputPrinter.php:class HtmlOutputPrinter extends \PHPUnit_TextUI_ResultPrinter {
core/tests/Drupal/Tests/Listeners/HtmlOutputPrinter.php:  public function printResult(\PHPUnit_Framework_TestResult $result) {
core/tests/Drupal/Tests/TestRequirementsTrait.php:        throw new \PHPUnit_Framework_SkippedTestError('Required modules: ' . implode(', ', $not_available));
core/tests/TestSuites/TestSuiteBase.php:abstract class TestSuiteBase extends \PHPUnit_Framework_TestSuite {
alexpott’s picture

@mondrake thanks for #29 - there's a lot of complexity in dealing with all of that. Symfony is doing things like this:

                $expectedExceptionClass = 'Symfony\\Component\\Finder\\Exception\\AccessDeniedException';
                if ($e instanceof \PHPUnit_Framework_ExpectationFailedException) {
                    $this->fail(sprintf("Expected exception:\n%s\nGot:\n%s\nWith comparison failure:\n%s", $expectedExceptionClass, 'PHPUnit_Framework_ExpectationFailedException', $e->getComparisonFailure()->getExpectedAsString()));
                }

                if ($e instanceof \PHPUnit\Framework\ExpectationFailedException) {
                    $this->fail(sprintf("Expected exception:\n%s\nGot:\n%s\nWith comparison failure:\n%s", $expectedExceptionClass, '\PHPUnit\Framework\ExpectationFailedException', $e->getComparisonFailure()->getExpectedAsString()));
                }

So code of ours like:

    // Test that the assertion fails correctly.
    try {
      $this->assertFieldByXPath("//input[@id = 'notexisting']");
      $this->fail('The "notexisting" field was found.');
    }
    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
      $this->pass('assertFieldByXPath correctly failed. The "notexisting" field was not found.');
    }

Might become something like:

    // Test that the assertion fails correctly.
    try {
      $this->assertFieldByXPath("//input[@id = 'notexisting']");
      $this->fail('The "notexisting" field was found.');
    }
    catch (\PHPUnit_Framework_ExpectationFailedException $e) {
      $this->pass('assertFieldByXPath correctly failed. The "notexisting" field was not found.');
    }
    catch (ExpectationFailedException $e) {
      $this->pass('assertFieldByXPath correctly failed. The "notexisting" field was not found.');
    }

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:

  1. Ditch PHP 5.5 testing and move to PHPUnit 5 - way less API breaks than moving to PHPUnit and we can do things like #2795317: Allow PHPUnit 6+ support for object mocking to smooth the ride for contrib and custom. However this does not officially support 7.2 and it is using code that is deprecated in 7.2 and therefore will break when 7.3 is out.
  2. 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.
  3. Go the Symfony solution and swap out 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.
  4. Fork PHPUnit 4 and make it work on all the PHP versions Drupal 8 is going to support.

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.

greg.1.anderson’s picture

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

alexpott’s picture

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

alexpott’s picture

So 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 :(

alexpott’s picture

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

alexpott’s picture

Made a slight mistake for the PHPUnit 4 code path.

alexpott’s picture

Argghh the result printer is still getting in the way on PHPUnit 6... a more elegant solution is probably possible - atm code duplication FTW.

alexpott’s picture

Yep we can remove all the duplicate code with a trait.

The last submitted patch, 33: 2927806-33.patch, failed testing. View results

mondrake’s picture

Great progress @alexpott!

From the sandbox on GitHub: https://github.com/mondrake/d8-php72

PHPUnit version ending up running the tests:

  • PHP 5.5, 5.6, 7.0 => PHPUnit 4.8.36
  • PHP 7.1, 7.2 => PHPUnit 6.5.2

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) with Fatal 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

alexpott’s picture

Some more progress - will still fail on 7.1 but less hopefully.

mondrake’s picture

+++ b/composer.json
@@ -50,10 +50,12 @@
+        "drupal-phpunit-upgrade": "@composer upgrade phpunit/phpunit --with-dependencies",

@composer upgrade phpunit/phpunit --with-dependencies --no-progress would look better on the testbot log.

mondrake’s picture

+++ b/core/composer.json
@@ -44,7 +44,7 @@
+        "phpunit/phpunit": "^4.8.35 || ^6.1",

Can 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

...
use PHPUnit\Framework\TestSuite;
...
abstract class TestSuiteBase extends TestSuite {

instead? That seems to be covered by the FC layer of PHPUnit 4.8.36.

Status: Needs review » Needs work

The last submitted patch, 40: 2927806-40.patch, failed testing. View results

alexpott’s picture

More 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 like The Drupal\Core\Theme\ActiveTheme::getStyleSheetsRemove method is deprecated (in Drupal 8.0.0, will be removed before Drupal 9.0.0.) when you run Drupal\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.

mondrake’s picture

@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'?

alexpott’s picture

@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

mondrake’s picture

  1. +++ b/core/tests/Drupal/Tests/Listeners/Legacy/HtmlOutputPrinter.php
    @@ -0,0 +1,33 @@
    +class HtmlOutputPrinter extends \PHPUnit_TextUI_ResultPrinter {
    
  2. +++ b/core/tests/Drupal/Tests/Listeners/Legacy/HtmlOutputPrinter.php
    @@ -0,0 +1,33 @@
    +  public function printResult(\PHPUnit_Framework_TestResult $result) {
    

lead to failure under PHPUnit 6 when running a --group test

https://travis-ci.org/mondrake/d8-php72/jobs/311867434

alexpott’s picture

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

alexpott’s picture

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

alexpott’s picture

+++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
@@ -50,14 +50,13 @@ public function assertTourTips($tips = []) {
-          $elements = \PHPUnit_Util_XML::cssSelect('#' . $tip['data-id'], TRUE, $raw_content, TRUE);
+          $elements = $this->getSession()->getPage()->find('css', '#' . $tip['data-id']);

+++ b/core/tests/bootstrap.php
@@ -166,3 +166,38 @@ function drupal_phpunit_populate_class_loader() {
+if (!class_exists('\PHPUnit_Util_XML') && class_exists('\PHPUnit\Util\XML')) {
+  class_alias('\PHPUnit\Util\XML', '\PHPUnit_Util_XML');
+}

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

mondrake’s picture

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

alexpott’s picture

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

mondrake’s picture

#49: wow! Both a --group Database and a one-shot tests/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!

greg.1.anderson’s picture

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

alexpott’s picture

Title: PHPUnit is using each() and this is deprecated in PHP7.2 » Use PHPUnit 6 for testing when PHP version >= 7.2
mondrake’s picture

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

alexpott’s picture

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

mondrake’s picture

alexpott’s picture

mondrake’s picture

Pass! Bravo @alexpott 👏👍

mondrake’s picture

Since we skip PHPUnit 5 altogether -

phpunit.xml.dist has

<phpunit bootstrap="tests/bootstrap.php" colors="true"
         beStrictAboutTestsThatDoNotTestAnything="true"
         beStrictAboutOutputDuringTests="true"
         beStrictAboutChangesToGlobalState="true"
         checkForUnintentionallyCoveredCode="false">

checkForUnintentionallyCoveredCode is deprecated in favor of beStrictAboutCoversAnnotation 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?)

larowlan’s picture

Title: [PP-2] Use PHPUnit 6 for testing when PHP version >= 7.2 » [PP-1] Use PHPUnit 6 for testing when PHP version >= 7.2
jibran’s picture

I applied the patch and ran composer install on PHP 7.2 and saw the following error.

Fatal error: Cannot declare class PHPUnit\Runner\Version, because the name is already in use in vendor/phpunit/phpunit/src/Runner/Version.php on line 18
PHP Fatal error:  Cannot declare class PHPUnit\Runner\Version, because the name is already in use in vendor/phpunit/phpunit/src/Runner/Version.php on line 18
Script @composer upgrade phpunit/phpunit --with-dependencies --no-progress handling the drupal-phpunit-upgrade event returned with error code 255
Script Drupal\Core\Composer\Composer::upgradePHPUnit handling the post-autoload-dump event terminated with an exception

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",?

larowlan’s picture

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",?

Correct, because we don't want to pin lower php versions to the newer phpunit and php platform requirememts

jibran’s picture

Title: [PP-1] Use PHPUnit 6 for testing when PHP version >= 7.2 » Use PHPUnit 6 for testing when PHP version >= 7.2
Status: Postponed » Needs review
FileSize
93.94 KB

Here is a reroll of #61. Both issues mentioned in #57 are already committed to 8.5.x.

jibran’s picture

Title: Use PHPUnit 6 for testing when PHP version >= 7.2 » [PP-2] Use PHPUnit 6 for testing when PHP version >= 7.2
Status: Needs review » Postponed
mondrake’s picture

Title: [PP-2] Use PHPUnit 6 for testing when PHP version >= 7.2 » Use PHPUnit 6 for testing when PHP version >= 7.2
Status: Postponed » Needs work
Issue tags: +Needs reroll

@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

alexpott’s picture

Yeah I'll handle the re-rolls on this one when I have a sec.

Mile23’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Reviewing the do-not-test patch in #71

  1. +++ b/composer.json
    @@ -50,10 +50,12 @@
             "post-autoload-dump": [
    -          "Drupal\\Core\\Composer\\Composer::ensureHtaccess"
    +          "Drupal\\Core\\Composer\\Composer::ensureHtaccess",
    +          "Drupal\\Core\\Composer\\Composer::upgradePHPUnit"
    

    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

  2. +++ b/core/composer.json
    @@ -44,7 +44,7 @@
    +        "phpunit/phpunit": "^4.8.35 || ^6.1",
    
    +++ b/core/lib/Drupal/Core/Composer/Composer.php
    @@ -143,6 +144,24 @@ public static function ensureHtaccess(Event $event) {
    +    if (version_compare(PHP_MAJOR_VERSION . '.' . PHP_MINOR_VERSION, '7.1') >= 0 && version_compare($phpunit_version, '6.0') < 0) {
    

    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.

  3. +++ b/core/lib/Drupal/Core/Composer/Composer.php
    @@ -143,6 +144,24 @@ public static function ensureHtaccess(Event $event) {
    +  public static function upgradePHPUnit(Event $event) {
    ...
    +    // @todo if we haven't determine version throw an error.
    

    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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.56 KB
95.88 KB

Thanks for the review @Mile23.

  1. Yeah that is a better event - it's not infinite-loop-y because of the PHPUnit version test. Fixed.
  2. We need to change 7.1 to 7.2 once the patch is ready to go. I just have it at 7.1 to make life a bit simpler for me. Since I only have 7.2 RC2 locally. "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.
  3. I've improved the PHPUnit version detection to make this not an issue anymore.

I messed up #71 and didn't include \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException().

Status: Needs review » Needs work

The last submitted patch, 73: 2927806-73.combined.patch, failed testing. View results

alexpott’s picture

FileSize
68.38 KB

Here's the do not test patch to review.

alexpott’s picture

Issue summary: View changes

Started the issue summary update.

mondrake’s picture

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

  1. +++ b/core/modules/tour/src/Tests/TourTestBase.php
    @@ -14,6 +14,17 @@
    +    // Support both PHPUnit 4 and 6.
    

    ... and 6 or above

  2. +++ b/core/scripts/run-tests.sh
    @@ -793,7 +793,11 @@ function simpletest_script_run_one_test($test_id, $test_class) {
    +      // This also prevents PHPUnit 6's mock code that triggers silenced
    

    PHPUnit 6+

  3. +++ b/core/scripts/run-tests.sh
    @@ -793,7 +793,11 @@ function simpletest_script_run_one_test($test_id, $test_class) {
    +      // this to 'strict' once we are no longer use PHPUnit 4.
    

    s/use/using

  4. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -497,6 +497,11 @@ protected function setUp() {
    +    // PHPIUnit 6 tests that only make assertions using $this->assertSession()
    

    s/PHPIUnit 6/PHPUnit 6+

  5. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
    @@ -242,7 +253,12 @@ public function testDateFormat($input, $timezone, $format, $format_date, $expect
    +    if (method_exists($this, 'expectException')) {
    +      $this->expectException($class);
    +    }
    +    else {
    +      $this->setExpectedException($class);
    +    }
    

    this is repeating - how about moving the if/else to a trait?

  6. +++ b/core/tests/Drupal/Tests/Component/DependencyInjection/ContainerTest.php
    @@ -983,6 +983,31 @@ protected function getCollection($collection, $resolve = TRUE) {
    +   * Compatibility layer for PHPUnit 6 to support PHPUnit 4 code.
    

    PHPUnit 6+

Mile23’s picture

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -143,6 +143,32 @@ public static function ensureHtaccess(Event $event) {
+    // If the PHP version is 7.1 or above and PHPUnit is less than version 6
+    // call the drupal-phpunit-upgrade script to upgrade PHPUnit.
+    if (version_compare(PHP_MAJOR_VERSION . '.' . PHP_MINOR_VERSION, '7.1') >= 0 && version_compare($phpunit_package->getVersion(), '6.0') < 0) {

What 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)?

+++ b/composer.json
@@ -49,11 +49,11 @@
+        "drupal-phpunit-upgrade": "@composer upgrade phpunit/phpunit --with-dependencies --no-progress",

Upgrade aliases to update.

alexpott’s picture

Broke 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

alexpott’s picture

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

alexpott’s picture

mondrake’s picture

Status: Needs review » Needs work

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

Mile23’s picture

Re #81:

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?

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

alexpott’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
67.04 KB

Re-uploading #81 because that's the patch to review and I think finally we're in a place to go forward.

Mile23’s picture

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.

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

+++ b/core/composer.json
@@ -44,7 +44,7 @@
+        "phpunit/phpunit": "^4.8.35 || ^6.0",

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -143,6 +143,32 @@ public static function ensureHtaccess(Event $event) {
+    // If the PHP version is 7.2 or above and PHPUnit is less than version 6
+    // call the drupal-phpunit-upgrade script to upgrade PHPUnit.
+    if (version_compare(PHP_MAJOR_VERSION . '.' . PHP_MINOR_VERSION, '7.2') >= 0 && version_compare($phpunit_package->getVersion(), '6.0') < 0) {

6.1 please. :-)

mondrake’s picture

+++ b/core/tests/bootstrap.php
@@ -166,3 +166,41 @@ function drupal_phpunit_populate_class_loader() {
+  class_alias('\PHPUnit\Framework\AssertionFailedError', '\PHPUnit_Framework_AssertionFailedError');
+}
+if (!class_exists('\PHPUnit_Framework_Error_Warning') && class_exists('\PHPUnit\Framework\Error\Warning')) {
+  class_alias('\PHPUnit\Framework\Error\Warning', '\PHPUnit_Framework_Error_Warning');
+}
+if (!class_exists('\PHPUnit_Framework_Error') && class_exists('\PHPUnit\Framework\Error\Error')) {

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

alexpott’s picture

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

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

👍 thanks @alexpott, RTBC

alexpott’s picture

Just related the other PHP 7.2 issue that fixes most of the remaining errors.

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -497,6 +497,11 @@ protected function setUp() {
    +    // PHPIUnit 6 tests that only make assertions using $this->assertSession()
    

    PHPIUNIT.

  2. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
    @@ -87,7 +87,13 @@ public function testDateDiff($input1, $input2, $absolute, \DateInterval $expecte
       public function testInvalidDateDiff($input1, $input2, $absolute) {
    -    $this->setExpectedException(\BadMethodCallException::class, 'Method Drupal\Component\Datetime\DateTimePlus::diff expects parameter 1 to be a \DateTime or \Drupal\Component\Datetime\DateTimePlus object');
    +    if (method_exists($this, 'expectException')) {
    +      $this->expectException(\BadMethodCallException::class);
    +      $this->expectExceptionMessage('Method Drupal\Component\Datetime\DateTimePlus::diff expects parameter 1 to be a \DateTime or \Drupal\Component\Datetime\DateTimePlus object');
    +    }
    +    else {
    +      $this->setExpectedException(\BadMethodCallException::class, 'Method Drupal\Component\Datetime\DateTimePlus::diff expects parameter 1 to be a \DateTime or \Drupal\Component\Datetime\DateTimePlus object');
    +    }
         $interval = $input1->diff($input2, $absolute);
    

    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.

alexpott’s picture

Thanks for the review @catch.

  1. Fixed
  2. Actually I think we should go the other way. I was just being a bit lazy and copying the BC in from \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:
    if (method_exists($this, 'expectException')) {
      $this->expectException(Error::class);
    }
    else {
      $this->setExpectedException(\PHPUnit_Framework_Error::class);
    }
    
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

#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

alexpott’s picture

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

  • catch committed 03fd77c on 8.5.x
    Issue #2927806 by alexpott, mondrake, jibran, Mile23: Use PHPUnit 6 for...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Created 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!

alexpott’s picture

Created #2932606: Use PHPUnit 6 for PHP 7.0 / 7.1 testing to discuss moving all PHP 7 tests to PHPUnit 6

larowlan’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Messenger/MessengerTest.php
@@ -11,7 +11,7 @@
-class LegacyMessengerTest extends KernelTestBase {
+class MessengerTest extends KernelTestBase {

was this intended?

larowlan’s picture

nvm, @tim.plunkett pointed out

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.

neclimdul’s picture

+++ b/core/tests/Drupal/Tests/Core/Listeners/DrupalStandardsListenerDeprecationTest.php
@@ -21,6 +21,7 @@
+ * @group legacy

@@ -29,6 +30,8 @@ class DrupalStandardsListenerDeprecationTest extends UnitTestCase {
+   * @expectedDeprecation Drupal\deprecation_test\Deprecation\FixtureDeprecatedClass is deprecated.
+   *

This change doesn't seem documented in the discussion. Its causing phpunit runner to break.

alexpott’s picture

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

alexpott’s picture

alexpott’s picture

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

Status: Fixed » Closed (fixed)

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

idebr’s picture

+++ b/core/phpunit.xml.dist
@@ -30,7 +29,7 @@
-    <!-- <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled"/> -->
+    <env name="SYMFONY_DEPRECATIONS_HELPER" value="weak_vendors"/>

This 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