Problem/Motivation

This issue is about allowing to optionally use PHPUnit ^7 to run tests in Drupal 8, while keeping support for PHPUnit ^6.5 .

  1. PHPUnit 6 is out of support since Feb 1, 2019. So we are already using an unsupported PHPUnit version in our testing ecosystem.
  2. PHPUnit has a very strict policy of releasing a new major in line with PHP releases, and supporting only current and current-1 major:

The problem to solve to support PHPUnit 7 vis-à-vis PHPUnit 6 is that it changes the signature of a number of its methods and removes some that were deprecated earlier, making it impossible to extend its classes in a way that can work seamlessly between the 2 versions. In a pratical example, PHPUnit 6 declares PHPUnit\TextUI\ResultPrinter::printResult like

public function printResult(TestResult $result);

whereas PHPUnit 7 like

public function printResult(TestResult $result): void;

so you cannot extend for example from ResultPrinter and override a method in a unique way, because in one of the versions PHP will step in and throw a fatal error for the different type hinting.

Several classes and methods in the current test codebase are extending and overriding directly from PHPUnit classes and are subject to this issue - listeners, asserts, etc.

Proposed resolution

Create a bridge between PHPUnit and the test codebase, that making use of class_alias loads classes and traits depending on the PHPUnit runner version. These classes/traits adhere to the runner version required type hinting, and are conveniently grouped in namespaces organized by runner version - for PHPUnit6 and PHPUnit7 here. This allows to (a) easily drop support for a PHPUnit version by removing all the classes of a PHPUnitX bridge and upping the composer constraint, and (b) add support for future PHPUnit versions by adding PHPUnit8, PHPUnit9 and so forth bridges - when time comes.

AFAICT, the concept is generally BC, as it allows to run the tests on both PHPUnit 6 and on PHPUnit 7 with a single test codebase, as latest patches show. Contrib might need to adjust some assertions, though, or tests that depend on the internals of PHPUnit.

It would not be necessary to force the composer.lock to be updated. We can just loosen the composer.json constraint to allow PHPUnit 7 to be installed, similiarly to the approach that is being taken, AFAICS, for #2976394: Allow Symfony 4.4 to be installed in Drupal 8. Then hi/lo dependencies testing will ensure running on both versions.

Remaining tasks

review

User interface changes

none

API changes

none

Data model changes

none

Release notes snippet

Drupal 8 now supports PHPUnit 6.5+ and PHPUnit 7. PHPUnit 6 will be used by default, but PHPUnit 7 is required for testing on PHP 7.4. If you are using core's root composer file, you can update to PHPUnit 7 by running composer run drupal-phpunit-upgrade. PHPUnit 7 is required for testing on PHP 7.4.

Original report:

I know that the testing infrastructure just switched to phpunit 6.5 two month ago but in the meantime PHPUnit 7 was released.

so the deadline to switch, and some important dates

so the deadline is like ~8 months from now based on PHP 7.3 release.

can PHPUnit 7 be used from now?

so it seems 2018 Nov-Dec is the date to switch..

.. or start using 3 different versions of phpunit now:

PHP 5.5; 5.6      > PHPUnit 4 (4.8)
PHP 7.0           > PHPUnit 6 (6.5)
PHP 7.1; 7.2; 7.3 > PHPUnit 7
CommentFileSizeAuthor
#162 2950132-2-162.patch55.3 KBalexpott
#159 2950132-2-159.patch55.31 KBalexpott
#151 2950132-2-151.patch53.71 KBalexpott
#150 2950132-2-150.patch55.73 KBalexpott
#147 2950132-2-147.patch58.12 KBalexpott
#147 145-147-interdiff.txt783 bytesalexpott
#145 2950132-2-144.patch58.24 KBalexpott
#145 138-144-interdiff.txt851 bytesalexpott
#138 2950132-2-138.patch57.41 KBalexpott
#138 136-138-interdiff.txt4.02 KBalexpott
#136 2950132-136.patch54.74 KBalexpott
#136 134-136-interdiff.txt19.53 KBalexpott
#134 2950132-134.patch54.18 KBalexpott
#134 133-134-interdiff.txt8.49 KBalexpott
#133 2950132-133-DO-NOT-COMMIT.patch47.1 KBmondrake
#132 2950132-132-DO-NOT-COMMIT.patch47.01 KBmondrake
#132 interdiff_131-132.txt481 bytesmondrake
#131 interdiff_126-130.txt18.9 KBmondrake
#131 2950132-130-DO-NOT-COMMIT.patch47.01 KBmondrake
#126 interdiff_124-126.txt3.26 KBmondrake
#126 2950132-126-DO-NOT-COMMIT.patch46.94 KBmondrake
#124 2950132-124.patch47.01 KBmondrake
#124 interdiff_115-124.txt4.55 KBmondrake
#116 2950132-116-no-drupalci-changes.patch46.46 KBmondrake
#115 interdiff_110-115.txt1.66 KBmondrake
#115 2950132-115.patch47.16 KBmondrake
#115 2950132-115-no-drupalci-changes.patch47.16 KBmondrake
#110 interdiff_104-110.txt752 bytesmondrake
#110 2950132-110.patch46.02 KBmondrake
#110 2950132-110-no-drupalci-changes.patch45.32 KBmondrake
#104 2950132-104.patch45.84 KBmondrake
#103 2950132-103.patch47.51 KBmondrake
#102 2950132-102.patch47.53 KBmondrake
#101 2950132-101.patch47.43 KBmondrake
#101 interdiff_100-101.txt561 bytesmondrake
#100 2950132-100.patch47.38 KBmondrake
#98 2950132-98.patch47.21 KBmondrake
#98 interdiff_97-98.txt3.2 KBmondrake
#97 2950132-97.patch45.72 KBmondrake
#97 interdiff_96-97.txt10.05 KBmondrake
#96 interdiff_95-96.txt13.26 KBmondrake
#96 2950132-96.patch48.01 KBmondrake
#95 2950132-95.patch47.25 KBmondrake
#89 2950132-88.patch49.04 KBmondrake
#89 interdiff_87-88.txt1.3 KBmondrake
#87 interdiff_86-87.txt2.34 KBmondrake
#87 2950132-87.patch48.78 KBmondrake
#86 2950132-86.patch46.44 KBmondrake
#86 interdiff_83-86.txt6.78 KBmondrake
#83 2950132-83.patch55.16 KBmondrake
#81 2950132-81.patch62.68 KBmondrake
#77 2950132-77.patch62.07 KBmondrake
#77 interdiff_74-77.txt3.88 KBmondrake
#76 2950132-76.patch61.85 KBmondrake
#76 interdiff_74-76.txt3.67 KBmondrake
#74 interdiff_70-74.txt33.95 KBmondrake
#74 2950132-74.patch58.85 KBmondrake
#70 2950132-70.patch56.23 KBmondrake
#70 interdiff_68-70.txt13.58 KBmondrake
#68 2950132-68.patch54.27 KBmondrake
#68 interdiff_66-68.txt625 bytesmondrake
#66 2950132-66.patch54.26 KBmondrake
#66 interdiff_64-66.txt636 bytesmondrake
#64 2950132-64.patch54.25 KBmondrake
#42 2950132-42.patch54.1 KBmondrake
#42 interdiff_36-42.txt3.41 KBmondrake
#36 2950132-36.patch50.69 KBmondrake
#36 interdiff_32-36.patch5.92 KBmondrake
#32 interdiff_25-32.txt5.94 KBmondrake
#32 2950132-32.patch49.56 KBmondrake
#25 2950132-25.patch47.09 KBmondrake
#25 interdiff_23-25.txt3.47 KBmondrake
#23 interdiff_21-23.txt6.8 KBmondrake
#23 2950132-23.patch44.36 KBmondrake
#21 interdiff_19-21.txt18.07 KBmondrake
#21 2950132-21.patch41.48 KBmondrake
#19 2950132-19.patch33.65 KBmondrake
#15 2950132-use-phpunit-7-reroll-15.patch14.64 KBtrzcinski.t@gmail.com
#8 2950132-8.patch14.52 KBmondrake
#8 interdiff_6-8.txt817 bytesmondrake
#6 interdiff_4-6.txt1.87 KBmondrake
#6 2950132-6.patch14.62 KBmondrake
#4 2950132-4.patch12.75 KBmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pasqualle’s picture

Pasqualle created an issue. See original summary.

Pasqualle’s picture

Issue summary: View changes
Pasqualle’s picture

Issue summary: View changes
mondrake’s picture

The big problem here is that PHPUnit 7 changes the signature of a number of methods and removes some that were deprecated earlier.

I have been trying to see the extent of the changes; patch attached that should allow running quite a lot of the test base on PHP 7.2 + PHPUnit 7, but mostly here to document the changes needed somehow.

Status: Needs review » Needs work

The last submitted patch, 4: 2950132-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Trying to fix some fails.

Status: Needs review » Needs work

The last submitted patch, 6: 2950132-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 8: 2950132-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

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

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

I think we also need to find out if PHPUnit 6 is really incompatible with PHP 7.3 before embarking on this. Also I think we should consider going the Symfony route and not having PHPUnit in our composer.json.

catch’s picture

jibran’s picture

andypost’s picture

Queued last patch for php 7.3 tests

trzcinski.t@gmail.com’s picture

Hi,

it was not applying for the latest 8.6.x branch - rerolled it. There was an update in the composer.json file and that caused it to fail.

Setting tests to 8.6.x for now. I will be testing that more thoroughly since I want to write tests on PHPUnit7.

alexpott’s picture

So in answer to #11 - we've not hit any problems what-so-ever in using PHPUnit 6 with PHP 7.3 - so the urgency to upgrade is not there atm.

I think we need to plan for the following in Drupal 9

Also I think we should consider going the Symfony route and not having PHPUnit in our composer.json.

This solves a number of problems about supporting and testing on a wide range of PHP versions.

mondrake’s picture

PhpUnit 8 was released today, does it warrant a separate issue of its own?

andypost’s picture

PHPUnit 8 receives bug fixes until February 5, 2021
PHPUnit 7 receives bug fixes until February 7, 2020

mondrake’s picture

Some work in the direction of loading conditionally traits and/or classes depending on the PHPUnit Runner version. We need to avoid loading code that uses PHP7 syntax in PHP5, or we get compiler errors. No interdiff as changes are significant. Any comment on the approach welcome.

Status: Needs review » Needs work

The last submitted patch, 19: 2950132-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Status: Needs work » Needs review
FileSize
41.48 KB
18.07 KB

Some progress.

Status: Needs review » Needs work

The last submitted patch, 21: 2950132-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Status: Needs work » Needs review
FileSize
44.36 KB
6.8 KB

More.

Status: Needs review » Needs work

The last submitted patch, 23: 2950132-23.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Status: Needs work » Needs review
FileSize
3.47 KB
47.09 KB

More.

alexpott’s picture

@mondrake - I think we need to re-evaluate whether we use PHPUnit via composer. I'm quite sure that we need to take Symfony's approach and use their simple-phpunit so that we can decouple our dependencies from our test framework. We're moving at different speeds and want to support a wider range of PHP versions.

mondrake’s picture

Thanks @alexpott.

we need to take Symfony's approach and use their simple-phpunit so that we can decouple our dependencies from our test framework

Is that for building the dependencies outside of Composer, or is there more to it? In previous patches I'm more focusing on getting tests pass on various PHPUnit versions that are using different signatures of the same methods, so not so much related to the building but rather to the execution.

Status: Needs review » Needs work

The last submitted patch, 25: 2950132-25.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

Is that for building the dependencies outside of Composer,

Yep exactly. I'm not saying this work is not valid - just trying to say that we should probably solve that first because it might influence how do things. Also not having PHP 5 will make this easier. So 8.8.x is probably the place to get the ball really rolling here. But the progress is looking good! Only 10 fails left.

There's also the issue on min max testing which feels relevant here as well.

mondrake’s picture

OK - then shouldn't we have a separate issue opened for 'Use Symphony simple-phpunit to include PHPUnit and dependencies in the test codebase build'?

Also - isn't #2976407: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT worth being bumped to critical?

I understand we will have to wait for 8.8.x for this, no worries. In the meantime the closer we can get the better, I think.

mondrake’s picture

mondrake’s picture

Status: Needs work » Needs review
FileSize
49.56 KB
5.94 KB

Some more test fixes. Looks like class_alias() works with Traits, too, so we could rework the rather unconventional way of loading traits conditionally I used in the patches so far.

Status: Needs review » Needs work

The last submitted patch, 32: 2950132-32.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

mondrake’s picture

Re #32

we could rework the rather unconventional way of loading traits conditionally I used in the patches so far

done that.

mondrake’s picture

Status: Needs work » Needs review

The last submitted patch, 36: interdiff_32-36.patch, failed testing. View results

mondrake’s picture

I'll pause for now - the remaining failures are related to tests that are testing the test classes :) Those would need some work that it's not useful to pursue now.

With this patch, you could reasonably use PHPUnit7 for 'normal' tests.

In the meantime, reviews of the two spinoff issues #3031466: EntityResourceTestBase::assert406Response uses a wrong assertion and results in false positives (and fails for PHPUnit 7) and #3031539: FileFieldTestBase tests fail under PHPUnit 7 would be very helpful as having those in 8.7.x would help later work. Actually, I think #3031466: EntityResourceTestBase::assert406Response uses a wrong assertion and results in false positives (and fails for PHPUnit 7) just surfaces an issue with the current tests themselves.

Status: Needs review » Needs work

The last submitted patch, 36: 2950132-36.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Status: Needs work » Needs review
FileSize
3.41 KB
54.1 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 42: 2950132-42.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Gábor Hojtsy’s picture

Title: Use PHPUnit 7 » Support PHPUnit 7 in Drupal 8
Issue tags: +Drupal 9

Updating title to be more accurate based on what is going on.

Pasqualle’s picture

https://phpunit.de/supported-versions.html

So, phpunit 6 is not supported any more, and even composer started to complain about it #3033761: Package phpunit/phpunit-mock-objects is abandoned

What should be the recommended step for developers now? Should we update to phpunit 7 (or 8), or should we remove phpunit from composer?

gillesbailleux’s picture

@Pasqualle: thank you for raising the concern about composer. Using composer version 1.8.3, the following message appears when typingcomposer updateon a fresh D8 installation:

Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.

rivimey’s picture

Found this issue for the same reason as #45; it would be nice to see mention of a plan, or is the plan "wait for 8.8?"

Berdir’s picture

Nothing is broken, it's just an annoying message. The project is still there and won't be deleted, the maintainer just made it *very* clear that there will be no further fixes/changes there.

So yes, this will likey definitely have to wait until 8.8 because updating that is likely going to break a decent amount of tests in core and contrib.

jedgar1mx’s picture

It definitely seems like a low priority, but it would be nice not to see it on my logs lol

philosurfer’s picture

clean_logs++

jatinkumar1989’s picture

I am also facing same issue, below is my composer. json part:

    "require": {
        "composer/installers": "^1.0.24",
        "wikimedia/composer-merge-plugin": "^1.4",
      "league/csv": "^8.0",
        "drupal/console": "@stable",
        "symfony/phpunit-bridge": ">=3.2.14 <5",
        "phpunit/phpunit": "^4.8.35 || ^6.5 || ^7",
    }

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Just created an issue to move to simple-phpunit - #3039344: Use Symfony's simple-phpunit

mondrake’s picture

Title: Support PHPUnit 7 in Drupal 8 » [PP-1] Support PHPUnit 7 in Drupal 8
Status: Needs work » Postponed
mondrake’s picture

It looks like in Symfony's simple-phpunit the earliest release that will support PHPUnit 7 will be Symfony 5, as I can see it's only loaded in the master branch at the moment. Symfony 4.2 is using PHPUnit 6.5 only for PHP 7.2+.

alexpott’s picture

@mondrake which is absolutely fine because it works.

mondrake’s picture

@alexpott sure, just reporting here to say this issue may not be needed until Symfony 5 is supported.

fgm’s picture

FWIW, a side-effect is that this locks usage of sebastian/phpcpd at 3.0.x : versions 4.x require php-timer:^2.0 which is not compatible with phpunit/phpunit:^6 (requiring php-timer:^1.0).

jibran’s picture

Title: [PP-1] Support PHPUnit 7 in Drupal 8 » Support PHPUnit 7 in Drupal 8
Status: Postponed » Needs work

Blocker is in.

jibran’s picture

Title: Support PHPUnit 7 in Drupal 8 » [PP-1] Support PHPUnit 7 in Drupal 8
Status: Needs work » Postponed
NikLP’s picture

So for the less savvy users out there, is the current course of action for "Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested" (using php 7.3) basically "do nothing" at the moment?

acb’s picture

It feels to me that relying on a unitTesting framework that is no longer supported is not the wisest course of action. Drupal already has a reputation for lagging, and this is yet another example of it. As a general principle, we should stay current. As a matter of practicality, downloading and installing all sorts of no-longer-supported code is a headache, and prone to break.

From https://phpunit.de/getting-started/phpunit-6.html:

Please note that PHPUnit 6 is no longer supported. Also note that PHP 7.0 is no longer supported.

alexpott’s picture

@acb it is not practical to follow PHPUnit's release schedule. PHP 7.0 will receive years more support via the Ubuntu LTS programme regardless of PHPUnit's documentation. We are not alone, Symfony 3 is still tested using PHPUnit 4 on PHP 5.3 for example. We are taking efforts to synchronise our supported PHP versions with PHP's release schedule but it is not simple.

@NikLP yes do nothing is exactly the right course of action. Unfortunately there's nothing we can do about this message whilst we are still testing using PHPUnit 4.

We are working on decoupling core from the test framework see #3039344: Use Symfony's simple-phpunit - once we are no longer dependent on PHPUnit these issues will not be so visible for users.

mondrake’s picture

Status: Postponed » Needs review
FileSize
54.25 KB

#42 no longer applied, reroll

mondrake’s picture

Status: Needs review » Postponed
mondrake’s picture

Status: Postponed » Needs review
FileSize
636 bytes
54.26 KB

Declaration of Drupal\Tests\Listeners\SimpletestUiPrinter7::write($buffer): void should be compatible with PHPUnit\Util\Printer::write(string $buffer): void

Status: Needs review » Needs work

The last submitted patch, 66: 2950132-66.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Status: Needs work » Needs review
FileSize
625 bytes
54.27 KB

Declaration of Drupal\Tests\Listeners\DrupalListener7::endTest(PHPUnit\Framework\Test $test, $time): void must be compatible with PHPUnit\Framework\TestListener::endTest(PHPUnit\Framework\Test $test, float $time): void

mondrake’s picture

Status: Needs review » Postponed
mondrake’s picture

Reroll after the commits of

#3053363: Remove support for PHP 5 in Drupal 8.8
#3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x)

Since drupal-phpunit-upgrade and drupal-phpunit-upgrade-check are now gone, added a container_command to drupalci.yml to require the upgrade of PHPUnit & friends.

mondrake’s picture

Status: Postponed » Needs review

... and the status change ...

Status: Needs review » Needs work

The last submitted patch, 70: 2950132-70.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Title: [PP-1] Support PHPUnit 7 in Drupal 8 » [PP-3] Support PHPUnit 7 in Drupal 8
Issue summary: View changes
Status: Needs work » Postponed
mondrake’s picture

This should be green now.

1. With one major of PHPUnit released every year, I think there should be a way to allow permanently bridging different versions of PHPUnit with the test suites.
For this purpose, I suggest to create a separate namespace for the shims necessary for each version. This code must not come across PHPUnit test discovery otherwise we risk it gets autoloaded and then we incur in issues like #3034015: Class to test deprecation error for deprecated trait triggers deprecation error itself.
So here I am putting everything into Drupal\Core\Test\PhpUnitCompatibility\PhpUnit6 (or ...7).

2. This is not dependent on #3039344: Use Symfony's simple-phpunit, here we just run PHPUnit 7 regardless of how the codebase is built.

Status: Needs review » Needs work

The last submitted patch, 74: 2950132-74.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
61.85 KB

Fix for failing run on PHPUnit6.

mondrake’s picture

mondrake’s picture

alexpott’s picture

+++ b/core/modules/file/tests/src/Functional/FileFieldTestBase.php
@@ -210,7 +210,7 @@ public function replaceNodeFile($file, $field_name, $nid, $new_revision = TRUE)
-  public static function assertFileExists($file, $message = NULL) {
+  public static function assertManagedFileExists($file, $message = NULL) {
...
     parent::assertFileExists($filename, $message);

@@ -237,7 +237,7 @@ public function assertFileEntryExists($file, $message = NULL) {
-  public static function assertFileNotExists($file, $message = NULL) {
+  public static function assertManagedFileNotExists($file, $message = NULL) {
...
     parent::assertFileNotExists($filename, $message);

No longer necessary to call the parent because the names have changed.

mondrake’s picture

Status: Needs review » Postponed

#79 thanks, it's a shortcut here, please review #3031539: FileFieldTestBase tests fail under PHPUnit 7 instead. This hunk will just go away with that in.

mondrake’s picture

mondrake’s picture

Status: Needs review » Postponed
mondrake’s picture

Title: [PP-3] Support PHPUnit 7 in Drupal 8 » [PP-2] Support PHPUnit 7 in Drupal 8
Issue summary: View changes
Status: Postponed » Needs review
FileSize
55.16 KB
mondrake’s picture

Status: Needs review » Postponed
mondrake’s picture

Title: [PP-2] Support PHPUnit 7 in Drupal 8 » [PP-1] Support PHPUnit 7 in Drupal 8
Issue summary: View changes
mondrake’s picture

mondrake’s picture

Deprecation test won't work in PHPUnit7 since assertFileExists only accepts string argument for $file, fails otherwise.

mondrake’s picture

mondrake’s picture

mondrake’s picture

mondrake’s picture

Title: [PP-1] Support PHPUnit 7 in Drupal 8 » [PP-2] Support PHPUnit 7 in Drupal 8
Issue summary: View changes
Related issues: +#3062122: phpunit: Code Coverage Fix Up
Wim Leers’s picture

Just wanted to say thanks for this epic work! 🙏 This looks really complicated, I'm grateful that somebody is pushing this forward! 👏

mondrake’s picture

#92 thank you!

This looks really complicated

The real fun will come when trying to support PHPUnit 8... Preview @ #3063887: Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7 :)

Mile23’s picture

+++ b/core/drupalci.yml
@@ -15,6 +15,10 @@ build:
+      # Update PHPUnit & friends.
+      container_command:
+        commands:
+          - "sudo -u www-data /usr/local/bin/composer update phpunit/phpunit symfony/phpunit-bridge phpspec/prophecy symfony/yaml --with-dependencies --no-progress"

DrupalCI has a composer step, so you can do:

composer:
  options: update phpunit/phpunit symfony/phpunit-bridge phpspec/prophecy symfony/yaml --with-dependencies --no-progress

But what we really should do is add the drupal-phpunit-upgrade script back to composer.json, since that's established in everyone's minds. Even though they kind of hate it. :-)

mondrake’s picture

Title: [PP-2] Support PHPUnit 7 in Drupal 8 » Support PHPUnit 7 in Drupal 8
Issue summary: View changes
Status: Postponed » Needs review
FileSize
47.25 KB
mondrake’s picture

Adding an helper class/method to determine the major version of PHPUnit being run, plus done some optimization/cleanup.

@Mile23 - re. #94:

DrupalCI has a composer step, so you can do

that does not seem to be documented, at least not in https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes...

In any case, IMHO in my view we should not be focusing on how the codebase is built in this issue, there are at least #3039344: Use Symfony's simple-phpunit and #2976407: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT for that or other issues may be opened to cover the DrupalCI integration. Ideally, in my view, the patch here should just do the tweaks to allow running on PHPUnit 7 if someone wants to; projects testing on other CI platforms may be interested to be free in making their choice of which PHPUnit version they want to use for testing.

mondrake’s picture

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

needs a reroll

mondrake’s picture

mondrake’s picture

mondrake’s picture

mondrake’s picture

mondrake’s picture

Gábor Hojtsy’s picture

Title: Support PHPUnit 7 in Drupal 8 » Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

First, thanks for this epic work. I hit this when debugging https://github.com/drupal8-rector/drupal8-rector/issues/20 -- updating to PHPUnit 7 would allow to include rector/rector in a Drupal project and thus modernize the drupalmoduleupgrader infrastructure to be based on rector rather than the abandoned pharborist.

IMHO it would be quite important to explain in the issue summary how PHPUnit 7 support is achieved while keeping PHPUnit 6.x support for PHP 7.0.x users. Whether any changes are considered BC breaking here, etc. I think that would greatly enhance the chances of landing this :)

mondrake’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated IS.

mondrake’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs framework manager review, +Needs release manager review

Thanks for doing that. The changes look good to me although this will definitely need to have a release manager / framework manager collaboration to land.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
  1. +++ b/core/drupalci.yml
    @@ -15,6 +15,10 @@ build:
    +      # Update PHPUnit & friends.
    +      container_command:
    +        commands:
    +          - "sudo -u www-data /usr/local/bin/composer update phpunit/phpunit symfony/phpunit-bridge phpspec/prophecy symfony/yaml --with-dependencies --no-progress"
    

    Should this be part of the rtbc patch? I guess so - other but the comment could be more complete and say when we expect things to be updated. So once PHP 7.0 support is dropped we'd no longer be testing on PHPUnit6. The fact is that whilst PHPUnit 6 is not official supported on PHP 7.3 and greater it is happily working for us on PHP 7.3.

    I'm wary of dropping testing using PHPUnit 6 on PHP 7.3 because many people might have sites built that test using that combination at the moment. And it likely that PHP 7.3 will be supported for quite sometime. A massive positive of this patch is that it allows these sites to upgrade to a supported version of PHPUnit - but this opens the door to us breaking existing support PHPUnit 6 once we no longer test on it (i.e. the moment PHP 7.0 is dropped). Tricky.

  2. This issue needs an accompanying change record that mentions that Drupal builds using PHP 7.1, 7.2, 7.3 will now get PHPUnit 7 if they run composer upgrade (unless there is something else that fixes the PHPUnit version).
  3. +++ b/core/lib/Drupal/Core/Test/PhpUnitCompatibility/RunnerVersion.php
    @@ -0,0 +1,22 @@
    +abstract class RunnerVersion {
    

    In other frameworks this would be a class and not abstract. And it would have something like

        /**
         * This class should not be instantiated.
         */
        private function __construct() {
        }
    

    Sometimes such a class would also be made final.

    See \Zend\Diactoros\HeaderSecurity
    and \Symfony\Component\Process\ProcessUtils

  4. The implementation looks fine to me - it shows how adding returning types is not going be simple for BC for Drupal :) - lots of repeated code but I'm not sure there's a better way around this.
mondrake’s picture

Thanks for the reviews.

Re #109:

1. IMO we should not commit the drupalci changes. Rather, we stick to PHPUnit 6.5 in regular testing on all PHP versions. Then, #3044155: PHP7.1 vendor max Testing issue - DO NOT COMMIT (or in the future, other hi/lo dependencies testing features) will run daily the highest dependency testing, including upgrade to PHPUnit 7.
2. Let's draft the CR once there is agreement on 1. - in that case the CR will only mention that PHPUnit ^7 can be used for testing, still with ^6.5 being the baseline testing environment.
3. Done here.
4. :) and :(

Here with two patches, one with and one without the DrupalCI changes. The one with no changes is for commit if 1. has consensus.

alexpott’s picture

@mondrake +1 to the plan to leave this to min/max - that does mean we have no explicit test coverage but min max testing exists in the rtbc atm so I'm not sure what else we can do. It's certainly the least disruptive. If we do this then the CR only needs to point out the PHPUNit 7 is supported and will be installed if you run composer update and if you project has a requirement on PHPUnit 6 then you need to fix that in your own root composer.

mondrake’s picture

The last submitted patch, 110: 2950132-110-no-drupalci-changes.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Ouch, #110 failure hurts. Drupal\Tests\ComposerIntegrationTest fails because it compares the composer hash of the current composer.lock file vs its rebuilt one from the current composer.json file. Since in the patch without the drupalci changes we change the composer.json file to allow PHPUnit ^7 without adjusting the lock file, the test fails. I think we can only change the composer.lock hash based on the results of the test... Then I think with min/max testing and composer update happening before the actual run of the tests things should work fine. AFAICS any future patch trying to loosen composer.json constraints will suffer from this...

mondrake’s picture

mondrake’s picture

mondrake’s picture

@Gábor Hojtsy re #105: with the patch in #116 it is possible to install drupal8-rector/drupal8-rector in the codebase build - see https://github.com/drupal8-rector/drupal8-rector/issues/20#issuecomment-...

amitgoyal’s picture

Thanks @mondrake and @gábor-hojtsy, with the patch in #116, we were able to install drupal8-rector/drupal8-rector.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the change record and for the fixes! Let's get this in then!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Test/PhpUnitCompatibility/PhpUnit6/FileFieldTestBaseTrait.php
    @@ -0,0 +1,57 @@
    +  public static function assertFileExists($file, $message = NULL) {
    +    if ($file instanceof FileInterface) {
    +      @trigger_error('Passing a File entity as $file argument to FileFieldTestBase::assertFileExists is deprecated in drupal:8.8.0. It will be removed from drupal:9.0.0. Instead, pass the File entity URI via File::getFileUri(). See https://www.drupal.org/node/3057326', E_USER_DEPRECATED);
    +      $file = $file->getFileUri();
    +    }
    +    $message = isset($message) ? $message : new FormattableMarkup('File %file exists on the disk.', ['%file' => $file]);
    +    parent::assertFileExists($file, $message);
    +  }
    ...
    +  public static function assertFileNotExists($file, $message = NULL) {
    +    if ($file instanceof FileInterface) {
    +      @trigger_error('Passing a File entity as $file argument to FileFieldTestBase::assertFileNotExists is deprecated in drupal:8.8.0. It will be removed from drupal:9.0.0. Instead, pass the File entity URI via File::getFileUri(). See https://www.drupal.org/node/3057326', E_USER_DEPRECATED);
    +      $file = $file->getFileUri();
    +    }
    +    $message = isset($message) ? $message : new FormattableMarkup('File %file exists on the disk.', ['%file' => $file]);
    +    parent::assertFileNotExists($file, $message);
    +  }
    
    +++ b/core/lib/Drupal/Core/Test/PhpUnitCompatibility/PhpUnit7/FileFieldTestBaseTrait.php
    @@ -0,0 +1,12 @@
    +<?php
    +
    +namespace Drupal\Core\Test\PhpUnitCompatibility\PhpUnit7;
    +
    +/**
    + * Makes Drupal's test API forward compatible with multiple versions of PHPUnit.
    + */
    +trait FileFieldTestBaseTrait {
    +
    +  // @todo remove in Drupal 9.
    +
    +}
    

    So this essentially means that we've broken this functionality when PHPUnit7 is in play. At least the error will be straight forward and simple to fix but it does mean that tests that are passing (with deprecations) are not passing when you swap to PHPUnit7. At the very least this needs to go into the issue summary and change record. And it probably needs further discussion.

  2. +++ b/core/lib/Drupal/Core/Test/PhpUnitCompatibility/PhpUnit6/SimpletestUiPrinter.php
    @@ -0,0 +1,26 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function write($buffer) {
    +    $buffer = Html::escape($buffer);
    +    // Turn HTML output URLs into clickable link <a> tags.
    +    $url_pattern = '@https?://[^\s]+@';
    +    $buffer = preg_replace($url_pattern, '<a href="$0" target="_blank" title="$0">$0</a>', $buffer);
    +    // Make the output readable in HTML by breaking up lines properly.
    +    $buffer = nl2br($buffer);
    +
    +    print $buffer;
    +  }
    
    +++ b/core/lib/Drupal/Core/Test/PhpUnitCompatibility/PhpUnit7/SimpletestUiPrinter.php
    @@ -0,0 +1,26 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function write(string $buffer): void {
    +    $buffer = Html::escape($buffer);
    +    // Turn HTML output URLs into clickable link <a> tags.
    +    $url_pattern = '@https?://[^\s]+@';
    +    $buffer = preg_replace($url_pattern, '<a href="$0" target="_blank" title="$0">$0</a>', $buffer);
    +    // Make the output readable in HTML by breaking up lines properly.
    +    $buffer = nl2br($buffer);
    +
    +    print $buffer;
    +  }
    

    Let's add this to the HtmlOutputPrinterTrait as ::SimpletestUiWrite() - the trait is already used by the parent classes.

  3. +++ b/core/lib/Drupal/Core/Test/PhpUnitCompatibility/PhpUnit6/StubTestSuiteBaseTrait.php
    @@ -0,0 +1,22 @@
    +<?php
    +
    +namespace Drupal\Core\Test\PhpUnitCompatibility\PhpUnit6;
    +
    +/**
    + * Makes Drupal's test API forward compatible with multiple versions of PHPUnit.
    + */
    +trait StubTestSuiteBaseTrait {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function addTestFiles($filenames) {
    +    // We stub addTestFiles() because the parent implementation can't deal with
    +    // vfsStream-based filesystems due to an error in
    +    // stream_resolve_include_path(). See
    +    // https://github.com/mikey179/vfsStream/issues/5 Here we just store the
    +    // test file being added in $this->testFiles.
    +    $this->testFiles = array_merge($this->testFiles, $filenames);
    +  }
    +
    +}
    
    +++ b/core/lib/Drupal/Core/Test/PhpUnitCompatibility/PhpUnit7/StubTestSuiteBaseTrait.php
    @@ -0,0 +1,22 @@
    +<?php
    +
    +namespace Drupal\Core\Test\PhpUnitCompatibility\PhpUnit7;
    +
    +/**
    + * Makes Drupal's test API forward compatible with multiple versions of PHPUnit.
    + */
    +trait StubTestSuiteBaseTrait {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function addTestFiles($filenames): void {
    +    // We stub addTestFiles() because the parent implementation can't deal with
    +    // vfsStream-based filesystems due to an error in
    +    // stream_resolve_include_path(). See
    +    // https://github.com/mikey179/vfsStream/issues/5 Here we just store the
    +    // test file being added in $this->testFiles.
    +    $this->testFiles = array_merge($this->testFiles, $filenames);
    +  }
    +
    +}
    

    This feels wrong place to do this. This only exists for the \Drupal\Tests\Core\Test\TestSuiteBaseTest() - we can fix this by doing a conditional in the main in that test and not pollute core with this test only code. Ie. do something like

    
    if (RunnerVersion::getMajor() < 7) {
      /**
       * Stub subclass of TestSuiteBase.
       *
       * We use this class to alter the behavior of TestSuiteBase so it can be
       * testable.
       */
      class StubTestSuiteBase extends TestSuiteBase {
    
        /**
         * Test files discovered by addTestsBySuiteNamespace().
         *
         * @var string[]
         */
        public $testFiles = [];
    
        /**
         * {@inheritdoc}
         */
        protected function findExtensionDirectories($root) {
          // We have to stub findExtensionDirectories() because we can't inject a
          // vfsStream filesystem into drupal_phpunit_find_extension_directories(),
          // which uses \SplFileInfo->getRealPath(). getRealPath() resolves
          // stream-based paths to an empty string. See
          // https://github.com/mikey179/vfsStream/wiki/Known-Issues
          return [];
        }
    
        /**
         * {@inheritdoc}
         */
        public function addTestFiles($filenames) {
          // We stub addTestFiles() because the parent implementation can't deal with
          // vfsStream-based filesystems due to an error in
          // stream_resolve_include_path(). See
          // https://github.com/mikey179/vfsStream/issues/5 Here we just store the
          // test file being added in $this->testFiles.
          $this->testFiles = array_merge($this->testFiles, $filenames);
        }
    
      }
    }
    else {
      /**
       * Stub subclass of TestSuiteBase.
       *
       * We use this class to alter the behavior of TestSuiteBase so it can be
       * testable.
       */
      class StubTestSuiteBase extends TestSuiteBase {
    
        /**
         * Test files discovered by addTestsBySuiteNamespace().
         *
         * @var string[]
         */
        public $testFiles = [];
    
        /**
         * {@inheritdoc}
         */
        protected function findExtensionDirectories($root) {
          // We have to stub findExtensionDirectories() because we can't inject a
          // vfsStream filesystem into drupal_phpunit_find_extension_directories(),
          // which uses \SplFileInfo->getRealPath(). getRealPath() resolves
          // stream-based paths to an empty string. See
          // https://github.com/mikey179/vfsStream/wiki/Known-Issues
          return [];
        }
    
        /**
         * {@inheritdoc}
         */
        public function addTestFiles($filenames): void {
          // We stub addTestFiles() because the parent implementation can't deal with
          // vfsStream-based filesystems due to an error in
          // stream_resolve_include_path(). See
          // https://github.com/mikey179/vfsStream/issues/5 Here we just store the
          // test file being added in $this->testFiles.
          $this->testFiles = array_merge($this->testFiles, $filenames);
        }
    
      }
    }
    

    Yes it is ugly but all the ugliness is in the test and doesn't spill over into core.

  4. +++ b/core/lib/Drupal/Core/Test/PhpUnitCompatibility/PhpUnit7/TestCompatibilityTrait.php
    @@ -0,0 +1,48 @@
    +  /**
    +   * @see \Drupal\simpletest\TestBase::assertTrue()
    +   */
    +  public static function assertTrue($actual, string $message = ''): void {
    +    if (is_bool($actual)) {
    +      parent::assertTrue($actual, $message);
    +    }
    +    else {
    +      parent::assertNotEmpty($actual, $message);
    +    }
    +  }
    +
    +  /**
    +   * @see \Drupal\simpletest\TestBase::assertFalse()
    +   */
    +  public static function assertFalse($actual, string $message = ''): void {
    +    if (is_bool($actual)) {
    +      parent::assertFalse($actual, $message);
    +    }
    +    else {
    +      parent::assertEmpty($actual, $message);
    +    }
    +  }
    

    We should file an issue to explore deprecating this behaviour.

  5. +++ b/core/modules/field/tests/src/Kernel/Number/NumberItemTest.php
    @@ -72,8 +72,8 @@ public function testNumberItem() {
    -    $this->assertEqual($entity->field_decimal->value, $decimal);
    -    $this->assertEqual($entity->field_decimal[0]->value, $decimal);
    +    $this->assertEquals((float) $decimal, $entity->field_decimal->value);
    +    $this->assertEquals((float) $decimal, $entity->field_decimal[0]->value);
    
    +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
    @@ -152,7 +152,7 @@ public function testNode() {
    -    $this->assertEquals('1', $node->field_float->value);
    +    $this->assertEquals(1, $node->field_float->value);
         $this->assertEquals('5', $node->field_integer->value);
    

    This looks really odd. So to do the comparison to a float we have to change a string to an int but and string comparison to int below works just fine. Also changing the order in just this one place whilst leaving the rest of the file alone is confusing. I think it is better to have the wrong order and fix the argument order in another issue.

    I guess that as pointed out in #98 float comparisons have changed. A couple of thoughts are:

    • Should we implement the old behaviour in our compatibility traits but deprecate it?
    • Doesn't this mean we always in danger of causing regressions if we don't test with PHPUnit 7 somewhere?
  6. +++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
    @@ -85,7 +85,7 @@ public function testComposerJson() {
    -    $this->assertSame($content_hash, $lock['content-hash']);
    +    $this->assertSame($lock['content-hash'], $content_hash);
    

    This change is out-of-scope and hopefully unnecessary.

alexpott’s picture

mondrake’s picture

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
4.55 KB
47.01 KB

re #121:

1. Leaving open for further discussion. I do not see ways around that ATM. Not changed the IS or CR yet.
2. I am not sure I understand - open. Can you please elaborate?
3. I am afraid we can't do that as long as we support PHP 7.0 - typehinting function return values to void was introduced in 7.1.
4. Added @todos to the referred issue.
5. Reverted the order of arguments, and returned at using assertEqual. Do I understand right the proposal would be to modify TestCompatibilityTrait::assertEquals to cast both expected and actual values to float if either one or the other are float (via is_float)? Should also a TestCompatibilityTrait::assertNotEquals method be added to match assertNotEqual?
6. Reverted. Agree it's out-of-scope here. Anyway, IMO the status in HEAD is wrong - $lock['content-hash'] is the expected value as read from the composer.json file, and as such it should be the first argument in an assertSame.

alexpott’s picture

re #124.3 well then we should put the stubs in a fixtures directory with different names for the PHPUnit versions. You're right my solution doesn't work but there are other solutions that don't result with this ending up in the main Drupal\Core\Test\PhpUnitCompatibility namespace.

re #124.2 I mean add

  /**
   * @todo
   */
  protected function SimpletestUiWrite($buffer) {
    $buffer = Html::escape($buffer);
    // Turn HTML output URLs into clickable link <a> tags.
    $url_pattern = '@https?://[^\s]+@';
    $buffer = preg_replace($url_pattern, '<a href="$0" target="_blank" title="$0">$0</a>', $buffer);
    // Make the output readable in HTML by breaking up lines properly.
    $buffer = nl2br($buffer);

    print $buffer;
  }

to core/tests/Drupal/Tests/Listeners/HtmlOutputPrinterTrait.php
and then in then core/lib/Drupal/Core/Test/PhpUnitCompatibility/PhpUnit6/SimpletestUiPrinter.php (and the Phpunit7 version) change the write method to

  /**
   * {@inheritdoc}
   */
  public function write($buffer) {
    $this->simpletestUiWrite($buffer);
  }

Also I've just realised that the new compatibility classes have moved to core/lib/Drupal/Core/Test/PhpUnitCompatibility - these should not be here. They have no business being autoloadable during general runtime. They can stay in core/tests/Drupal/Tests a sub directory in there should be fine.

mondrake’s picture

#121.1 - still open for discussion, I can update IS and draft CR if there is consensus we stay as is
#121.2 - Got it, thanks. Done here.
#121.3 - IMO these thingos are neither core nor test code. The suggestion to move these to a fixture directory is a reflection of that... Also,

the new compatibility classes have moved to core/lib/Drupal/Core/Test/PhpUnitCompatibility - these should not be here. They have no business being autoloadable during general runtime. They can stay in core/tests/Drupal/Tests a sub directory in there should be fine

but the point is that in that way the thingos will be autoloaded during the test dicovery since they are in a test namespace, and if the syntax is not supported by a specific PHP version, then we will fail badly. This may be less evident now, but in earlier exercises when we were still testing with PHP 5 that was big. An idea could be to add a, say, phpunit subdirectory to the core subdirectory and move the thingos there (maybe listeners, too, in a separate issue), and somehow register this in the test bootstrap.php file as a separate namespace, so that we can still use autoloader without starting reguire_once __DIR__ dances. Something discussed about this with @Mile23 in #2905007-68: Allow run-tests.sh to report skipped/risky/incomplete PHPUnit-based tests and #69.
#121.4 - OK
#121.5 - Second time thinking here - IMO PHP and therefore PHPUnit are becoming stricter in type checking, and I think we should move along with that instead of trying to relax our testing. So I'd go for just mentioning in IS and CR that assertEqual for float checking need to be revised in contrib tests. That is the direction that #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests is taking, no?
#121.6 - OK, do we need a follow-up to revert the order of the arguments in ComposerIntegrationTest?

alexpott’s picture

@mondrake I'm not sure that

autoloaded during the test dicovery since they are in a test namespace

is true. We only try to instantiate classes which end in *Test.php during test discovery as far as I know. And if this is not true then we need to explore solutions that don't pollute the runtime autoloader with these classes.

mondrake’s picture

#127 right that happens when running tests via run-tests.sh and DrupalCI. But you might run tests with PHPUnit directly, and when it runs --testsuite(s) or --group(s), PHPUnit autoloads those classes, at least it was doing in #3034015: Class to test deprecation error for deprecated trait triggers deprecation error itself. Happy to be wrong, though ... :)

mondrake’s picture

BTW

we need to explore solutions that don't pollute the runtime autoloader with these classes

fully agree here

mondrake’s picture

mondrake’s picture

Re #121.3 and #125

Here I am introducing a new namespace Drupal/Tool from separate core/tools subdirectory, and moving all the compatibility code there. This way we do not pollute Drupal/Core and as well we keep it away from Drupal/Test. Just an idea, naming and location can obviously change.

Oustanding: #121.1, 3, 5, 6.

mondrake’s picture

mondrake’s picture

alexpott’s picture

As we need this for PHP 7.4 compatibility we need to go back to a way of switching between PHPUnit version. This patch adds back in the composer script to update PHPUnit and if we're on PHP 7.3 or above switches to PHPUnit 7. I choose PHP 7.3 so we can validate PHPUnit 7 before we've got a green run on PHP 7.4. See #3086374: Make Drupal 8 & 9 compatible with PHP 7.4 for more 7.4 goodness - atm we have 1 other issue but that's likely to increase once we can actually run tests on it (which this will allow us to do).

alexpott’s picture

+++ b/composer.json
@@ -59,7 +59,8 @@
+            "Drupal\\Core\\Composer\\": "core/lib/Drupal/Core/Composer",
+            "Drupal\\Tool\\": "core/tools"

This for me is still not what we want. The test code added here shouldn't be autoloadable by the regular run-time. We need to resolve this in another way.

alexpott’s picture

Here's another way we're we don't change the autoloader in the root composer.json (which actually would break composer built drupal projects!) and only affects the test autoloader but works well with test discovery.

Status: Needs review » Needs work

The last submitted patch, 136: 2950132-136.patch, failed testing. View results

alexpott’s picture

The composer update is not running on DrupalCI properly... also broke lots :)

Status: Needs review » Needs work

The last submitted patch, 138: 2950132-2-138.patch, failed testing. View results

mondrake’s picture

Re. #135 can we move that to the autoload-dev section in composer.json? I am not sure #138 will work when running tests via PHPUnit directly

Wim Leers’s picture

#134 🤦‍♂️😔 — nothing we can do about PHPUnit's choices of course…

alexpott’s picture

#138 works just fine when running PHPUnit's tests directly - core/tests/bootstrap.php is what makes the test system autoloadable for it already.

alexpott’s picture

@mondrake your concerns about test discovery are entirely valid but core/tests/Drupal/TestTools is placed outside test discovery - so running all the unit tests with something like $ ./vendor/bin/phpunit -v -c ./core --testsuite unit still works a treat with PHPUnit 7 or PHPUnit 6.

mondrake’s picture

@alexpott re. #142, #143: yep, confirmed, I tested the patch with direct PHPUnit calls by --group and by --testsuite, all good. Thanks and sorry for the noise.

alexpott’s picture

One place we need to update the autoloader for tests so the UI works.

mondrake’s picture

+++ b/core/scripts/run-tests.sh
@@ -483,7 +490,26 @@ function simpletest_script_init() {
+  // @todo https://www.drupal.org/project/drupal/issues/2942473 Remove when
+  //   dropping PHPUnit 4 and PHP 5 support.

I think this is no longer needed.

Other than that, +1 on RTBC, but will need some adjustments on the CR for #121.1, and #121.5. Obviously I cannot RTBC myself.

Re. #121.6 I think I misinterpreted the purpose of that test, and HEAD is correctly checking that the value of the content-hash as calculated from composer.json (expected) is the same as the actual one stored in composer.lock (actual). Since that was corrected in #124, all good here.

alexpott’s picture

Good call in #146. We're probably going to need to for PHP 8.0 too so removing the entire todo seems best. Once we remove the lock file this all gets a bit easier.

mondrake’s picture

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

Needs a reroll, alas

alexpott’s picture

Status: Needs work » Needs review
FileSize
55.73 KB

Re-rolled. Our lock file feels very unstable at the moment. Doing composer update --lock on HEAD should be a no-op but its not. See #3082866-26: composer complains "Skipped installation of bin bin/composer for package composer/composer: file not found in package".

Here's a re-roll.

alexpott’s picture

Rerolled. Also this is a critical task because it is blocking PHP 7.4 compatibility.

larowlan’s picture

Nasty, but necessary evils etc - some real black magic going on here - one question -

  1. +++ b/core/scripts/run-tests.sh
    @@ -483,7 +490,24 @@ function simpletest_script_init() {
    +  // Detect if we're in the top-level process using the private 'execute-test'
    +  // argument. Determine if being run on drupal.org's testing infrastructure
    +  // using the presence of 'drupalci' in the sqlite argument.
    +  // @todo https://www.drupal.org/project/drupalci_testbot/issues/2860941 Use
    +  //   better environment variable to detect DrupalCI.
    +  if (!$args['execute-test'] && preg_match('/drupalci/', $args['sqlite'])) {
    +    // Update PHPUnit if needed and possible. There is a later check once the
    +    // autoloader is in place to ensure we're on the correct version. We need to
    +    // do this before the autoloader is in place to ensure that it is correct.
    +    $composer = ($composer = rtrim('\\' === DIRECTORY_SEPARATOR ? preg_replace('/[\r\n].*/', '', `where.exe composer.phar`) : `which composer.phar`))
    +      ? $php . ' ' . escapeshellarg($composer)
    +      : 'composer';
    +    passthru("$composer run-script drupal-phpunit-upgrade-check");
    +  }
    +
       $autoloader = require_once __DIR__ . '/../../autoload.php';
    

    is this not something we can do in drupalci.yml?

  2. +++ b/core/tests/Drupal/KernelTests/AssertLegacyTrait.php
    @@ -78,7 +54,7 @@ protected function assertNotEqual($actual, $expected, $message = '') {
    +    $this->assertSame($expected, $actual, !empty($message) ? $message : '');
    
    @@ -88,7 +64,7 @@ protected function assertIdentical($actual, $expected, $message = '') {
    +    $this->assertNotSame($expected, $actual, !empty($message) ? $message : '');
    
    @@ -101,7 +77,7 @@ protected function assertIdenticalObject($actual, $expected, $message = '') {
    +    $this->assertEquals($expected, $actual, !empty($message) ? $message : '');
    

    we can null coalesce these now right?

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

alexpott’s picture

Re #152.1 - nope - we want contrib to be able to test on PHP 7.4 too.

I fixed #152.2 a bit differently - with a string cast - just in case someone decides to try to get Drupal 8.7.x to test on PHP 7.4.

alexpott’s picture

Version: 8.9.x-dev » 8.8.x-dev

This is targeting 8.8.x atm

greg.1.anderson’s picture

I just wanted to draw your collective attentions towards [Composer Test Scenarios](https://github.com/g1a/composer-test-scenarios). The idea is that you define new named test scenarios in your composer.json file. In this context, a "test scenario" is a collection of dependency versions that you want to test together. For example, this patch defines a new scenario consisting of upgrades to:

  • phpunit/phpunit
  • symfony/phpunit-bridge
  • phpspec/prophecy
  • symfony/yaml

If we named this scenario "phpunit7", then you would be able to switch to it by running composer scenario phpunit7. This can be done for ad-hoc testing, or as a step in drupalci (or perhaps automatically from a Composer hook, as this patch currently does to call composer udpate).

The advantage of defining a test scenario is that Composer Test Scenarios generates a separate composer.lock file for each scenario it manages. Without a lock file, e.g. as is the case with this PR, phpunit7 are always "highest" tests for the dependencies that are updated. Since we are only updating "require-dev" dependencies here, the use of a lock file is not as important as if we needed to vary the non-dev dependencies. That is usually only an issue for libraries; it seems unlikely that Drupal would ever want to define non-dev scenarios. I just thought I'd point this library out in case anyone was interested in the extra bit of organization.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

Hey, what happened here? I didn't refresh before posting, cross-posted with @alexpost, and it hid his two patch files? I don't even see them in the list of uploads to un-hide. They are still available from his comment, though.

Anyway, I was just waiting for his answer to @larowlan before RTBC'ing. This looks good. LMK if there's any interest in setting up test scenarios as a follow-on.

greg.1.anderson’s picture

To be clear, yes, I intended to RTBC this issue in #157.

alexpott’s picture

Chasing HEAD.

mondrake’s picture

alexpott’s picture

Issue summary: View changes

Updated release note.

alexpott’s picture

Here's a reroll because composer.json has changed. Note I've confirmed that the lock file is correct for 8.8.x, 8.9.x and 9.0.x

catch’s picture

There are some very ugly things in here, but they're not our fault and we have longer-term plans to clean them up.

This would be good to get into alpha to flesh out any issues with it earlier rather than later, we don't really have an option other than what's in the patch if we don't want to fall behind with phpunit/PHP itself.

alexpott’s picture

Updated issue credit and created @Gábor Hojtsy and @Pasqualle for issue management and @larowlan and @Mile23 for code review.

  • catch committed c9898ab on 9.0.x
    Issue #2950132 by mondrake, alexpott, tom_ek, Gábor Hojtsy, Pasqualle,...

  • catch committed 822fa07 on 8.9.x
    Issue #2950132 by mondrake, alexpott, tom_ek, Gábor Hojtsy, Pasqualle,...

  • catch committed 30e8f31 on 8.8.x
    Issue #2950132 by mondrake, alexpott, tom_ek, Gábor Hojtsy, Pasqualle,...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs framework manager review +8.8.0 release notes

Committed bcee1df and pushed to 9.0.x. Thanks!
(and cherry-picking back to 8.8.x)

Sam152’s picture

Hm, I upgraded to 8.8.x-dev and am running PHPUnit 6.5.14 and I get the following error when running tests:

Fatal error: Uncaught Error: Class 'Drupal\TestTools\PhpUnitCompatibility\RunnerVersion' not found in /data/app/core/tests/Drupal/Tests/Listeners/HtmlOutputPrinter.php on line 15

Edit: nevermind, our project had a custom autoloader and needed the following addition:

+ $loader->add('Drupal\\TestTools', __DIR__);

This comment might help someone who ran into the same error message.

pookmish’s picture

I'm getting the same error as @Sam152 with a fresh composer installation and using drupal-check. Nothing custom done and it produces the same error. I can reproduce the error if i checkout the commit that was made in #168. If i checkout any commit prior to that one, I don't receive the error.

This particular issue appears to stem from `mglaman/phpstan-drupal` library. I've opened a conversation about this specific scenario at https://github.com/mglaman/phpstan-drupal/issues/87

xjm’s picture

Status: Fixed » Needs work

Let's work #169 and #170 into the CR and release note, so that we can reduce the number of bug reports following the 8.8.0 ugprade. Thanks!

xjm’s picture

Issue summary: View changes
mradcliffe’s picture

#169 and #170 are committed in the primary branch of mglaman/phpstan-drupal, but not in a release version. Composer failed to resolve the dependencies when I tried to pull in phpstan-drupal's primary branch and then drupal-check because of that. In order to pull in the changes until a release is made I needed to run:

composer global remove mglaman/drupal-check
composer global require mglaman/phpstan-drupal:'dev-master as 0.11.11' mglaman/drupal-check:dev-master

Those instructions probably should not be used in the change record. It would be preferable to only need to run an update.

greg.1.anderson’s picture

Aside: It is best not to use composer global require for packages that are not Composer plugins, as it can cause unintended problems. See Fixing the Composer Global Command.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Fixed

Added #169 to the change record. It did not add this to the release note snippet because it is extremely technical and the PHPStan issue has been fixed already.

Status: Fixed » Closed (fixed)

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