Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50902 closed task (blessed) (fixed)

Build/CI: fix running of the unit tests on PHP 8/nightly

Reported by: jrf's profile jrf Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: major Version:
Component: Build/Test Tools Keywords: has-patch php8 commit has-unit-tests
Focuses: coding-standards Cc:

Description

PHP 8.0 will introduce a huge amount of changes and some of the more severe issues for WP are things which static analysis with tools like PHPCompatibility will not be able to detect as they involve the runtime value of variables, which static analysis tools do not have access to.

Among others this includes:

So, the pressure on the unit tests suite being expanded and passing on PHP 8 can be expected to increase over the next few months as testing, testing and testing will be the only way to catch a lot of these PHP 8 compatibility issues.

At this time, the build against PHP 8/nightly is failing, or rather, not even running.

See: https://travis-ci.com/github/WordPress/wordpress-develop/jobs/370784915

This ticket intends to fix that, so we can at least see what's failing.

Bear with me while I explain what's going on and why the attached path will fix this:

  1. PHP 8 introduces a new control structure called match, which makes match a reserved keyword in PHP 8.
  2. One of the dependencies of PHPUnit declares a class named Match, which triggers a fatal error before PHPUnit can even start to run.
  3. The dependency in PHPUnit has been updated to get rid of the Match classname and has released a new version.
  4. However, the Travis build/docker image uses the PHPUnit 7.5.20 PHAR, which, as PHPUnit 7.x is no longer supported, will not be updated anymore for the new version of the dependency.
  5. In other words, to be able to use PHPUnit 7.x on PHP 8 and run the tests, we need the new version of the dependency.

To solve this:

  1. The Travis build should disregard the PHPUnit PHAR on PHP 8/nightly and should do a composer install instead and run the unit tests using the vendor/bin/phpunit command.
  2. But... as WP has a committed composer.lock file, that still won't work until we also update the composer.lock file using composer update phpunit/phpunit --with-dependencies to pull the new version of the dependency in. The PHPUnit version itself does not change when running that command as the restraints in the composer.json have not been updated.
  3. And unless we use --ignore-platform-reqs when doing the install on PHP 8, the build would still fail as "officially" PHPUnit 7 does not install/should not be installed on PHP 8.

I've discussed the proposed change with @pento and together we figured this to be the best solution-direction for now.

This solution-direction builds on the presumption that support for PHP < 7.1 will be dropped in WP 5.6, so the test suite can be upgraded to use the void return type for the fixtures and support PHPUnit8 and 9 in the foreseeable future, with PHPUnit 9.3 - released last week - being the first version to officially support PHP 8.

With that in mind, the currently proposed fix is the simplest way to get things working again.

Attachments (2)

50902-travis-test-on-php8.patch (18.6 KB) - added by jrf 4 years ago.
50902-1-travis-test-on-php8-separate-script.patch (18.9 KB) - added by jrf 4 years ago.
This patch contains the previous changes + runs the tests runs on PHP 8 individually instead of chained.

Download all attachments as: .zip

Change History (22)

#1 @johnbillion
4 years ago

  • Milestone changed from Awaiting Review to 5.6
  • Type changed from defect (bug) to task (blessed)

This ticket was mentioned in PR #471 on WordPress/wordpress-develop by jrfnl.


4 years ago
#2

See the extensive description in the Trac ticket.

Trac ticket: https://core.trac.wordpress.org/ticket/50902

#3 @jrf
4 years ago

Note: as the various test runs are "chained", once this patch has been committed, the first test run will be enabled and fail. Because of that the other test runs don't run.

If so desired, I can change the setup to run each of the 7 test runs separately on PHP 8 to get proper insight in the failures of all test runs.

To be specific:

phpunit --verbose -c phpunit.xml.dist

=> Fails with: Tests: 10895, Assertions: 49400, Errors: 184, Failures: 387, Warnings: 3, Skipped: 100, Risky: 3.

The following test runs aren't executed because of the above failure.

phpunit --verbose -c phpunit.xml.dist --group ajax &&
phpunit --verbose -c tests/phpunit/multisite.xml &&
phpunit --verbose -c tests/phpunit/multisite.xml --group ms-files &&
phpunit --verbose -c phpunit.xml.dist --group external-http &&
phpunit --verbose -c phpunit.xml.dist --group restapi-jsclient &&
phpunit -v --group xdebug --exclude-group fakegroup

#4 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#5 @jorbin
4 years ago

  • Keywords commit added

I've reviewed https://core.trac.wordpress.org/attachment/ticket/50902/50902-travis-test-on-php8.patch and like this as an approach.

As far as running all the tests without chaining, I think that would be best to do as a separate ticket. I think it might be interesting to see if we can add it as a new part of the test matrix to see if the additional parallel runs speed up the entire process. I don't know if that will lead to too large of a test matrix though.

#6 @jrf
4 years ago

@jorbin Thanks for looking this over.

Regarding the chaining - I was actually thinking of doing that just in the script part of the .travis.yml, not in the matrix.

Maybe a combination would actually work best ?
What about having one extra build per PHP version & splitting the test runs across the two builds, for instance divided between single-site test runs and multi-site test runs ?

I agree moving the implementation of that to a separate ticket would be best.

As for PHP 8/nightly: I've had a look at some of the current test failures and there are some which we won't be able to solve in WP as they are related to code in PHPUnit dependencies, most notably the mocking implementation, so unless we have each test run in a separate condition within the Travis script part (possibly in the matrix), only the first test run will ever be executed (and even when the WP native issues have been solved, it will fail on things outside the scope of WP).

Fixing that part does belong with this ticket.

@jrf
4 years ago

This patch contains the previous changes + runs the tests runs on PHP 8 individually instead of chained.

#7 @jrf
4 years ago

@jorbin I've added a separate commit to the GH PR to show what I mean about running the tests without chaining on PHP 8.

The newly uploaded patch is the previous patch + the new commit squashed together.

This ticket was mentioned in PR #483 on WordPress/wordpress-develop by jrfnl.


4 years ago
#8

  • Keywords has-unit-tests added

The commits in this PR combined:

  • Bump the minimum supported PHP version for WordPress to PHP 7.1.26.

The "patch" part of the version number - 7.1.patch - is open for discussion, please see commit https://github.com/WordPress/wordpress-develop/commit/e913a6889a05cee5dab9a4eb2f3727029199076a for my argumentation for this specific version.

  • Applies the proposed patch for trac #50902 to allow the unit tests to run on PHP 8.0/nightly.
  • Bumps the supported PHPUnit versions to ^7.5 || ^8.0 || ^9.0.
  • Removes now redundant backfills for PHPUnit native functionality.
  • Adds a few polyfills for new PHPUnit functionality in PHPunit-version based traits.
  • Comprehensively applies any and all fixes needed to make the complete test suite compatible with PHPUnit 8.x and 9.x.

This PR will be easiest to review by looking at the individual commits and their detailed commit messages.

Once the patch from ticket 50902 and the patches from this PR are merged, we will be able to get proper insight into the problems we need to solve to make WordPress compatible with PHP 8.0 based on the existing tests.

Trac ticket 50913 already contains a couple of patches to make a start with PHP 8.0 compatibility.

Expanding the test suite, strict type checking in the test suite (Trac 38266) and code coverage tracking (Trac 46373) combined with adding @covers annotations (Trac 39265) to get better insight into which parts of the code base need more tests, is strongly recommended all the same.

Trac ticket: https://core.trac.wordpress.org/ticket/46149

#9 @jrf
4 years ago

Grr... ok, so that wasn't meant to happen - the above patch is for another ticket but the matcher has gotten confused. I will close that PR and open a new one to make sure the patch is linked to the correct ticket.

jrfnl commented on PR #483:


4 years ago
#10

Closing as the patch got linked to the wrong Trac ticket.

#11 @SergeyBiryukov
4 years ago

In 48957:

Build/Test Tools: Allow unit tests to run on PHP 8 in full.

PHP 8 introduces a new control structure called match, which makes match a reserved keyword in PHP 8.

One of the PHPUnit dependencies declares a class named Match, which triggered a fatal error before PHPUnit could even start.

To be able to use PHPUnit 7.x on PHP 8 and run the tests, core needs a new version of that dependency, which is now installed using Composer.

This is the simplest way to get things working again and start addressing the individual test failures.

Additionally, various test runs on PHP 8 on Travis are now performed individually instead of being chained, so that failures outside of WP scope don't block further execution.

Props jrf, jorbin, pento.
See #50902.

#12 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

This appears to work as expected.

See #50913 for addressing the individual test failures, and #46149 for using a newer version of PHPUnit.

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 years ago

#14 @SergeyBiryukov
4 years ago

In 48972:

Tests: Replace the native PHPUnit getMockForAbstractClass() and getMockBuilder() methods.

This avoids parse errors in PHPUnit internals due to match being a reserved keyword in PHP 8.

To run on PHP 8, the tests relying on these methods require PHPUnit 9.3 or later.

When the test suite is updated for compatibility with PHPUnit 9.x, these overrides can be removed.

See #50913, #50902.

#15 @SergeyBiryukov
4 years ago

In 49037:

Tests: Backport two changes from PHPUnit 9.3:

  • Replace the Match interface with ParametersMatch, to avoid parse errors due to match being a reserved keyword in PHP 8.
  • Replace ReflectionParameter::getClass() usage, which is deprecated in PHP 8.

This allows tests relying on the getMockForAbstractClass() and getMockBuilder() methods to run again on PHP 8.

When the test suite is updated for compatibility with PHPUnit 9.x, these overrides can be removed.

Follow-up to [48972].

See #50913, #50902.

#16 @SergeyBiryukov
4 years ago

In 49074:

Build/Test Tools: Comment out the xdebug group test run for PHP 8 for now.

Xdebug supports PHP 8 only from version 3.0, which is not released yet.

Once Xdebug 3.0 is released and included in the Docker image, this should be uncommented again.

Follow-up to [48957], [49037].

See #50913, #50902.

#17 @SergeyBiryukov
4 years ago

In 49077:

Build/Test Tools: Remove PHP 8 from allowed failures.

With all known unit test issues now addressed, WordPress 5.6 aims to support PHP 8 as much as possible.

See #50913, #50902.

jrfnl commented on PR #471:


4 years ago
#18

Closing as committed.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 years ago

#20 @konamiman
4 years ago

Hi, I'm From Proton team @ Automattic and I'm working on ensuring PHP 8 compatibility for WooCommerce. I'll explain a solution I've found to be able to run tests with PHPUnit 7 in PHP 8.

Since indeed PHPUnit 7 reached end of life and won't be fixed, I created a fork fixing the couple of things needed to make it PHP 8-ready. Here's the fork, with a pull request created for better readability: https://github.com/woocommerce/phpunit/pull/1

Then I've created a (draft at the moment) pull request that uses that fork (with the appropriate branch) instead of the official PHPUnit package: https://github.com/woocommerce/woocommerce/pull/27844

It's not clear if we'll actually end up using that solution, but at least it's helpful for having the test suite working in order to be able to check other stuff.

And that's it, a bit hacky but it works, as a temporary solution at least. I thought it would be appropriate to mention this here, in case it's useful.

Note: See TracTickets for help on using tickets.