#50902 closed task (blessed) (fixed)
Build/CI: fix running of the unit tests on PHP 8/nightly
Reported by: | jrf | Owned by: | 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:
- Numerous engine notices/warnings which are now catchable exceptions (= fatal error unless caught). See: https://wiki.php.net/rfc/engine_warnings
- Changes to how numbers are compared and how numeric strings are handled. See: https://wiki.php.net/rfc/string_to_number_comparison and https://wiki.php.net/rfc/saner-numeric-strings
- Errors when arithmetic operations are executed on non-numeric values. See: https://wiki.php.net/rfc/arithmetic_operator_type_checks
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:
- PHP 8 introduces a new control structure called
match
, which makesmatch
a reserved keyword in PHP 8. - One of the dependencies of PHPUnit declares a class named
Match
, which triggers a fatal error before PHPUnit can even start to run. - The dependency in PHPUnit has been updated to get rid of the
Match
classname and has released a new version. - 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.
- 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:
- 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 thevendor/bin/phpunit
command. - But... as WP has a committed
composer.lock
file, that still won't work until we also update thecomposer.lock
file usingcomposer 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 thecomposer.json
have not been updated. - 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)
Change History (22)
#1
@
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
#3
@
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
#5
@
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
@
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.
@
4 years ago
This patch contains the previous changes + runs the tests runs on PHP 8 individually instead of chained.
#7
@
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
trait
s. - 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
@
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.
This ticket was mentioned in Slack in #core by sergey. View the logs.
4 years ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
4 years ago
#20
@
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.
See the extensive description in the Trac ticket.
Trac ticket: https://core.trac.wordpress.org/ticket/50902