Make WordPress Core

Opened 5 years ago

Closed 2 years ago

#46149 closed task (blessed) (fixed)

PHPUnit 8.x support

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: jrf's profile jrf
Milestone: 5.9 Priority: high
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests early php8 has-dev-note
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Background: #43218

PHPUnit 8.0.0 is scheduled for February 2019. The changelog includes some backwards compatibility breaks.

From the PHPUnit 7 announcement (February 2, 2018):

Looking Forward

The following methods do not have a return value and should therefore have a void return type declaration:

  • PHPUnit\Framework\TestCase::setUpBeforeClass()
  • PHPUnit\Framework\TestCase::setUp()
  • PHPUnit\Framework\TestCase::assertPreConditions()
  • PHPUnit\Framework\TestCase::assertPostConditions()
  • PHPUnit\Framework\TestCase::tearDown()
  • PHPUnit\Framework\TestCase::tearDownAfterClass()
  • PHPUnit\Framework\TestCase::onNotSuccessfulTest()

These methods will have a void return type declaration in PHPUnit 8. Please declare your methods that overwrite the above mentioned methods void now so you are not affected by this backward compatibility break.

This is now implemented in PHPUnit 8, so our method signatures are no longer compatible and cause fatal errors:

Fatal error: Declaration of WP_UnitTestCase_Base::setUpBeforeClass() must be compatible with PHPUnit\Framework\TestCase::setUpBeforeClass(): void in tests/phpunit/includes/abstract-testcase.php on line 1110

We could look into using different signatures for different PHPUnit versions in the same way as in [44701]. However, given the large number of tests extending the ::setUp() and ::tearDown() methods, it's probably easier to address this when we bump the required PHP version to 7.1.

Attachments (3)

46149.diff (1.3 KB) - added by thomasplevy 3 years ago.
46149-hard-deprecate-checkRequirements.patch (3.1 KB) - added by jrf 3 years ago.
Build/Test Tools: hard deprecate WP_UnitTestCase_Base::checkRequirements()
46149-test-bootstrap-improve-composer-command.patch (3.2 KB) - added by jrf 2 years ago.
Test bootstrap: refine the Composer command mentioned in error messages

Download all attachments as: .zip

Change History (144)

#1 @desrosj
5 years ago

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

#2 @SergeyBiryukov
5 years ago

We could also look into displaying a friendly message instead of a fatal error. Something like:

Looks like you're using PHPUnit 8.x, WordPress currently is only compatible with PHPUnit 7.x. Please use the latest PHPUnit version from the 7.x branch.

#3 @SergeyBiryukov
5 years ago

In 44723:

Build/Test Tools: Display a message about currently supported PHPUnit branch to avoid fatal errors on later versions.

See #46149.

#4 @desrosj
5 years ago

PHPUnit 8.0 and 8.0.1 have officially been released.

#5 @desrosj
5 years ago

Noticed today that the PHP nightly build is now failing from the changes in #43218. This should be fixed when the needed changes are made for this ticket.

https://travis-ci.org/WordPress/wordpress-develop/jobs/490350822

#6 @pento
5 years ago

Nightly switched from PHP 7.4 to PHP 8.0. #46235 is for adding PHP 7.4 back to the test matrix.

#7 @conner_bw
5 years ago

Please consider upgrading. I ran into this issue today trying to setup pcov:

https://github.com/krakjoe/pcov

Requires phpunit v8. Improves code coverage performance significantly.

#8 @ZanderZ
4 years ago

In February, PHPUnit 7 support will end and only PHPUnit 8 and 9 (to be released on February 7) will be supported.

The tickets says "it's probably easier to address this when we bump the required PHP version to 7."
Is there any news related to that?

#9 @davidshq
4 years ago

Also interested to hear a status update on this.

#10 @johnbillion
4 years ago

  • Keywords needs-patch added

There isn't one. If someone wants to work on this, that would be great.

#11 follow-ups: @SergeyBiryukov
4 years ago

Note that this still depends on bumping the required PHP version to 7.

As of this time, the decision has not been made yet.

#12 @netweb
4 years ago

  • Owner set to netweb
  • Status changed from new to assigned

I've partially implemented this for a client, will work on bringing those changes here

#13 in reply to: ↑ 11 @ZanderZ
4 years ago

Replying to SergeyBiryukov:

Note that this still depends on bumping the required PHP version to 7.

As of this time, the decision has not been made yet.

There has been 0 news on this front since the initial announcement. That announcement said "Depending upon feedback and effectiveness, we can follow up by bumping the required PHP version to 7 as early as December 2019." but that timeframe has clearly passed.

Can the WP team please share some news on this?

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


4 years ago

#15 in reply to: ↑ 11 @SergeyBiryukov
4 years ago

  • Description modified (diff)

Replying to SergeyBiryukov:

Note that this still depends on bumping the required PHP version to 7.

Just to clarify, bumping the required version to PHP 7.0 is apparently not enough, as the void return type declarations only appeared in PHP 7.1. So to support PHPUnit 8.x, we'd need PHP 7.1 as the minimum required version.

Updating the ticket description accordingly.

#16 follow-up: @jrf
4 years ago

As per #50902 - we will never be able to get proper insight into the issues which need fixing on PHP 8 without making the testsuite compatible with PHPUnit 8 + 9.

PHPUnit 9.3.x was released a week ago and is the first PHPUnit version which is officially claiming support for PHP 8.0.

As per the WP 5.6 release planning support for PHP 5.6 will be dropped in WP 5.6.

Is there an official decision yet that PHP 7.1 (or higher) will be the new minimum ? If so, I'd willing to create an initial patch to fix the test suite.

#17 in reply to: ↑ 16 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 5.6

Replying to jrf:

Is there an official decision yet that PHP 7.1 (or higher) will be the new minimum ? If so, I'd willing to create an initial patch to fix the test suite.

No official decision yet, but I'm pretty sure that is the goal that was agreed upon in the May 11th Site Health meeting (Slack logs).

Since PHP 8 support is also one of the WP 5.6 focuses, and PHPUnit 8+ is a prerequisite for that, let's proceed with this.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

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


4 years ago
#18

  • Keywords has-patch has-unit-tests added; needs-patch removed

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

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.

jrfnl commented on PR #483:


4 years ago
#19

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

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


4 years ago
#20

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

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.

#21 @jrf
4 years ago

Please see the GitHub PR for a comprehensive set of patches to address this ticket.

jrfnl commented on PR #484:


4 years ago
#22

@ntwb Thanks for the review! I've left replies to all remarks.

#23 @jrf
4 years ago

Before I forget to mention it: the "WP version bump" commit does not have to be included in the commits for this ticket, however, as the patches for this ticket require to stop testing against PHP < 7.1, it seemed like a logical commit to include.


After this ticket has been addressed, I would like to suggest follow-up tickets to address the following additional points in the test suite:

  • Review any @requires and version_compare()'s et al in the test suite to see if any of these can be removed now the requirements have changed.
  • Review any disabled tests (like commented out tests) to see if they were disabled due to version requirements not being met.
  • Review if the includes/phpunit6/compat.php file can be removed completely.

Similarly, after the version bump patch has been committed, follow-up tickets should be opened for the following additional points:

  • Removing the random_compat library. This library is no longer needed when the minimum PHP requirement is PHP 7.1. (Similar to #47699 for the PHP < 5.6 drop)
  • Go through the compat.php file to see if there are other PHP polyfills which can be removed. (Similar to #47698 for the PHP < 5.6 drop)
  • Go through the code base to review any code using version_compare(), function_exists(), defined(), extension_loaded() et al to verify whether these kind of toggles are still needed and remove them whenever possible. (Similar to #48074 for the PHP < 5.6 drop)
  • Updating external PHP dependencies if an update was so far blocked by a minimum PHP version mismatch.
Last edited 4 years ago by jrf (previous) (diff)

ntwb commented on PR #484:


4 years ago
#24

Thanks @jrfnl , a few more thoughts:

### PHP 5.6 support:

  • https://wordpress.org/about/stats/
  • PHP 5.6 = 15.1%
  • This pretty much rules out PHP 5.6 being dropped in WP 5.6, that usage is far too high to consider dropping it, the considerations historically have been to consider if usage drops below 5%

### PHPUnit test matrix

  • PHPUnit 8.x supports PHP 7.2, 7.3
  • PHPUnit 9.x supports PHP 7.3, 7.4 & 8.0

Would it be worth setting up or changing the existing jobs to use newer versions of PHPUnit?

  • PHP 7.2 using PHPUnit 8.x
  • PHP 7.3 and/or 7.4 using PHPUnit 9.x

ntwb commented on PR #484:


4 years ago
#25

Taking a read of the #site-health chat logs

The numbers for PHP 5.6 are similar to those we had for < 5.6 when we last bumped the version for WordPress 5.2, so it seems like we could bump the minimum to PHP 7.0 by the end of year as is. We do want to bump to PHP 7.1, though (as that would unblock using PHPUnit 8+ in core and testing WordPress on PHP 8), and we need additional efforts for that.

Given that we're still releasing security updates for WP 3.7 (released almost 7 years ago), it's not like we're leaving PHP 5.6 or 7.0 users without security updates, they just won't have some latest and greatest features of WP 5.6+, which seems fair.

A good first step might be to highlight PHP 7.0 as no longer "acceptable" in Browse Happy dashboard widget, same as we currently do for 5.6.

That's some reassuring news

Just noted the RECOMMENDED_PHP value in servehappy-config.php is still at 7.3. It was bumped 18 months ago in https://meta.trac.wordpress.org/changeset/7990.

Per the comment, it's supposed to be "The latest branch of PHP which WordPress.org recommends".

Given that 7.3 support ends in 5 months, should this be updated to 7.4?

Some things to note:

Ok, that's also some good steps


I'm somewhat more optimistic now having read the entire site #site-health Slack channel history since May 11 🤞🏼

#26 @jrf
4 years ago

Hmm.. something weird going on with the GH bot as my response which should be between those last two comments did not get cross-posted.

Quoting it here now for completeness:

This pretty much rules out PHP 5.6 being dropped in WP 5.6, that usage is far too high to consider dropping it, the considerations historically have been to consider if usage drops below 5%

@ntwb Dropping PHP 5.6 support is on the roadmap for WP 5.6, so IMO this is a foregone conclusion.

I did ask for confirmation before working on this patch and wouldn't have bothered spending this much time on getting it set up if that intention hadn't been clarified.

Would it be worth setting up or changing the existing jobs to use newer versions of PHPUnit?

No, that is not an option because of the void return type introduced for all fixtures in PHPUnit 8.
Return types were only introduced in PHP 7.0, the void return type in PHP 7.1, so it is not easy to make the test suite cross-version compatible with both PHPUnit 7 as well as PHPUnit 8 if the test suite also still has to be compatible with PHP 5.6/7.0.

There is a way to do it, but I would strongly recommend against it if it can be avoided in any conceivable way. IMO dropping support for PHP 5.6 and 7.0 is the much better choice.

Last edited 4 years ago by jrf (previous) (diff)

#27 @jrf
4 years ago

  • Keywords early added
  • Priority changed from normal to high

Ticket #51043 has been created to handle the version bump specifically.

#28 follow-up: @dd32
4 years ago

FWIW after looking through this and doing some non-trivial testing, I think it'd be best to get PHP8 unit tests running ASAP, with the assumption that PHP 5.6 support is going to stick around for a bit longer, probably until at least until the end of the year - and even if it doesn't, getting unit tests running on a not-too-far-away PHP8 is more important than that decision.

That means that migrating to using :void doesn't appear to be possible quite yet.

After talking through things with an unnamed committer, I suspect the best option might be to shim the functions in question.

  • Move from setUp() to something prefixed like _setUp() in all the actual Unit Tests (plus all the other newly void functions)
  • Conditionally load a WP_UnitTestCase class which defines setUp() as either non-void for older PHPUnits+PHPs or with Void for newer PHPUnits, which calls said _setUp() method instead.
  • Use Traits (PHP 5.4+) as per PR 484 to polyfill newer PHPUnit functionality
  • Migrate all the tests to the newer test syntax, as with PR 484.

That might mean that we'd ultimately end up with a phpunit-[5-9]/testcase.php file set, but that seems like a more reasonable framework to ensure that going forward we'll have a framework in place to support future PHPUnit/PHP syntax changes.

Related to that.. I've PR'd the wpdev images to add a PHPUnit9 image and use it for PHP 8 Since it's the only version that is supported there.

Last edited 4 years ago by dd32 (previous) (diff)

#29 @jorbin
4 years ago

I think the approach @dd32 is suggesting is going to be the best route for us. Supporting multiple versions of PHPunit is an unfortunate reality, but one that will help us not just now but also possibly farther into the future. While the ultimate goal of WordPress supporting only the versions of PHP that the PHP core team supports (which is the policy in place for PHPUnit) is admirable, until hosts are updating PHP versions with more regularity, it isn't something that I see becoming a reality any time soon.

#30 @ayeshrajans
4 years ago

I doubt Symfony's PHPUnit bridge will be able to make WordPress's test suite compatible across 5 PHPUnit versions, having a single test suite that works with multiple PHPUnit versions is one if its goals: https://symfony.com/doc/current/components/phpunit_bridge.html

#31 @pputzer
4 years ago

While problably not "production-ready" for Core, you can take https://github.com/mundschenk-at/phpunit-cross-version as a proof of concept for cross-version PHPUnit tests based on the Symfony phpunit-bridge. Caveat: Newer PHPUnit versions require their particular file naming conventions for individual test classes for separate execution (complete test suite works fine even when you use WordPress conventions).

Last edited 4 years ago by pputzer (previous) (diff)

#32 follow-ups: @jrf
4 years ago

I understand the direction you all want to take, but cannot in good conscience support it for the following reasons:

  1. It really isn't as straight forward as outlined in the comments above and I say so based on both my experience with other test code bases as well as on the work I did for this ticket.
  2. It will raise the barrier for entry to new contributors by a factor five as they will need to learn new (undocumented) names for fixtures, assertions etc The PHPUnit knowledge they already have becomes next to useless, the PHPUnit manual can no longer be used as reference etc.
  3. It will alienate existing contributors to the tests.
  4. I truly believe our time would be better spend on making WP compatible with newer PHP versions, than on a huge undertaking of making the unit test suite compatible with PHPUnit 5 - 9.

So good luck to you. I wish you well.

#33 in reply to: ↑ 32 @dd32
4 years ago

Replying to jrf:

I understand the direction you all want to take, but cannot in good conscience support it for the following reasons:

I totally understand your position here, and I want to state that your input is incredibly valuable and the work you've done so far in adding support is tremendous.

  1. It really isn't as straight forward as outlined in the comments above and I say so based on both my experience with other test code bases as well as on the work I did for this ticket.

I totally agree, this isn't super straight forward (Implementation vs Trac comments never match 1:1!), and will be messy, but it's the unfortunate reality when building with tools that have a mismatch in PHP distributions and user focus. If we only had to support one version of PHP, one version of PHPUnit, and could enforce that our lives would be so much easier, but WordPress would mostly be a developer-only tool at that point.

WordPress has been relatively lucky in that up until now, there hasn't been any significant changes like this language syntax change that cause such problems.

  1. It will raise the barrier for entry to new contributors by a factor five as they will need to learn new (undocumented) names for fixtures, assertions etc The PHPUnit knowledge they already have becomes next to useless, the PHPUnit manual can no longer be used as reference etc.

They will still be able to use their own knowledge, it's just that come PR time / patch time, whomever merges/commits would just need to check that the correct functions are in use.

Developers writing WordPress PHPUnit tests will still need to figure out all of our unit test factories, custom assertion methods, etc, this is far from the only change/knowledge they'd need to get up to speed on.

A unit test could even scan the tests looking for non-prefixed cases to make it easier for all.

  1. It will alienate existing contributors to the tests.

This has always been a struggle with WordPress, be it PHP 5.2 vs 5.3 OOP, PHP 5.2 vs 7.0, and now PHP 5.6 vs 7.1. If done correctly, I would hope that the narrow functionality of PHPUnit that we do use would just work and most contributors wouldn't notice a thing.

  1. I truly believe our time would be better spend on making WP compatible with newer PHP versions, than on a huge undertaking of making the unit test suite compatible with PHPUnit 5 - 9.

I do believe that too - WordPress should be, and has to be, compatible with newer PHP versions, but that shouldn't have to mean dropping support for older versions of PHP.


There's an alternative option we have here too, we could standardise on PHP8 + PHPUnit9 for *all* tests and rely upon a PHP transpile process to run it on older PHP versions / PHPUnit versions. I attempted that with PHP7.4/PHPUnit7 as a base and a transpile (let's be honest, regex..) to PHP8 which would work for travis unit testing, but falls over because we can't force everyone to use PHP7.4 for unit test development, hopefully PHP8 would be common soon.

#35 in reply to: ↑ 28 @dd32
4 years ago

Replying to dd32:

I suspect the best option might be to shim the functions in question.

I went ahead and put something together that does just that, it's not super pretty, but it does work on PHP 5.6 -> 8.0 with PHPUnit 5 -> 9.

Most importantly, it introduces a way to shim functionality from newer PHPUnits for older PHPUnit versions, so the tests can immediately use the new functionality from PHPUnit 9 - it just has to include a compat method for the older versions.

Unfortunately it did require ceasing using the setUp() (and friends) methods directly, as there's simply no way to have those functions defined AND work on all current supported PHPs.
For now, I've simply prefixed those methods with an underscore, we do have custom wpSetUpBeforeClass() and wpTearDownAfterClass() methods though, we could probably do the same for the setUp()/etc.

I think this will affect plugins in some way or another, but if they're affected they'd have had to change their testing to work with PHP8 anyway.

My primary goal here was to get unit testing for PHP 5.6-8.0 running for core, so that there was a possibility to actually see the PHP 8 Unit test status..

To reduce the diff size for review, all of the code changes to the tests are done through a bash script, mostly based on @jrf's PR, only the changes to the test runner framework is in the PR as a code change.

If you want to see the complete changes to the tests, you can run the ./tools/migrate-test-syntax.sh script locally or look around line 200 of the Travis tests for a 16,000 line diff.

Here's the PR and hopefully, all the Travis tests will pass.. except for PHP8.. which well you'll see :)
edit: Seems the PR Travis tests are all failing on the PR, but passing on the fork branch..

Last edited 4 years ago by dd32 (previous) (diff)

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


4 years ago

#38 in reply to: ↑ 32 ; follow-up: @dd32
4 years ago

Replying to jrf:

  1. It really isn't as straight forward as outlined in the comments above and I say so based on both my experience with other test code bases as well as on the work I did for this ticket.

@jrf I'd like to bow down to you and agree with you. I apologise for doubting you. It's not realistic to follow what I outlined above. It works, but it's not worth it, and it's a support burden that can't be carried forward.

I've taken my above PR and forked it into a new PR 491 which does two things, which I think is a reasonable middleground

  • Standardises on PHPUnit8/9 assert calls, requires PHP 7.2+ to run. (edit: 7.1+ but lets say 7.2 for future compat)
  • Includes a bash script for use on Travis to remove the void return type hinting so that Travis can still run the Unit tests for PHP 5.6, 7.0 , and 7.1 (edit: Not needed for 7.1 yet)

There are two bash scripts included, one of which will be required to be bundled for Travis:

  • tools/migrate-test-syntax.sh Responsible for doing some automated changes to the codebase that's needed for PHPUnit8/9 support - @jrf did a better job than this script in PR484 but this let me test that PHP8 support was working without making the PR diff huge.
  • tools/tests-old-php.sh It's responsible for removing the PHP 7.2+ syntax (Right now, just the void return type hinting) so that the tests will run on PHP 5.6-7.1 with PHPUnit 5-6. This one is going to be needed on Travis, It could be done just be done inline in .travis.yml but I'm assuming that we might end up adding other things when PHPUnit 10 comes out next year, so it seems worth while including it separately, which also allows developers to run it locally if needed.

The PHPUnit Compat traits that I've got in there are super minimal and just pass it through to other assertion methods that should exist. I did it slightly different to PR484 which makes it far easier and simpler as we don't have to use matching function definitions, nor check if the native function exists. PR484 has better PHPDocs though.

There's a bunch of other cleanups that can be done, but I didn't look into, such as using the PHPUnit namespaces and the SpeedTrap changes.

all of the unit tests run with only PHP8 failing due to some Core changes needed and some test alterations that I couldn't easily automate.

Last edited 4 years ago by dd32 (previous) (diff)

dd32 commented on PR #489:


4 years ago
#39

The changes proposed in this PR are not viable, this PR was forked into #491

#40 in reply to: ↑ 38 @SergeyBiryukov
4 years ago

Replying to dd32:

I've taken my above PR and forked it into a new PR 491 which does two things, which I think is a reasonable middleground

  • Standardises on PHPUnit8/9 assert calls, requires PHP 7.2+ to run.
  • Includes a bash script for use on Travis to remove the void return type hinting so that Travis can still run the Unit tests for PHP 5.6, 7.0, and 7.1.

Just noting that the void return type hinting was introduced in PHP 7.1, so unless I'm missing something, PHP 7.1 should be able to run the newer syntax without any modifications, I think they are only needed for 5.6 and 7.0. Is that assumption wrong?

#41 follow-up: @jrf
4 years ago

Just noting that the void return type hinting was introduced in PHP 7.1, so unless I'm missing something, PHP 7.1 should be able to run the newer syntax without any modifications, I think they are only needed for 5.6 and 7.0. Is that assumption wrong?

Yes and no. Void is not the problem for PHP 7.1. The problem there is that PHPUnit 7 is still needed - PHPUnit 8 has a minimum requirement of PHP 7.2 - and if per @dd32 comment above, the test suite will be standardized on PHPUnit8/9 assert calls (which I support as the sensible thing to do), then some of those assertions will either need wrappers, like the traits I created in the original PR, or the proposed bash script will need to run to "rewrite" some of those assertions to their old name/ to the old way of doing that assertion.

#42 in reply to: ↑ 41 @SergeyBiryukov
4 years ago

Replying to jrf:

Yes and no. Void is not the problem for PHP 7.1. The problem there is that PHPUnit 7 is still needed

Ah, that makes sense. Thanks for the clarification!

#43 @dd32
4 years ago

I did indeed mix PHP 7.1/7.2 and PHPUnit 8/9 requirements up.
The bash script isn't currently needed for PHP 7.1 as the void return type hints work there.

However, as @jrf suggested, if we're going to take this route (Which in all honesty, is the only way forward I can see) it makes zero sense to recommend PHPUnit 7 / PHP 7.1 be natively supported by the tests - It may still just work, but we shouldn't advocate it's use, and we have to assume that it won't always run without run-time modifications in the future.

#44 @hellofromTonya
4 years ago

  • Keywords php8 added

Adding the php8 keyword to group it with the PHP8 tickets.

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


4 years ago

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


4 years ago

#47 @desrosj
4 years ago

  • Keywords needs-dev-note added

Adding needs-dev-note. Detailing things to be aware of related to unit testing and PHP 8 should be outlined.

dd32 commented on PR #491:


4 years ago
#48

This is no longer needed, as the work has been done by others.

#49 @SergeyBiryukov
3 years ago

  • Milestone changed from 5.6 to Future Release

PHPUnit 8.x/9.x support would be great to have, but is complicated if we also have to support PHP 5.6 and 7.0 at the same time.

Currently, we have the tests running on PHP 8 with PHPUnit 7.5.x, as result of the changes in #50902, specifically [48957] and [49037]. It's not pretty, but does the job for now. So I think the best way forward at the moment would be to continue with PHPUnit 7.5.x, as that already works.

Since core no longer requires PHPUnit 9.x support for PHP 8 testing, I'm moving this to a Future Release to be explored separately. Once the minimum required PHP version is bumped to 7.1 or later, this should be easier to address.

#50 follow-up: @jrf
3 years ago

The first alpha of PHP 8.1 has been released yesterday and brings with it new challenges for getting the tests running on PHP 8.1, so this ticket needs to be revived again.

I know I'm biased, but I would strongly recommend using the PHPUnit Polyfills to solve the majority of issues.

The Polyfills allow for using the expectations and assertions as available in PHPUnit 9.x and to still run the tests on PHPUnit 4.8 - 9.x.
It also includes a TestCase solution solving the void problem.

Additionally, I'd like to draw attention to the fact that PHPUnit 9.1.0 deprecated support for test file names which do not match the class name of the test class within.

Ref: https://github.com/sebastianbergmann/phpunit/pull/4109/

This means that the test file naming conventions will also need revisiting when addressing this ticket.

Loosely related to #53010 and https://github.com/WordPress/WordPress-Coding-Standards/issues/1995.

This ticket was mentioned in Slack in #core-test by mkaz. View the logs.


3 years ago

#52 @SergeyBiryukov
3 years ago

In 51331:

Build/Test Tools: Replace assertInternalType() usage in unit tests.

The assertInternalType() and assertNotInternalType() methods are deprecated in PHPUnit 8 and removed in PHPUnit 9.

While WordPress test suite currently only supports PHPUnit up to 7.5.x, this allows us to switch to newer assertions ahead of adding full support for PHPUnit 8+.

These methods introduced in PHPUnit 7.5 should be used as an alternative:

  • assertIsArray()
  • assertIsBool()
  • assertIsFloat()
  • assertIsInt()
  • assertIsNumeric()
  • assertIsObject()
  • assertIsResource()
  • assertIsString()
  • assertIsScalar()
  • assertIsCallable()
  • assertIsIterable()
  • assertIsNotArray()
  • assertIsNotBool()
  • assertIsNotFloat()
  • assertIsNotInt()
  • assertIsNotNumeric()
  • assertIsNotObject()
  • assertIsNotResource()
  • assertIsNotString()
  • assertIsNotScalar()
  • assertIsNotCallable()
  • assertIsNotIterable()

As WordPress currently uses PHPUnit 5.7.x to run tests on PHP 5.6, polyfills for these methods are now added to the WP_UnitTestCase class for PHPUnit < 7.5.

Props pbearne, jrf, dd32, SergeyBiryukov.
Fixes #53491. See #46149.

#53 in reply to: ↑ 50 @SergeyBiryukov
3 years ago

Replying to jrf:

I know I'm biased, but I would strongly recommend using the PHPUnit Polyfills to solve the majority of issues.

The Polyfills allow for using the expectations and assertions as available in PHPUnit 9.x and to still run the tests on PHPUnit 4.8 - 9.x.
It also includes a TestCase solution solving the void problem.

Just voicing my support for this as it seems to be the most sensible approach.

Per discussion with Juliette, I think we can take things one step at a time for clarity and modernize the assertions first, then tackle the void return type issue separately.

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


3 years ago

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


3 years ago

This ticket was mentioned in Slack in #hosting-community by jadonn. View the logs.


3 years ago

#57 @SergeyBiryukov
3 years ago

In 51462:

Tests: Replace assertContains() with assertStringContainsString() when used with strings.

Using the assertContains() and assertNotContains() methods with string haystacks was deprecated in PHPUnit 8 and removed in PHPUnit 9.

While WordPress test suite currently only supports PHPUnit up to 7.5.x, this allows us to switch to newer assertions ahead of adding full support for PHPUnit 8+.

These methods introduced in PHPUnit 7.5 should be used as an alternative:

  • assertStringContainsString()
  • assertStringContainsStringIgnoringCase
  • assertStringNotContainsString()
  • assertStringNotContainsStringIgnoringCase

As WordPress currently uses PHPUnit 5.7.x to run tests on PHP 5.6, polyfills for these methods were added to the WP_UnitTestCase class for PHPUnit < 7.5.

Follow-up to [51331], [51451], [51461].

Props jrf, dd32, SergeyBiryukov.
See #53363, #46149.

#58 @johnbillion
3 years ago

  • Milestone changed from Future Release to 5.9

#59 @hellofromTonya
3 years ago

During today's working session with @johnbillion @SergeyBiryukov @jrf @netweb and me (see the live stream here), we discussed and reached consensus on adopting the practice of building tests to the latest PHPUnit version:

  • Adopt PHPUnit Polyfills to shim backwards for cross-version compatibility
  • Add an adapter layer between the tooling and Core's test cases for future flexibility
  • Change the inheritance order
  • Remove the copied PHPUnit files and PHPUnit assertions
  • Adopt Polyfills snakecase fixture methods (as PHPUnit 8 method signatures changed) -> this is a breaking change for extenders who are using fixture methods in their tests
  • Help extenders with this breaking change (ticket #53904)
  • Get the atomic commits (listed here) committed (in order) ASAP
  • Implement a very very early communication plan for outreach to the extender community including publishing a Dev Note on Make Core (i.e. to give the extender community awareness and guidance on how to prepare their test suites)

This plan will add PHPUnit 8 and 9 support to Core. Plus, it sets up the test suites to adopt future PHPUnit versions.

Last edited 3 years ago by hellofromTonya (previous) (diff)

#60 @jrf
3 years ago

To add to @hellofromTonya's comment, the commit workflow for these changes will be as follows:

  1. Commit the patch for ticket #47381, which is a prerequisite. The patch has been pulled to GitHub, been reviewed by various committers and is now ready for commit.
  2. Once that patch has been committed a new PR on GitHub will be opened against this ticket with the first set of commits referenced by @hellofromTonya above. This first set of commits will introduce the PHPUnit Polyfills package and sort out all assertion and expectation method changes.
  3. Once that patch has also been committed a next PR on GitHub will be opened against this ticket with the second set of commits referenced above. This second set of commits will address the voidreturn type and make the test suite cross-version compatible with PHPUnit 5.7.x up to PHPUnit 9.5.x (current) and will enable running of the tests against PHP 8.1 in CI.

#61 @johnbillion
3 years ago

Couple extra points not mentioned so far:

  1. SpeedTrapListener will be removed, see 6:ticket:38237 for details.
  2. Some degree of backporting will happen in order to allow a plugin or theme test suite to use the snakecase methods in all test classes so they run on older WP versions too. Ticket(s) to be opened in due course.

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


3 years ago
#62

Note: this patch/these commits were reviewed and discussed extensively during the livestreamed mobprogramming session of July 30 with @johnbillion @SergeyBiryukov @hellofromtonya and me.
The livestream can be watched back as video on demand.

### Composer: add dependency on the PHPUnit Polyfills package

The PHPUnit Polyfills package is an add-on for PHPUnit, which works around common issues for writing PHPUnit cross-version compatible tests.

Features:

  • It offers a full set of polyfills for assertions and expectations introduced in PHPUnit since PHPUnit 4.8.
  • It offers two generic TestCases which include these polyfills, but also solve the void return type issue for the fixtures methods.
  • It offers a PHPUnit cross-version solution for the changes to the PHPUnit TestListener implementation.
  • Supports PHPUnit 4.8 - current.
  • Supports and is compatible with PHP 5.4 - current.

The package has no outside dependencies, other than PHPUnit, is actively maintained and endorsed by the maintainer of PHPUnit itself (the only package of its kind which has ever been endorsed).

### Build/Tests: unify the PHPUnit adapter TestCases

This commit:

  • Removes the PHPUnit 7 specific TestCase.
  • Removes all existing polyfills from the PHPUnit 5.x TestCase.
  • Imports all polyfill traits from the PHPUnit Polyfills package into the WP_UnitTestCase class and updates the docblock to reflect the actual function of the class.

Note: the list of polyfills needs to be verified and updated after each new release of the PHPUnit Polyfills package.
Alternatively (recommended), one of the build-in TestCase classes from the PHPUnit Polyfills package can be used instead.

  • Moves the require for the WP abstract-testcase.php to the bootstrap.php file.
  • Adds a require_once for the PHPUnit Polyfills autoloader to the bootstrap.php file.

Note: while this isn't _strictly_ necessary when the tests are run via Composer, having the include in the bootstrap allows for the tests to also be run via a PHPUnit Phar, providing contributors with more flexibility.

### Build/Tests: change the inheritance order of the abstract test classes

As things were, the inheritance order of the abstract test classes was as follows:
WP_UnitTestCase (PHPUnit adapter layer)

extends WP_UnitTestCase_Base (base test class)

extends PHPUnit\Framework\TestCase (PHPUnit native class.

Concrete (child) test classes, as well as more specific abstract TestCases are/were expected to extend the WP_UnitTestCase.

This order is not optimal as it means that the WP_UnitTestCase_Base class would not be able to benefit from any polyfills and/or shims in the PHPUnit adapter layer.

With that in mind, this commit changes the inheritance to:
WP_UnitTestCase (empty class, left in place to not break BC for plugin/theme integration tests)

extends WP_UnitTestCase_Base (base test class)

extends PHPUnit_Adapter_TestCase (PHPUnit adapter layer)

extends PHPUnit\Framework\TestCase (PHPUnit native class.

The new order allow for the WP_UnitTestCase_Base to also benefit from the PHPUnit adapter layer.

For BC reasons the WP_UnitTestCase, which all test classes are (were) expected to extend is left in place, though it is now an empty class and explicitly abstract.

### WP_UnitTestCase_Base::setExpectedException(): simplify redundant PHPUnit shim

PHPUnit 6 deprecated the setExpectedException() method in favour of the expectException(), expectExceptionMessage() and expectExceptionCode() methods.

The WP_UnitTestCase_Base::setExpectedException() backfilled the old method.
As the PHPUnit Polyfills polyfill the _new_ method, this backfill can now be simplified.

This backfill _should_ be removed in a future iteration, but is, for now, left in place so as not to break BC for plugin/theme test suites which extend the WP native test suite for their integration tests.

Follow up on [48996], [48997].
See #51344

### Tests: replace expectException() for PHP native errors

... with calls to the dedicated PHPUnit 8.4.0+ methods.

The old manner of testing these is soft deprecated as of PHPUnit 8.4, hard deprecated as of PHPUnit 9.0 and will be removed in PHPUnit 10.0.0.

The dedicated expectDeprecation(), expectDeprecationMessage(), expectDeprecationMessageMatches(), expectNotice(), expectNoticeMessage(), expectNoticeMessageMatches(), expectWarning(), expectWarningMessage(), expectWarningMessageMatches(), expectError(), expectErrorMessage(), expectErrorMessageMatches() methods were introduced as replacement in PHPUnit 8.4 and should be used instead.

These new PHPUnit methods are all polyfilled by the PHPUnit Polyfills and switching to these will future proof the tests some more.

Ref:

### Tests: replace assertFileNotExists() with assertFileDoesNotExist()

The assertFileNotExists() method was hard deprecated in PHPUnit 9.1 and the functionality will be removed in PHPUnit 10.0.0.

The assertFileDoesNotExist() method was introduced as a replacement in PHPUnit 9.1.

This new PHPUnit method is polyfilled by the PHPUnit Polyfills and switching to it will future proof the tests some more.

Ref:

### Tests: replace assertRegExp() with assertMatchesRegularExpression()

The assertRegExp() and assertNotRegExp() methods were hard deprecated in PHPUnit 9.1 and the functionality will be removed in PHPUnit 10.0.0.

The assertMatchesRegularExpression() and assertDoesNotMatchRegularExpression() methods were introduced as a replacement in PHPUnit 9.1.

This new PHPUnit method is polyfilled by the PHPUnit Polyfills and switching to it will future proof the tests some more.

Ref:

### Tests: replace assertNotRegExp() with assertDoesNotMatchRegularExpression()

The assertRegExp() and assertNotRegExp() methods were hard deprecated in PHPUnit 9.1 and the functionality will be removed in PHPUnit 10.0.0.

The assertMatchesRegularExpression() and assertDoesNotMatchRegularExpression() methods were introduced as a replacement in PHPUnit 9.1.

This new PHPUnit method is polyfilled by the PHPUnit Polyfills and switching to it will future proof the tests some more.

Ref:

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

#63 @SergeyBiryukov
3 years ago

In 51559:

Build/Test Tools: Add Composer dependency on the PHPUnit Polyfills package.

The PHPUnit Polyfills package is an add-on for PHPUnit, which works around common issues for writing PHPUnit cross-version compatible tests.

Features:

  • It offers a full set of polyfills for assertions and expectations introduced in PHPUnit since PHPUnit 4.8.
  • It offers two generic TestCases which include these polyfills, but also solve the void return type issue for the fixtures methods.
  • It offers a PHPUnit cross-version solution for the changes to the PHPUnit TestListener implementation.
  • Supports PHPUnit 4.8 – current.
  • Supports and is compatible with PHP 5.4 – current.

The package has no outside dependencies, other than PHPUnit, is actively maintained and endorsed by the maintainer of PHPUnit itself (the only package of its kind which has ever been endorsed).

Props jrf, hellofromTonya, johnbillion, netweb, SergeyBiryukov.
See #46149.

#64 @SergeyBiryukov
3 years ago

In 51560:

Build/Test Tools: Unify the PHPUnit adapter TestCases.

This commit:

  • Removes the PHPUnit 7 specific TestCase.
  • Removes all existing polyfills from the PHPUnit 5.x TestCase.
  • Imports all polyfill traits from the PHPUnit Polyfills package into the WP_UnitTestCase class and updates the DocBlock to reflect the actual function of the class.
    • Note: The list of polyfills needs to be verified and updated after each new release of the PHPUnit Polyfills package. Alternatively (recommended), one of the built-in TestCase classes from the PHPUnit Polyfills package can be used instead.
  • Moves the require for the WP abstract-testcase.php to the bootstrap.php file.
  • Adds a require_once for the PHPUnit Polyfills autoloader to the bootstrap.php file.
    • Note: while this isn't _strictly_ necessary when the tests are run via Composer, having the include in the bootstrap allows for the tests to also be run via a PHPUnit Phar, providing contributors with more flexibility.

Follow-up to [51559].

Props jrf, hellofromTonya, johnbillion, netweb, SergeyBiryukov.
See #46149.

#65 @SergeyBiryukov
3 years ago

In 51561:

Build/Test Tools: Change the inheritance order of the abstract test classes.

As things were, the inheritance order of the abstract test classes was as follows:

WP_UnitTestCase (PHPUnit adapter layer)
    extends WP_UnitTestCase_Base (base test class)
        extends PHPUnit\Framework\TestCase (PHPUnit native class)

Concrete (child) test classes, as well as more specific abstract TestCases, are/were expected to extend the WP_UnitTestCase.

This order is not optimal as it means that the WP_UnitTestCase_Base class would not be able to benefit from any polyfills and/or shims in the PHPUnit adapter layer.

With that in mind, this commit changes the inheritance to:

WP_UnitTestCase (empty class, left in place to not break BC for plugin/theme integration tests)
    extends WP_UnitTestCase_Base (base test class)
        extends PHPUnit_Adapter_TestCase (PHPUnit adapter layer)
            extends PHPUnit\Framework\TestCase (PHPUnit native class)

The new order allows for the WP_UnitTestCase_Base to also benefit from the PHPUnit adapter layer.

For backward compatibility reasons the WP_UnitTestCase, which all test classes are (were) expected to extend, is left in place, though it is now an empty class and explicitly abstract.

Follow-up to [51559], [51560].

Props jrf, hellofromTonya, johnbillion, netweb, SergeyBiryukov.
See #46149.

#66 follow-ups: @swissspidy
3 years ago

Soo...looks like r51560 breaking a lot of plugins' test suites which are using scripts like install-wp-tests.sh (which is quite common) to get the test suite. At least in my plugin I now get You need to run composer update before running the tests. errors when trying to run tests.

Any thoughts on how this can be fixed?

#67 in reply to: ↑ 66 @jrf
3 years ago

Replying to swissspidy:

Soo...looks like r51560 breaking a lot of plugins' test suites which are using scripts like install-wp-tests.sh (which is quite common) to get the test suite. At least in my plugin I now get You need to run composer update before running the tests. errors when trying to run tests.

Any thoughts on how this can be fixed?

As per https://core.trac.wordpress.org/ticket/46149#comment:59, a dev-note will be published soonish, once all the relevant commits for the changes being made here have been committed.
The dev-note will contain information on how to make plugin/theme integration test setups compatible.

In the mean time: have you tried doing what the message says ?

#68 follow-up: @swissspidy
3 years ago

In the mean time: have you tried doing what the message says ?

The `install-wp-tests.sh` script typically downloads the tests/phpunit/includes and tests/phpunit/data folders for running the test suite. No other folders and no compoer.json file, so one can't run composer update in such a setup.

I appreciate that you all are planning on writing a dev-note soon-ish and I hope that you take this scenario into account, so that ideally we can update that shell script as well.

#69 in reply to: ↑ 68 @jeherve
3 years ago

Replying to swissspidy:

Soo...looks like r51560 breaking a lot of plugins' test suites which are using scripts like install-wp-tests.sh (which is quite common) to get the test suite. At least in my plugin I now get You need to run composer update before running the tests. errors when trying to run tests.

Just in case we need some examples, I'll jump in to say we're running into that exact same issue with tests in the Jetpack plugin, with the same setup you described in your follow-up comment:

The `install-wp-tests.sh` script typically downloads the tests/phpunit/includes and tests/phpunit/data folders for running the test suite. No other folders and no compoer.json file, so one can't run composer update in such a setup.

#70 in reply to: ↑ 66 @thomasplevy
3 years ago

My test suites running on PHP8 for my plugin utilize tests lib on the trunk/master branch in order to work around compatibility issues with building our plugin test suites off the top of the WP core test suite.... Maybe this was short sighted but I needed php8 unit tests working and this was the fastest way to get it working.

The PHPUnit polyfills works wonderfully. Thank you for all this hard work.

After suites started failing yesterday I started looking into what's going on and found my way here.

I've added the https://github.com/Yoast/PHPUnit-Polyfills as a dependency for my own project (since I install via install-wp-tests.sh (as noted above). However. I've chosen to install my test stuff in a separate directory that gets ignored from version control. As a result the reference to the autoloader assumes installation in the root directory and I can't get it working without forking the bootstrap file.

I'd like to propose a configurable vendor directory constant in the bootstrap file (similar to how the wp-tests-config.php file can be configured with a constant (https://github.com/WordPress/wordpress-develop/commit/1e88432e34a5c8405c75e5a56977362766b4cf95)

This would allow me to add a custom vendor directory location in my phpunit.xml (or elsewhere)

I've written (and tested) a patch that will allow this kind of setup and doesn't break the core tests lib (attached)

Would this be an acceptable addition to help someone in my position. Maybe I'm the only silly guy to install in a different directory...

Last edited 3 years ago by thomasplevy (previous) (diff)

@thomasplevy
3 years ago

#71 @jrf
3 years ago

@thomasplevy The changes now committed are only the first three commits of a much larger PR, with a second follow-up PR still waiting to be pulled. Please be patient and wait until everything from those PRs has been committed.

We'll sort out a solution for the polyfill autoloader, but please have a few days/weeks patience.

#72 @jrf
3 years ago

And just as a general note for anyone now panicking:

WP 5.9 is currently in early development.
As a general best practice, plugins/themes should be tested against stable versions of WP and against the alpha/beta/RC of an upcoming version, with this latter build potentially being allowed to fail.

We're nowhere near WP 5.9-alpha yet, so please give us a little time to finish these changes.

Things like the scaffold command will be taken into account and if needs be, small changes will be made to accommodate, but let's first get the bulk of the changes to fix the PHPUnit cross-version compatibility for WordPress Core committed.

If you're interested in what more is upcoming, please watch the live stream as linked above, which is available as video on demand, in which the changes are discussed in detail and/or peruse the PRs when they are opened.

#73 @thomasplevy
3 years ago

@jrf Don't mistake this as impatience. I didn't intend it that way. I saw a problem and it was unclear to me what other changes were coming and I saw a problem with my (possibly weird) setup and instead of pointing out a problem I thought I'd try to help.

Thanks for the update and for whatever it's worth I don't care how far away a release is, I test nightlies automatically. I have a small team with limited resources and advanced warning of whatever is coming is very helpful. If I wait until an RC is available and then find a problem it might be too late for my team and I to get compatibility ready before it's stable.

I appreciate the effort and work being done here and I don't intend to come off as rude or impatient.

#74 @SergeyBiryukov
3 years ago

In 51562:

Build/Test Tools: Simplify redundant PHPUnit shim for setExpectedException().

PHPUnit 6 deprecated the setExpectedException() method in favor of the expectException(), expectExceptionMessage(), and expectExceptionCode() methods.

WP_UnitTestCase_Base::setExpectedException() backfilled the old method. As the PHPUnit Polyfills have a polyfill for the new method, this backfill can now be simplified.

This backfill should be removed in a future iteration, but is, for now, left in place so as not to break backward compatibility for plugin/theme test suites which extend the WP native test suite for their integration tests.

Follow-up to [48996], [48997], [51559-51561].

Props jrf.
See #46149.

#75 @SergeyBiryukov
3 years ago

In 51563:

Tests: Replace expectException() for PHP native errors with calls to the dedicated PHPUnit 8.4+ methods.

The old manner of testing these is soft deprecated as of PHPUnit 8.4, hard deprecated as of PHPUnit 9.0 and will be removed in PHPUnit 10.0.

These dedicated methods introduced in PHPUnit 8.4 should be used as an alternative:

  • expectDeprecation()
  • expectDeprecationMessage()
  • expectDeprecationMessageMatches()
  • expectNotice()
  • expectNoticeMessage()
  • expectNoticeMessageMatches()
  • expectWarning()
  • expectWarningMessage()
  • expectWarningMessageMatches()
  • expectError()
  • expectErrorMessage()
  • expectErrorMessageMatches()

These new PHPUnit methods are all polyfilled by the PHPUnit Polyfills and switching to these will future-proof the tests some more.

References:

Follow-up to [51559-51562].

Props jrf.
See #46149.

#76 @SergeyBiryukov
3 years ago

In 51564:

Tests: Replace assertFileNotExists() with assertFileDoesNotExist().

The assertFileNotExists() method was hard deprecated in PHPUnit 9.1 and the functionality will be removed in PHPUnit 10.0.

The assertFileDoesNotExist() method was introduced as a replacement in PHPUnit 9.1.

This new PHPUnit method is polyfilled by the PHPUnit Polyfills and switching to it will future-proof the tests some more.

References:

Follow-up to [51559-51563].

Props jrf.
See #46149.

#77 @SergeyBiryukov
3 years ago

In 51565:

Tests: Replace assertRegExp() with assertMatchesRegularExpression().

The assertRegExp() and assertNotRegExp() methods were hard deprecated in PHPUnit 9.1 and the functionality will be removed in PHPUnit 10.0.

The assertMatchesRegularExpression() and assertDoesNotMatchRegularExpression() methods were introduced as a replacement in PHPUnit 9.1.

These new PHPUnit methods are polyfilled by the PHPUnit Polyfills and switching to them will future-proof the tests some more.

References:

Follow-up to [51559-51564].

Props jrf.
See #46149.

#78 @SergeyBiryukov
3 years ago

In 51566:

Tests: Replace assertNotRegExp() with assertDoesNotMatchRegularExpression().

The assertRegExp() and assertNotRegExp() methods were hard deprecated in PHPUnit 9.1 and the functionality will be removed in PHPUnit 10.0.

The assertMatchesRegularExpression() and assertDoesNotMatchRegularExpression() methods were introduced as a replacement in PHPUnit 9.1.

These new PHPUnit methods are polyfilled by the PHPUnit Polyfills and switching to them will future-proof the tests some more.

References:

Follow-up to [51559-51565].

Props jrf.
See #46149.

jrfnl commented on PR #1545:


3 years ago
#79

Closing as committed via patches 51559, 51560, 51561, 51562, 51563, 51564, 51565, 51566

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


3 years ago
#80

### Build/Tests: use the PHPUnit Polyfill Testcase as void work-around

PHPUnit 8.0.0 introduced a void return type declaration to the "fixture" methods - setUpBeforeClass(), setUp(), tearDown() and tearDownAfterClass(). As the void return type was not introduced until PHP 7.1, this makes it more difficult to create cross-version compatible tests when using fixtures, due to signature mismatches.

The Yoast\PHPUnitPolyfills\TestCases\TestCase overcomes the signature mismatch by having two versions. The correct one will be loaded depending on the PHPUnit version being used.

When using this TestCase, if an individual test, or another TestCase which extends this TestCase, needs to overload any of the "fixture" methods, it should do so by using a snake_case variant of the original fixture method name, i.e. set_up_before_class(), set_up(), assert_pre_conditions(), assert_post_conditions(), tear_down() and tear_down_after_class().

The snake_case methods will automatically be called by PHPUnit.

IMPORTANT: The snake_case methods should not call the PHPUnit parent, i.e. do not use parent::setUp() from within an overloaded set_up() method. If necessary, DO call parent::set_up().

Source: https://github.com/Yoast/PHPUnit-Polyfills#testcases

This commit:

  • Lets the PHPUnit_Adapter_TestCase extend the Yoast\PHPUnitPolyfills\TestCases\TestCase, which makes this solution for the void return type available to the WordPress test suite.
  • Removes the individual import and trait use statements for the Polyfill traits.

These are no longer necessary as the Yoast\PHPUnitPolyfills\TestCases\TestCase already includes those.

### Build/Tests: implement use of the void solution

... by renaming all declared fixture methods, and calls to parent versions of those fixture methods, from camelCase to snake_case.

### Test_WP_Widget_Media::test_constructor(): remove use of assertArraySubset()

The assertArraySubset() method has been deprecated in PHPUnit 8 and removed in PHPUnit 9.

Replacing the assertions with looping through the array and testing both the key and the value individually.

Refs:

Note: there is a polyfill package available for the removed assertion - dms/phpunit-arraysubset-asserts -, but as the assertion was only used in this one test method, adding this seems redundant.

### Build/Tests: alias the Getopt class conditionally

... as the class no longer exists in PHPUnit 9.x.

To be honest, most of the aliasing in this compat.php file is redundant as PHPUnit 5.7.21+ contains a forward compatibility layer for these classes anyway (= PHPUnit provides both the namespaced and underscore named versions of these classes in PHPUnit 5.7.21+).

All the same, I'm choosing to leave the file (and the aliases) in place for the time being, as plugins/themes using the WP test suite as the basis for their integration tests may rely on it, though WP itself should not really need it anymore, save for maybe one or two classes.

### Tests: fix tests failing due to assertContains() using strict checking

Since PHPUnit 8.0.2, the assertContains() method when checking whether a value exists in an array will do a strict type comparison of the values.

This caused a couple of tests to fail.

Using the correct data type in the test fixes that.

Refs:

### Build/Tests: handle removal of TestCase::getAnnotations()

The PHPUnit native TestCase::getAnnotations() method is used to check for WP flavoured deprecation notices, however, this method was not covered by the backward compatibility promise for PHPUnit (and was annotated as excluded).

The method has been removed as part of an internal refactor in commit sebastianbergmann/phpunit@6858204, which is included in PHPUnit 9.5.0.

For now, I've put a work-around in place, but I'd recommend that the WP expectDeprecated() method be re-evaluated in a future iteration.

### Build/Tests: remove SpeedTrapListener

Now the tests can run PHPUnit cross-version and Composer will be used to install the test suite in CI, we could switch out the local copies of the PHPUnit speedtrap package in favour of using the Composer package, which would prevent us having to make the WP local copies of the class compatible with later PHPUnit versions.

The SpeedTrap test listener was introduced to identify slow tests and take action on these to make them faster.
In practice, however, no notable action was ever taken based on the output of the testlistener in all the years the test listener was in place.

With that in mind, it was decided to remove the SpeedTrap testlisteners without replacement.

If - at a future date - contributors would want to take action to speed up slow tests anyway, they can:

### Build/Tests: loosen the PHPUnit restriction

composer.json:

Remove the PHPUnit dependency in favour of allowing the PHPUnit Polyfills library to manage the supported PHPUnit version.
This automatically now widens the supported PHPUnit versions to 5.7.21 to 9.5.8 (current).

Letting the PHPUnit Polyfills handle the version constraints for PHPUnit prevents potential version conflicts in the future as well as allows WP to benefit straight away when a new PHPUnit version would be released and the PHPUnit Polyfills package adds support for that PHPUnit version.

Test Bootstrap

Update the supported version number for PHPUnit 5.x, as the minimum PHPUnit 5.x version supported by the PHPUnit Polyfills is PHPUnit 5.7.21 and remove the PHPUnit maximum.

.gitignore:

Add the PHPUnit cache file to the list of files to be ignored.

Since PHPUnit 8, PHPUnit has a build in caching feature which creates a .phpunit.result.cache file. This file should not be committed.

Note: This same change needs to be made to the .svnignore file.

### Build/Tests: remove the copied in PHPUnit 9.x MockObject files

As the version constraints for PHPUnit now allow the tests to be run on PHPUnit 8.x and 9.x, these files are no longer needed.

### Tests: replace expectException() for PHP native errors

... with calls to the dedicated PHPUnit 8.4.0+ methods.

The old manner of testing these is soft deprecated as of PHPUnit 8.4, hard deprecated as of PHPUnit 9.0 and will be removed in PHPUnit 10.0.0.

Most calls like this were already replaced in the patch which introduced the PHPUnit Polyfills, however, this particular one could not be changed yet due to the mismatch between the PHPUnit version and the PHP version on which the tests were being run.
Fixed now anyway.

Ref:

### Build/Tests: remove redundant @requires tags

As the minimum supported PHPUnit version has been upped to PHPUnit 5.7.21, these @requires tags are now redundant.

---

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

#81 @jrf
3 years ago

Thank you @SergeyBiryukov for getting the first PR fully committed. 💖

I've now opened the second PR which addresses the void issue and various other PHPUnit cross-version compatibility issues.

We are fully aware that this will cause plugin/theme integration tests against trunk to fail (even more) for now. This was discussed in depth and we cannot avoid a breaking change here, but intend to backport select commits to older WP branches to make this less painful for plugins/themes supporting multiple WP versions.

Please bear with us while we work through all the tasks and commits.

Also note that this PR does not yet turn on the build for PHP 8.1 (even though the tests can run fine on PHP 8.1 now, though there are still a lot of failures to address), as the CI build would not run due to issues with the NPM/Docker scripts.
See this test build for details and take note that the actual failure is not so much in the test run, but in the "Install WordPress" step before it.

#82 @jrf
3 years ago

FYI: tickets #52825 and #53858 are both prerequisites before the test runs against PHP 8.1 in CI can be turned on.

With patches for those two tickets committed, the tests will run, though there will still be warnings all over the place in earlier steps and lots of test failures/errors, but at least they will run.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#83 @SergeyBiryukov
3 years ago

In 51567:

Build/Test Tools: Use the PHPUnit Polyfill TestCase as void workaround.

PHPUnit 8.0.0 introduced a void return type declaration to the "fixture" methods – setUpBeforeClass(), setUp(), tearDown() and tearDownAfterClass(). As the void return type was not introduced until PHP 7.1, this makes it more difficult to create cross-version compatible tests when using fixtures, due to signature mismatches.

The Yoast\PHPUnitPolyfills\TestCases\TestCase overcomes the signature mismatch by having two versions. The correct one will be loaded depending on the PHPUnit version being used.

When using this TestCase, if an individual test, or another TestCase which extends this TestCase, needs to overload any of the "fixture" methods, it should do so by using a snake_case variant of the original fixture method name, i.e. set_up_before_class(), set_up(), assert_pre_conditions(), assert_post_conditions(), tear_down(), and tear_down_after_class().

The snake_case methods will automatically be called by PHPUnit.

IMPORTANT: The snake_case methods should not call the PHPUnit parent, i.e. do not use parent::setUp() from within an overloaded set_up() method. If necessary, DO call parent::set_up().

Reference: https://github.com/Yoast/PHPUnit-Polyfills#testcases

This commit:

  • Lets the PHPUnit_Adapter_TestCase extend the Yoast\PHPUnitPolyfills\TestCases\TestCase, which makes this solution for the void return type available to the WordPress test suite.
  • Removes the individual import and trait use statements for the Polyfill traits. These are no longer necessary as the Yoast\PHPUnitPolyfills\TestCases\TestCase already includes those.

Follow-up to [51559-51566].

Props jrf, hellofromTonya, johnbillion, netweb, SergeyBiryukov.
See #46149.

#84 @SergeyBiryukov
3 years ago

In 51568:

Build/Test Tools: Implement use of the void solution.

PHPUnit 8.0.0 introduced a void return type declaration to the "fixture" methods – setUpBeforeClass(), setUp(), tearDown() and tearDownAfterClass(). As the void return type was not introduced until PHP 7.1, this makes it more difficult to create cross-version compatible tests when using fixtures, due to signature mismatches.

The Yoast\PHPUnitPolyfills\TestCases\TestCase overcomes the signature mismatch by having two versions. The correct one will be loaded depending on the PHPUnit version being used.

When using this TestCase, if an individual test, or another TestCase which extends this TestCase, needs to overload any of the "fixture" methods, it should do so by using a snake_case variant of the original fixture method name, i.e. set_up_before_class(), set_up(), assert_pre_conditions(), assert_post_conditions(), tear_down(), and tear_down_after_class().

The snake_case methods will automatically be called by PHPUnit.

IMPORTANT: The snake_case methods should not call the PHPUnit parent, i.e. do not use parent::setUp() from within an overloaded set_up() method. If necessary, DO call parent::set_up().

Reference: https://github.com/Yoast/PHPUnit-Polyfills#testcases

This commit renames all declared fixture methods, and calls to parent versions of those fixture methods, from camelCase to snake_case.

Follow-up to [51559-51567].

Props jrf, hellofromTonya, johnbillion, netweb, dd32, pputzer, SergeyBiryukov.
See #46149.

#85 @SergeyBiryukov
3 years ago

In 51569:

Tests: Remove use of assertArraySubset() in Test_WP_Widget_Media::test_constructor().

The assertArraySubset() method has been deprecated in PHPUnit 8 and removed in PHPUnit 9.

This replaces the assertions with looping through the array and testing both the key and the value individually.

References:

Note: There is a polyfill package available for the removed assertion: dms/phpunit-arraysubset-asserts, but as the assertion was only used in this one test method, adding this seems redundant.

Follow-up to [51559-51568].

Props jrf.
See #46149.

#86 @SergeyBiryukov
3 years ago

In 51570:

Build/Test Tools: Alias the Getopt class conditionally, as the class no longer exists in PHPUnit 9.x.

Most of the aliasing in this compat.php file is redundant as PHPUnit 5.7.21+ contains a forward compatibility layer for these classes anyway (= PHPUnit provides both the namespaced and underscore named versions of these classes in PHPUnit 5.7.21+).

All the same, the file and the aliases are left in place for the time being, as plugins/themes using the WP test suite as the basis for their integration tests may rely on it, though WP itself should not really need it anymore, save for maybe one or two classes.

Follow-up to [51559-51569].

Props jrf.
See #46149.

#87 @SergeyBiryukov
3 years ago

In 51571:

Tests: Fix tests failing due to assertContains() using strict checking.

Since PHPUnit 8.0.2, the assertContains() method, when checking whether a value exists in an array, will do a strict type comparison of the values.

This caused a couple of tests to fail. Using the correct data type in the test fixes that.

References:

Follow-up to [51559-51570].

Props jrf.
See #46149.

#88 @SergeyBiryukov
3 years ago

In 51572:

Build/Test Tools: Handle removal of TestCase::getAnnotations().

The PHPUnit native TestCase::getAnnotations() method is used to check for WP flavored deprecation notices, however, this method was not covered by the backward compatibility promise for PHPUnit (and was annotated as excluded).

The method has been removed as part of an internal refactor in commit sebastianbergmann/phpunit@6858204, which is included in PHPUnit 9.5.0.

For now, a workaround is put in place, but it is recommended that the WP expectDeprecated() method should be reevaluated in a future iteration.

Follow-up to [51559-51571].

Props jrf.
See #46149.

#89 @SergeyBiryukov
3 years ago

In 51573:

Build/Test Tools: Remove SpeedTrapListener.

Now that the tests can run PHPUnit cross-version and Composer will be used to install the test suite in CI, we could switch out the local copies of the PHPUnit speedtrap package in favor of using the Composer package, which would prevent us having to make the WP local copies of the class compatible with later PHPUnit versions.

The SpeedTrap test listener was introduced to identify slow tests and take action on these to make them faster.

In practice, however, no notable action was ever taken based on the output of the test listener in all the years it was in place.

With that in mind, it was decided to remove the SpeedTrap test listeners without replacement.

If – at a future date – contributors would want to take action to speed up slow tests anyway, they can:

  • Either add the package to their local install and use the output they receive locally to identify slow tests.
  • Or use the PHPUnit native @small annotations in combination with the PHPUnit PHP_Invoker package as described in the PHPUnit documentation to run tests with time limits.

Follow-up to [35214], [35226], [35767], [44701], [51559-51572].

Props jrf.
See #46149.

#90 @SergeyBiryukov
3 years ago

In 51574:

Build/Test Tools: Loosen the PHPUnit restriction.

composer.json:

Remove the PHPUnit dependency in favor of allowing the PHPUnit Polyfills library to manage the supported PHPUnit version. This automatically now widens the supported PHPUnit versions to 5.7.21 to 9.5.8 (current).

Letting the PHPUnit Polyfills handle the version constraints for PHPUnit prevents potential version conflicts in the future, as well as allows WordPress to benefit straight away when a new PHPUnit version would be released and the PHPUnit Polyfills package adds support for that PHPUnit version.

Test Bootstrap

Update the supported version number for PHPUnit 5.x, as the minimum PHPUnit 5.x version supported by the PHPUnit Polyfills is PHPUnit 5.7.21, and remove the PHPUnit maximum.

.gitignore and svn:ignore:

Add the PHPUnit cache file to the list of files to be ignored.

Since PHPUnit 8, PHPUnit has a built-in caching feature which creates a .phpunit.result.cache file. This file should not be committed.

Follow-up to [40536], [40853], [44701], [51559-51573].

Props jrf.
See #46149.

#91 @SergeyBiryukov
3 years ago

In 51575:

Build/Test Tools: Remove the copied-in PHPUnit 9.x MockObject files.

As the version constraints for PHPUnit now allow the tests to be run on PHPUnit 8.x and 9.x, these files are no longer needed.

Follow-up to [48957], [49037], [51544], [51559-51574].

Props jrf.
See #46149.

#92 @SergeyBiryukov
3 years ago

In 51576:

Tests: Replace expectException() for PHP native errors with calls to the dedicated PHPUnit 8.4+ methods.

The old manner of testing these is soft deprecated as of PHPUnit 8.4, hard deprecated as of PHPUnit 9.0 and will be removed in PHPUnit 10.0.

Most calls like this were already replaced in [51563], however, this particular one could not be changed yet due to the mismatch between the PHPUnit version and the PHP version on which the tests were being run. This is fixed now.

References:

Follow-up to [51559-51575].

Props jrf.
See #46149.

#93 @SergeyBiryukov
3 years ago

In 51577:

Tests: Remove redundant @requires tags.

As the minimum supported PHPUnit version has been upped to PHPUnit 5.7.21, these @requires tags are now redundant.

Follow-up to [51226], [51234], [51559-51576].

Props jrf.
See #46149.

jrfnl commented on PR #1551:


3 years ago
#94

Closing as committed via 51567, 51568, 51569, 51570, 51571, 51572, 51573, 51574, 51575, 51576, 51577

#95 @SergeyBiryukov
3 years ago

  • Owner changed from netweb to jrf

Reassigning the ownership per discussion with @jrf as the author of most of the PRs here.

#96 @jrf
3 years ago

FYI: The turning on of test runs against PHP 8.1 in CI will be handled in ticket #53891

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


3 years ago
#98

The PHPUnit Polyfills are, since [51559], a required dependency for the WP test suite and, by extension, for plugin/theme integration test suites which are based on and use (parts of) the WP Core test suite.

However, plugin/theme integration test suites may not use a full WordPress installation.

This commit:

  • Removes the presumption that a full WP install, including vendor directory, will be available when the test bootstrap.php file is run.
  • Makes the loading of the PHPUnit Polyfills autoload file more flexible by:
    • Checking if the autoload class contained within the autoload file is already available before attempting to load the file.

This allows for plugin/theme integration test suites to load the phpunitpolyfills-autoload.php file from any location, as long as it is loaded before the WP Core test bootstrap.php file is run.

  • Allowing for the path to an arbitrary installation location for the PHPUnit polyfills to be passed as a constant.

As long as the provided location is a valid file path and the phpunitpolyfills-autoload.php file exists in the provided location, that file will be loaded.
The constant can be declared in a plugin/theme integration test suite native test bootstrap file, in the wp-tests-config.php file or even in a phpunit.xml[.dist] file via <php><const name="WP_TESTS_PHPUNIT_POLYFILLS_PATH" value="path/to/yoast/phpunit-polyfills"/></php>.

  • Adds a version check for the PHPUnit Polyfills to prevent a mismatch between the version of the package expected by WordPress and the version used by plugins/themes.

The version this checks for should be in line with the minimum version requirement for the PHPUnit Polyfills as declared in the composer.json file.
This version number should only be updated when new features added in later PHPUnit Polyfills releases are actually used in the WP Core tests suite.

  • Adds appropriate error messages for every possible error condition.
  • Upgrades the PHPUnit Polyfills to version 1.0.1, which now includes a version constant.

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

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


3 years ago
#99

The PHPUnit Polyfills are, since [51559], a required dependency for the WP test suite and, by extension, for plugin/theme integration test suites which are based on and use (parts of) the WP Core test suite.

However, plugin/theme integration test suites may not use a full WordPress installation.

This commit:

  • Removes the presumption that a full WP install, including vendor directory, will be available when the test bootstrap.php file is run.
  • Makes the loading of the PHPUnit Polyfills autoload file more flexible by:
    • Checking if the autoload class contained within the autoload file is already available before attempting to load the file.

This allows for plugin/theme integration test suites to load the phpunitpolyfills-autoload.php file from any location, as long as it is loaded before the WP Core test bootstrap.php file is run.

  • Allowing for the path to an arbitrary installation location for the PHPUnit polyfills to be passed as a constant.

As long as the provided location is a valid file path and the phpunitpolyfills-autoload.php file exists in the provided location, that file will be loaded.
The constant can be declared in a plugin/theme integration test suite native test bootstrap file, in the wp-tests-config.php file or even in a phpunit.xml[.dist] file via <php><const name="WP_TESTS_PHPUNIT_POLYFILLS_PATH" value="path/to/yoast/phpunit-polyfills"/></php>.

  • Adds a version check for the PHPUnit Polyfills to prevent a mismatch between the version of the package expected by WordPress and the version used by plugins/themes.

The version this checks for should be in line with the minimum version requirement for the PHPUnit Polyfills as declared in the composer.json file.
This version number should only be updated when new features added in later PHPUnit Polyfills releases are actually used in the WP Core tests suite.

  • Adds appropriate error messages for every possible error condition.
  • Upgrades the PHPUnit Polyfills to version 1.0.1, which now includes a version constant.

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

#100 follow-up: @jrf
3 years ago

@swissspidy @jeherve @thomasplevy Now all the commits to get the WP Core test suite running on PHP 8.1 are in, I've spend some time on addressing your concerns.

I've just opened a PR on GitHub with a proposal to make the loading of the PHPUnit Polyfills package more flexible.

I'd really appreciate it if you could look it over, test it and would give your opinion.

Aside from the above, the next steps for this "project" are:

  • Prepare a proposal for which parts of these commits to backport.
  • Once consensus is reached about that proposal, apply the backports, probably to WP 5.2 to 5.8.
  • Prepare the dev-note.
  • Possibly prepare one or two plugin integration test suites for compatibility with these changes as "proof of concept". The WP-CLI scaffold command and WP Test Utils are the likely candidates to look at as a start.

swissspidy commented on PR #1563:


3 years ago
#101

I've successfully tested this patch together with a plugin that already uses PHPUnit-Polyfills and uses a shell script as added by https://github.com/wp-cli/scaffold-command to set up the integration tests.

Setting WP_TESTS_PHPUNIT_POLYFILLS_PATH works as expected and allows running tests just fine. The version comparison works as expected too.

So overall LGTM 👍

hellofromtonya commented on PR #1563:


3 years ago
#102

When running the test suite without first doing composer install, there are 2 different experiences:

Scenario 1:

  • No vendor folder as composer install not ran
  • WP_TESTS_PHPUNIT_POLYFILLS_PATH constant not defined

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
You need to run `composer update` before running the tests.
You can still use a PHPUnit phar to run them, but the dependencies do need to be installed.

Scenario 2:

  • No vendor folder as composer install not ran
  • WP_TESTS_PHPUNIT_POLYFILLS_PATH constant defined to a valid directory that does not yet have the vendor folder in it as composer install has not yet been run

{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', './valid_dir/vendor/yoast/phpunit-polyfills/' );
}}}

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
The PHPUnit Polyfills autoload file was not found in "./valid_dir/vendor/yoast/phpunit-polyfills/"
Please verify that the file path provided in the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is correct.

Notice how Scenario 2 does not guide the extender to run composer install or composer update.

@jrfnl What do you think? Should it be more descriptive?

hellofromtonya commented on PR #1563:


3 years ago
#103

When running the test suite without first doing composer install, there are 2 different experiences:

Scenario 1:

  • No vendor folder as composer install not ran
  • WP_TESTS_PHPUNIT_POLYFILLS_PATH constant not defined

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
You need to run `composer update` before running the tests.
You can still use a PHPUnit phar to run them, but the dependencies do need to be installed.

Scenario 2:

  • No vendor folder as composer install not ran
  • WP_TESTS_PHPUNIT_POLYFILLS_PATH constant defined to a valid directory that does not yet have the vendor folder in it as composer install has not yet been run

{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', './valid_dir/vendor/yoast/phpunit-polyfills/' );
}}}

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
The PHPUnit Polyfills autoload file was not found in "./valid_dir/vendor/yoast/phpunit-polyfills/"
Please verify that the file path provided in the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is correct.

Notice how Scenario 2 does not guide the extender to run composer install or composer update.

@jrfnl What do you think? Should it be more descriptive?

hellofromtonya commented on PR #1563:


3 years ago
#104

## Testing Results

### Composer Install not Run

  • No vendor folder as composer install not ran
  • WP_TESTS_PHPUNIT_POLYFILLS_PATH constant not defined

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
You need to run `composer update` before running the tests.
You can still use a PHPUnit phar to run them, but the dependencies do need to be installed.

Expected error message ✅

#### With Constant Defined

  • No vendor folder as composer install not ran
  • WP_TESTS_PHPUNIT_POLYFILLS_PATH constant defined to a valid directory that does not yet have the vendor folder in it as composer install has not yet been run

{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', './valid_dir/vendor/yoast/phpunit-polyfills/' );
}}}

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
The PHPUnit Polyfills autoload file was not found in "./valid_dir/vendor/yoast/phpunit-polyfills/"
Please verify that the file path provided in the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is correct.

Expected error message ✅

### Constant Defined: Empty

Defined an empty constant in wp-tests-config.php:
{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', );
}}}

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
The PHPUnit Polyfills autoload file was not found in ""
Please verify that the file path provided in the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is correct.

Expected error message ✅

### Constant Defined: Invalid Path

Defined the constant in wp-tests-config.php:
{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', './doesnotexist/vendor/yoast/phpunit-polyfills/' );
}}}

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
The PHPUnit Polyfills autoload file was not found in "./doesnotexist/vendor/yoast/phpunit-polyfills/"
Please verify that the file path provided in the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is correct.

Expected error message ✅

### Constant Defined: Valid Path

Defined the constant in wp-tests-config.php:
{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', './valid_dir/vendor/yoast/phpunit-polyfills/' );
}}}
and copied the vendor folder into this directory.

Results:
No error messages and tests run ✅

### Older Version of PHPUnit Polyfills Installed

Installed 1.0.0 of polyfills

Results:

Error: Version mismatch detected for the PHPUnit Polyfills. Please ensure that PHPUnit Polyfills 1.0.1 or higher is loaded.

Expected error message ✅

#### With Path Constant Defined to Valid Path

Defined the constant in wp-tests-config.php to a valid path:
{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', './valid_dir/vendor/yoast/phpunit-polyfills/' );
}}}

Results:

Error: Version mismatch detected for the PHPUnit Polyfills. Please ensure that PHPUnit Polyfills 1.0.1 or higher is loaded.

Expected error message ✅

#### With Path Constant Defined to Invalid Path

Defined the constant in wp-tests-config.php to a invalid path:
{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', './doesnotexist/vendor/yoast/phpunit-polyfills/' );
}}}

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
The PHPUnit Polyfills autoload file was not found in "./doesnotexist/vendor/yoast/phpunit-polyfills/"
Please verify that the file path provided in the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is correct.

Expected error message ✅

jrfnl commented on PR #1563:


3 years ago
#105

For visibility (on Trac) re-posting my inline reply to @hellofromtonya's earlier question:

I did it like this _on purpose_ as, in the case the constant is used, WordPress has no awareness of the type of installation (and shouldn't care either), so we cannot presume that it will be a Composer install.

The constant is expected to be pointed to the _installation directory_ for the PHPUnit Polyfills.
This can be - and most likely will be - a Composer vendor/... directory, but it can also be a stand-alone install of the PHPUnit Polyfills (tested and works just fine), a directory containing a copied in version of the Polyfills (not recommended, violates copyright, but will work), a custom install directory or even a Composer global install directory.

It is not for WordPress to dictate how a user who chooses to use the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant organizes their development environment, so no assumptions about this should be made in the message either.

Also notice that there is still a third scenario for which the same message from scenario 2 will be shown:
Scenario 3:

  • vendor folder exist, composer install or update has been run.
  • WP_TESTS_PHPUNIT_POLYFILLS_PATH constant defined, but contains a typo or otherwise points to the wrong directory, like when it would be pointed to the vendor directory without the subpath.

Scenario 3 has since been tested.

@hellofromtonya There's still one (two) scenario untested in case you want to be complete...

Scenario: Polyfills autoload file already loaded before the WordPress bootstrap file has been run.
Variant: Polyfills autoload file already loaded before the WordPress bootstrap file has been run with a version lower than the minimum required version.

Both should also work as expected and a cursory test I did for this seemed to work fine, but confirmation by a second person testing this would, of course, be welcome.

hellofromtonya commented on PR #1563:


3 years ago
#106

Scenario: Polyfills autoload file already loaded before the WordPress bootstrap file has been run.

Variant: Polyfills autoload file already loaded before the WordPress bootstrap file has been run with a version lower than the minimum required version.

## More Testing

### * Scenario: Polyfills autoload file already loaded before the WordPress bootstrap file runs

How:
Added the following code to the wp-tests-config.php file:

{{{php
require_once DIR . '/vendor/yoast/phpunit-polyfills/phpunitpolyfills-autoload.php';
}}}

Results:
No error messages and tests run as expected ✅

### * Scenario: Older version of Polyfills autoload file already loaded before the WordPress bootstrap file runs

How:

  • Install polyfills 1.0.0
  • Keep the same code from above in the wp-tests-config.php file

Results:

Error: Version mismatch detected for the PHPUnit Polyfills. Please ensure that PHPUnit Polyfills 1.0.1 or higher is loaded.

Expected error message ✅

#107 @hellofromTonya
3 years ago

  • Keywords commit added

@SergeyBiryukov PR 1563 is ready for commit.

SergeyBiryukov commented on PR #1563:


3 years ago
#108

I got this message (as expected) when testing the PR with the Polyfills 1.0:

Error: Version mismatch detected for the PHPUnit Polyfills. Please ensure that PHPUnit Polyfills 1.0.1 or higher is loaded.

However, I don't have the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant defined, and just forgot to run composer update :slightly_smiling_face:

If the constant is not defined, would it make sense to add a sentence like "Please run composer update to install the latest version"?

jrfnl commented on PR #1563:


3 years ago
#109

If the constant is not defined, would it make sense to add a sentence like "Please run composer update to install the latest version"?

Great suggestion. Applied.

#110 @SergeyBiryukov
3 years ago

In 51598:

Build/Test Tools: Make the polyfills loading more flexible.

The PHPUnit Polyfills are, since [51559], a required dependency for the WP test suite and, by extension, for plugin/theme integration test suites which are based on and use (parts of) the WP core test suite.

However, plugin/theme integration test suites may not use a full WordPress installation.

This commit:

  • Removes the presumption that a full WP install, including vendor directory, will be available when the test bootstrap.php file is run.
  • Makes the loading of the PHPUnit Polyfills autoload file more flexible by:
    • Checking if the autoload class contained within the autoload file is already available before attempting to load the file.
      This allows for plugin/theme integration test suites to load the phpunitpolyfills-autoload.php file from any location, as long as it is loaded before the WP core test bootstrap.php file is run.
    • Allowing for the path to an arbitrary installation location for the PHPUnit Polyfills to be passed as a constant.
      As long as the provided location is a valid file path and the phpunitpolyfills-autoload.php file exists in the provided location, that file will be loaded.
      The constant can be declared in a plugin/theme integration test suite native test bootstrap file, or in the wp-tests-config.php file, or even in a phpunit.xml[.dist] file via <php><const name="WP_TESTS_PHPUNIT_POLYFILLS_PATH" value="path/to/yoast/phpunit-polyfills"/></php>.
  • Adds a version check for the PHPUnit Polyfills to prevent a mismatch between the version of the package expected by WordPress and the version used by plugins/themes.
    The version this checks for should be in line with the minimum version requirement for the PHPUnit Polyfills as declared in the composer.json file.
    This version number should only be updated when new features added in later PHPUnit Polyfills releases are actually used in the WP core test suite.
  • Adds appropriate error messages for every possible error condition.
  • Upgrades the PHPUnit Polyfills to version 1.0.1, which now includes a version constant.

Follow-up to [51559-51577].

Props jrf, hellofromTonya, swissspidy, jeherve, thomasplevy, SergeyBiryukov.
See #46149.

hellofromtonya commented on PR #1563:


3 years ago
#111

Closing via changeset 51598.

#112 @hellofromTonya
3 years ago

  • Keywords commit removed

Removing commit as PR 1563 is merged via [51598].

#113 @jrf
3 years ago

Patch 46149-hard-deprecate-checkRequirements.patch is a follow-up to the previous round of changes where this method, which is not used by WP Core was missed.

Full description for the patch:

Build/Test Tools: hard deprecate WP_UnitTestCase_Base::checkRequirements().

The WP_UnitTestCase_Base::checkRequirements() method calls the parent::checkRequirements() method, but this method became private in PHPUnit 7.0.0 via commit sebastianbergmann/phpunit@932238a.

Aside from that the TestCase::getAnnotations() method which is called next is now also removed in PHPUnit 9.5.0.

WP Core doesn't use the method anymore and the method only remains to prevent potentially breaking external integration tests relying on the method.
However, in effect, the method is not functional anymore knowing the above.

So, I'm proposing to hard deprecate the method, effectively rendering it useless, except to prevent breaking test suites which still called the method.

#114 @SergeyBiryukov
3 years ago

Looking at 46149-hard-deprecate-checkRequirements.patch, I was wondering if we still need something to handle the ms-required and ms-excluded groups, but then realized they are already handled in configuration files:

  • ms-required is excluded in single-site phpunit.xml.dist.
  • ms-excluded is excluded in tests/phpunit/multisite.xml.

So the patch should be good to go.

#115 @SergeyBiryukov
3 years ago

Some history on the ms-required and ms-excluded groups:

It looks like handling of these groups was added to WP_UnitTestCase_Base::checkRequirements() to avoid fatal errors in case they are not properly excluded in plugin/theme PHPUnit configuration files, or in case plugin/theme test suites don't have a multisite-specific configuration file.

In practice, as the ::checkRequirements() method has silently stopped being called with the update to PHPUnit 7 in [44701] / #43218, it seems safe to deprecate it.

#116 @SergeyBiryukov
3 years ago

In 51602:

Build/Test Tools: Hard deprecate WP_UnitTestCase_Base::checkRequirements().

The WP_UnitTestCase_Base::checkRequirements() method calls the parent::checkRequirements() method, but this method became private in PHPUnit 7.0 via commit sebastianbergmann/phpunit@932238a.

Aside from that, the TestCase::getAnnotations() method which is called next is now also removed in PHPUnit 9.5.

WP core does not use the method anymore, and the method only remains to prevent potentially breaking external integration tests relying on the method. However, in effect, the method is not functional anymore in PHPUnit 7.0+.

Follow-up to [893/tests], [894/tests], [896/tests], [918/tests], [30526], [40520], [40564], [43005], [44701], [51559-51577].

Props jrf.
See #46149.

#117 follow-up: @SergeyBiryukov
3 years ago

@jrf Looks like [51602] broke the tests for PHP 5.6 and 7.0 (PHPUnit 5.7.27 and PHPUnit 6.5.14, respectively):

There was 1 error:

1) Tests_oEmbed_HTTP_Headers::test_rest_pre_serve_request_headers
Error: Call to undefined function xdebug_get_headers()

/var/www/tests/phpunit/tests/oembed/headers.php:32

I guess we'll still need to call parent::checkRequirements() there? That fixes the tests for me.

#118 @SergeyBiryukov
3 years ago

In 51603:

Build/Test Tools: Revert [51602] for now to investigate test failures on PHPUnit < 7.0.

See #46149.

#119 @jrf
3 years ago

@SergeyBiryukov I'll have a look a little later today.

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


3 years ago

#121 in reply to: ↑ 100 ; follow-up: @thomasplevy
3 years ago

Replying to jrf:

I've just opened a PR on GitHub with a proposal to make the loading of the PHPUnit Polyfills package more flexible.

I'd really appreciate it if you could look it over, test it and would give your opinion.

I know I'm probably a bit late as the changes already appear to be merged in but this *works perfectly* for my needs. I've been able to switch my test utils framework, drop phpunit as a dependency and include the PHPUnit Polyfills and it all loads as expected (since I'm including my composer autoloader (that makes PHPUnit Polyfills autoloader available) in my own bootstrap file loaded via my phpunit.xml config file.

I can't exactly switch today (but I also *don't have to*) as I have quite a bit of other changes to make (namely switching incompatible fixture usage over to snake case methods) and then there's some deprecations in PHP Unit 8+ that we're still using that we'll have to switch off of. But that's for me to deal with.

Thanks so much for all the hard work here @jrf -- I'm very excited to see the version compatibility matrix nightmare being handled by the WP Core so that I can run my testing matrix with a relatively small number of headaches. A million thank yous <3

@jrf
3 years ago

Build/Test Tools: hard deprecate WP_UnitTestCase_Base::checkRequirements()

#122 in reply to: ↑ 117 @jrf
3 years ago

Replying to SergeyBiryukov:

@jrf Looks like [51602] broke the tests for PHP 5.6 and 7.0 (PHPUnit 5.7.27 and PHPUnit 6.5.14, respectively):

There was 1 error:

1) Tests_oEmbed_HTTP_Headers::test_rest_pre_serve_request_headers
Error: Call to undefined function xdebug_get_headers()

/var/www/tests/phpunit/tests/oembed/headers.php:32

I guess we'll still need to call parent::checkRequirements() there? That fixes the tests for me.

Correct. As the method in PHPUnit 5 and 6 is still an overload of the PHPUnit native method, the @requires function xdebug_get_headers tags in the test method docblocks were not being correctly handled (for the two tests which are normally only run in the xdebug group).

Let's prevent the fatal error on PHPUnit >= 7.0 though, just in case people would call the WP version of the method in their tests.

I've uploaded a fixed version of the patch and ran a test build which is now passing:
https://github.com/jrfnl/wordpress-develop-official/actions/runs/1121782154

#123 in reply to: ↑ 121 @jrf
3 years ago

Replying to thomasplevy:

I know I'm probably a bit late as the changes already appear to be merged in but this *works perfectly* for my needs.
...
I'm very excited to see the version compatibility matrix nightmare being handled by the WP Core so that I can run my testing matrix with a relatively small number of headaches. A million thank yous <3

Thanks Thomas! Very happy to hear that the solution now committed works for you and yes, I'm really hoping the setup now adopted will sustain WP Core for quite a few years to come with PHPUnit Polyfills adding compatibility for future versions of PHPUnit. Hoping it will make life a lot easier in the long run, both for Core as well as for extenders.

#124 @SergeyBiryukov
3 years ago

In 51605:

Build/Test Tools: Hard deprecate WP_UnitTestCase_Base::checkRequirements().

The WP_UnitTestCase_Base::checkRequirements() method calls the parent::checkRequirements() method, but this method became private in PHPUnit 7.0 via commit sebastianbergmann/phpunit@932238a.

Aside from that, the TestCase::getAnnotations() method which is called next is now also removed in PHPUnit 9.5.

WP core does not use the method anymore, and the method only remains to prevent potentially breaking external integration tests relying on the method. However, in effect, the method is not functional anymore in PHPUnit 7.0+.

Follow-up to [893/tests], [894/tests], [896/tests], [918/tests], [30526], [40520], [40564], [43005], [44701], [51559-51577].

Props jrf.
See #46149.

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


3 years ago
#125

@schlessera gave me some really good further feedback on the messages shown when running the WP Core test bootstrap file in a plugin/theme integration test situation, most notably about when the plugin/theme test suite has not been adjusted yet for the changes in WP 5.9.

This PR attempts to address these comments.

There is one comment @schlessera made which I haven't addressed, which is to do with the use of composer update in the messages versus composer install.

Alain correctly stated that running composer install when there is no composer.lock file in place, is effectively the same as running composer update and will prevent accidental updates to other dependencies.

To address this, I have made sure that the composer update messages are now only shown in a WP Core test run context.

I have, however, not changed the message from composer update to composer install as WP Core, as of recently, no longer has a composer.lock file and if a user running the core tests has already previously run composer install, they will have a local-only composer.lock file in place.
In that situation running composer install will not update the dependency, it will verify the existing installation against the lock file and if necessary, (re-)install the dependencies as per the composer.lock file. It will also show a message that the lock file is out of date, but a next test run would in still be blocked by the outdated dependency.

@schlessera I can't tag you as a reviewer, but would appreciate it if you would look this PR over as well.

## Commit details

### Tests/bootstrap: make constant setting more flexible

The constant WP_TESTS_PHPUNIT_POLYFILLS_PATH is intended to contain the path to the root directory of the PHPUnit Polyfills library without trailing slash.

The code already took into account that the value could potentially include a trailing slash.
Now it will also take into account if it is accidentally set to point to the autoload file instead of the path.

### Tests/bootstrap: improve messaging when polyfills can not be found

Previously, two situations were taken in to account:

  1. The WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is defined => show message specific to that constant not being set correctly.

This message would typically be shown for plugin/theme integration tests which are already aware of the changes in WP 5.9.

  1. The constant is not defined => show a message to run composer update.

This message is intended for people trying to run the WP Core tests.

This left two situations unaccounted for:

  • Someone trying to run the WP Core tests, but not having set the WP_RUN_CORE_TESTS constant or not having set it to 1.
  • Someone trying to run plugin/theme integration tests without the new WP_TESTS_PHPUNIT_POLYFILLS_PATH constant being defined as they are not (yet) aware of the changes made in WP 5.9.

The changes made in this commit, are intended to improve the error messages displayed in those situations.

### Tests/bootstrap: improve messaging when polyfills do not comply with version requirements

Previously, two situations were taken into account:

  1. The WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is defined => just show a message about the version mismatch.
  2. The constant is not defined => show a message to run composer update.

This message is intended for people trying to run the WP Core tests.

This could lead to an unclear situation for people trying to run plugin/theme integration tests without the new WP_TESTS_PHPUNIT_POLYFILLS_PATH constant being defined.

They could be shown the message to run composer update while if they would do so for their local install without adding the Polyfills, the message would still display the next time they would attempt to run the tests.

This commit:

  1. Provides more information about the PHPUnit Polyfills version detected vs the version expected.
  2. Shows a more specific message to guide users which have the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant declared.
  3. Only shows the message to run composer update when the WP_RUN_CORE_TESTS constant is declared to prevent confusing people more.

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

#126 @jrf
3 years ago

FYI: I've opened a new PR with a further iteration on the test bootstrap messages around the Polyfills requirement. Feedback and reviews welcome.

hellofromtonya commented on PR #1587:


3 years ago
#127

## Test Report

### Scenario 1: Someone trying to run the WP Core tests, but not having set the WP_RUN_CORE_TESTS constant or not having set it to 1

  • define( 'WP_RUN_CORE_TESTS', false );
  • The constant WP_TESTS_PHPUNIT_POLYFILLS_PATH is not defined
  • did not run composer install or composer update
  • ran tests

Results:

Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
If you are trying to run plugin/theme integration tests, make sure the PHPUnit Polyfills library is available and either load the autoload file of this library in your own test bootstrap before calling the WP Core test bootstrap file; or set the path to the PHPUnit Polyfills library in a "WP_TESTS_PHPUNIT_POLYFILLS_PATH" constant to allow the WP Core bootstrap to load the Polyfills.

If you are trying to run the WP Core tests, make sure to set the "WP_RUN_CORE_TESTS" constant to 1 and run `composer update` before running the tests.
Once the dependencies are installed, you can run the tests using the Composer-installed version of PHPUnit or using a PHPUnit phar file, but the dependencies do need to be installed whichever way the tests are run.

Works as expected ✅

### Scenario 2: Someone trying to run plugin/theme integration tests without the new WP_TESTS_PHPUNIT_POLYFILLS_PATH constant being defined as they are not (yet) aware of the changes made in WP 5.9

Steps to test:

  1. Run composer install
  2. Create a new directory in the root of the project called `my-vendors'
  3. Move the yoast folder from vendor folder and put it into the new my-vendors folder
  4. Add the following code to the wp-test-config.php file:

{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', './my-vendors/yoast/phpunit-polyfills/phpunitpolyfills-autoload.php' );
}}}

  1. Run the tests

Results:

  • Tests are successfully ✅

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


3 years ago

hellofromtonya commented on PR #1587:


3 years ago
#129

# Test Report

## Plugin/theme integration tests

### Scenario 1: Constant not defined and no Composer packages installed

  • WP_RUN_CORE_TESTS is 0
  • The constant WP_TESTS_PHPUNIT_POLYFILLS_PATH is not defined
  • Composer packages not installed
  • Run the tests

Results:

  • Tests do not run
  • Error message in command-line:
    Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
    If you are trying to run plugin/theme integration tests, make sure the PHPUnit Polyfills library (https://github.com/Yoast/PHPUnit-Polyfills) is available and either load the autoload file of this library in your own test bootstrap before calling the WP Core test bootstrap file; or set the absolute path to the PHPUnit Polyfills library in a "WP_TESTS_PHPUNIT_POLYFILLS_PATH" constant to allow the WP Core bootstrap to load the Polyfills.
    
    If you are trying to run the WP Core tests, make sure to set the "WP_RUN_CORE_TESTS" constant to 1 and run `composer update` before running the tests.
    Once the dependencies are installed, you can run the tests using the Composer-installed version of PHPUnit or using a PHPUnit phar file, but the dependencies do need to be installed whichever way the tests are run.
    

Works as expected ✅

### Scenario 2: Constant defined as relative path with Composer packages installed

  • WP_RUN_CORE_TESTS is 0
  • Composer packages are installed, but in different folder than vendor. This can be set in the plugin/theme's composer.json file, for example:
        "config": {
    	  "vendor-dir": "./plugin-vendors"
        }
    
  • Constant is defined as a relative path. For example:

{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', '/plugin-vendors/yoast/phpunit-polyfills' );
}}}

  • Run the tests

Results:

  • Tests do not run
  • Error message in command-line:
    Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
    The PHPUnit Polyfills autoload file was not found in "/plugin-vendors/yoast/phpunit-polyfills"
    Please verify that the file path provided in the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is correct.
    The WP_TESTS_PHPUNIT_POLYFILLS_PATH constant should contain an absolute path to the root directory of the PHPUnit Polyfills library.
    

Works as expected ✅

### Scenario 3: Constant defined as absolute path without the autoload file with Composer packages installed.

  • Repeat previous scenario
  • Change the constant to an absolute path, e.g.:

{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', DIR . '/plugin-vendors/yoast/phpunit-polyfills' );
}}}

  • Run the tests

Results:

  • No error messages ✅
  • Tests run ✅

Works as expected ✅

### Scenario 4: Constant defined as absolute path with the autoload file.

  • Repeat previous scenario
  • Change constant to include autoload file, e.g.:

{{{php
define( 'WP_TESTS_PHPUNIT_POLYFILLS_PATH', DIR . '/plugin-vendors/yoast/phpunit-polyfills/phpunitpolyfills-autoload.php' );
}}}

  • Run the tests

Results:

  • No error messages ✅
  • Tests run ✅

Works as expected ✅

### Scenario 3: Version mismatch

  • Repeat previous scenario
  • Wrong version of Polyfills package is installed, e.g.:
    composer require yoast/phpunit-polyfills:1.0.0
    
  • Run the tests

Results:

  • Tests do not run
  • Error message in command-line:
    Error: Version mismatch detected for the PHPUnit Polyfills. Please ensure that PHPUnit Polyfills 1.0.1 or higher is loaded. Found version: 1.0.0 or lower
    Please run `composer update` to install the latest version.
    

Works as expected ✅

## Core Tests

### Scenario 1: WP_RUN_CORE_TESTS constant set it to 0

  • WP_RUN_CORE_TESTS is 0
  • The constant WP_TESTS_PHPUNIT_POLYFILLS_PATH is not defined
  • Composer packages not installed
  • Run the tests

Results:

  • Tests do not run
  • Error message in command-line:
    Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
    If you are trying to run plugin/theme integration tests, make sure the PHPUnit Polyfills library (https://github.com/Yoast/PHPUnit-Polyfills) is available and either load the autoload file of this library in your own test bootstrap before calling the WP Core test bootstrap file; or set the absolute path to the PHPUnit Polyfills library in a "WP_TESTS_PHPUNIT_POLYFILLS_PATH" constant to allow the WP Core bootstrap to load the Polyfills.
    
    If you are trying to run the WP Core tests, make sure to set the "WP_RUN_CORE_TESTS" constant to 1 and run `composer update` before running the tests.
    Once the dependencies are installed, you can run the tests using the Composer-installed version of PHPUnit or using a PHPUnit phar file, but the dependencies do need to be installed whichever way the tests are run.
    

Works as expected ✅

### Scenario 2: Composer packages not installed

  • WP_RUN_CORE_TESTS is 1
  • The constant WP_TESTS_PHPUNIT_POLYFILLS_PATH is not defined
  • did not run composer install or composer update
  • ran tests

Results:

  • Tests do not run
  • Error message in command-line:
    Error: The PHPUnit Polyfills library is a requirement for running the WP test suite.
    You need to run `composer update` before running the tests.
    Once the dependencies are installed, you can run the tests using the Composer-installed version of PHPUnit or using a PHPUnit phar file, but the dependencies do need to be installed whichever way the tests are run.
    

Works as expected ✅

### Scenario 3: Version mismatch

  • WP_RUN_CORE_TESTS is 1
  • Composer packages are installed
  • Wrong version of Polyfills package is installed, e.g.:
    composer require yoast/phpunit-polyfills:1.0.0
    
  • Run the tests

Results:

  • Tests do not run
  • Error message in command-line:
    Error: Version mismatch detected for the PHPUnit Polyfills. Please ensure that PHPUnit Polyfills 1.0.1 or higher is loaded. Found version: 1.0.0 or lower
    Please run `composer update` to install the latest version.
    

Works as expected ✅

hellofromtonya commented on PR #1587:


3 years ago
#130

Per feedback and consensus, the latest changes require the Polyfills path to be absolute. In my testing, changes work as expected.

Next step plan: Tomorrow I'll commit this PR unless there are any objections or further feedback that requires action.
Why tomorrow? Gives more time for folks to test and review.

jrfnl commented on PR #1587:


3 years ago
#131

@hellofromtonya Thanks for your extensive testing Tonya!

#132 @hellofromTonya
3 years ago

In 51810:

Build/Test Tools: Make WP_TESTS_PHPUNIT_POLYFILLS_PATH more flexible.

The constant WP_TESTS_PHPUNIT_POLYFILLS_PATH is intended to contain the path to the root directory of the PHPUnit Polyfills library without trailing slash.

The code already took into account that the value could potentially include a trailing slash.

Now it will also take into account if it is accidentally set to point to the autoload file instead of the path.

Follow-up to [51598].

Props jrf, schlessera, hellofromTonya, jeherve, lucatume.
See #46149.

#133 @hellofromTonya
3 years ago

In 51811:

Build/Test Tools: Improve messaging when PHPUnit Polyfills cannot be found.

Previously, two situations were taken in to account:

  1. The WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is defined => show message specific to that constant not being set correctly.

This message would typically be shown for plugin/theme integration tests which are already aware of the changes in WP 5.9.

  1. The constant is not defined => show a message to run composer update.

This message is intended for people trying to run the WP Core tests.

This left two situations unaccounted for:

  • Someone trying to run the WP Core tests, but not having set the WP_RUN_CORE_TESTS constant or not having set it to 1.
  • Someone trying to run plugin/theme integration tests without the new WP_TESTS_PHPUNIT_POLYFILLS_PATH constant being defined as they are not (yet) aware of the changes made in WP 5.9.

The changes made in this commit, are intended to improve the error messages displayed in those situations.

Follow-up to [51598], [51810].

Props jrf, schlessera, hellofromTonya, jeherve, lucatume.
See #46149.

#134 @hellofromTonya
3 years ago

In 51812:

Build/Test Tools: Improve messaging when PHPUnit Polyfills do not comply with version requirements.

Previously, two situations were taken in to account:

  1. The WP_TESTS_PHPUNIT_POLYFILLS_PATH constant is defined => just show a message about the version mismatch.
  2. The constant is not defined => show a message to run composer update. This message is intended for people trying to run the WP Core tests.

This could lead to an unclear situation for people trying to run plugin/theme integration tests without the new WP_TESTS_PHPUNIT_POLYFILLS_PATH constant being defined.

They could be shown the message to run composer update while if they would do so for their local install without adding the Polyfills, the message would still display the next time they would attempt to run the tests.

This commit:

  1. Provides more information about the PHPUnit Polyfills version detected vs the version expected.
  2. Shows a more specific message to guide users which have the WP_TESTS_PHPUNIT_POLYFILLS_PATH constant declared.
  3. Only shows the message to run composer update when the WP_RUN_CORE_TESTS constant is declared to prevent confusing people more.

Follow-up to [51598], [51810], [51811].

Props jrf, schlessera, hellofromTonya, jeherve, lucatume.
See #46149.

#135 @hellofromTonya
3 years ago

In 51813:

Build/Test Tools: Expect an absolute path in WP_TESTS_PHPUNIT_POLYFILLS_PATH constant.

This commit:

  • Removes the use of realpath() to prevent issues with WSL and other virtualized filesystems.
  • Changes the logic of the Polyfill bootstrap loading to expect an absolute path, rather than a relative path to the root directory of the PHPUnit Polyfills library.
  • Adjusts the relevant inline documentation and error messages to expect an absolute path.
  • Breaks up error messages into smaller line lengths for readability.

Follow-up to [51598], [51810], [51811], [51812].

Props jrf, schlessera, hellofromTonya, jeherve, lucatume.
See #46149.

hellofromtonya commented on PR #1587:


3 years ago
#136

Committed in the following changesets: 51810-51813.

jrfnl commented on PR #1587:


3 years ago
#137

FYI: PR #1588 (backporting of the test changes to WP 5.2 - 5.8) has been updated to be in sync with the additional changes which were proposed in this PR as they have now been committed.

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


3 years ago

@jrf
2 years ago

Test bootstrap: refine the Composer command mentioned in error messages

#139 @hellofromTonya
2 years ago

In 51828:

Build/Test Tools: Improve Composer update command in bootstrap error messages.

Refines the test bootstrap error message to include the -W in the Composer update command.

Why?

To also update the chain of dependencies for the tests' dependencies.

composer update will update the tests' direct dependencies.

composer update -W will update the dependencies including *their* dependencies, which is the recommended course of action for WP.

Follow-up to [51598], [51811], [51813].

Props jrf.
See #46149.

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


2 years ago

#141 @hellofromTonya
2 years ago

  • Keywords has-dev-note added; needs-dev-note removed
  • Resolution set to fixed
  • Status changed from assigned to closed

With the publication of the dev note, all work for the scope of this ticket is complete. Closing this ticket.

Thank you everyone for collaborating together to make this happen!

Note: See TracTickets for help on using tickets.