Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests: make test suite compatible with PHPUnit 8 and 9 (Trac 46149) #484

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Aug 16, 2020

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


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

... use `composer install` instead.

The unit tests currently aren't running on PHP 8.0/nightly, which is kind of detrimental for any preparation efforts to make WP compatible with PHP 8.0.

The reason for this that one of the dependencies of PHPUnit _itself_ at version 7.x is not compatible with PHP 8.0 at the version which is included in the last PHPUnit 7.x phar.

The problematic dependency _has_ released a new version in which the problem is resolved, however as PHPUnit 7 is no longer supported, the phar will not be updated with this more recent dependency.

Doing a `composer install` and running the tests via the composer installed PHPUnit version will solve this as the more recent version of the dependency will be used in that case.

This commit implements this work-around in the Travis script to allow the tests to run on PHP 8.0.

---

PHP 8.0 raises the error level of quite some notices and warnings to (catchable) exceptions, which when uncaught result in fatal errors.
PHP 8.0 also makes a number of changes in how numeric strings are handled in comparisons.

In nearly all cases, issues resulting from either or these changes will **NOT** be detectable by static analysis tools as it involves the runtime value of variables.

So being able to see which tests are failing and with what error message is imperative to being able to make WP compatible with PHP 8.0.
Reasoning for this specific version:
* 80% of 7.1 sites are on 7.1.26 or higher.
* PHP 7.1.26 has less than 20 known security vulnerabilities.

Sources:
* https://w3techs.com/technologies/details/pl-php/7.1
* https://www.cvedetails.com/version-list/74/128/2/PHP-PHP.html?sha=78e084dbc6897971bd02e634619a193e623904d6&order=1&trc=1128
Move the `WP_TEST_REPORTER=true` flag to the PHP 7.1 build.
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.
The `assertNotFalse()` has existed in PHPUnit since, at least, version 4.x. While declaring the method may have been necessary for PHPUnit 3.x, it hasn't been necessary for a long time and _most definitely_ never was necessary in the PHPUnit 7.x test case...
... and rename the `phpunit/includes/phpunit7/testcase.php` file to `phpunit/includes/testcase.php`.
* Adjust the PHPUnit config files to load the speed-trap-listener file directly.
* Remove the `listener-loader.php` file.
* Rename the `phpunit/includes/phpunit7/speed-trap-listener.php` file to `phpunit/includes/speed-trap-listener.php`, effectively removing the PHPUnit < 7 speed-trap-listener.
No need for the feature toggle anymore, just return the plain version number.
…t shim

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

The `WP_UnitTestCase_Base::setExpectedException()` backfilled the method.
As PHPUnit < 6 is no longer supported, let's just use the (modern) PHPUnit native way of expecting exceptions instead.

Includes replacing all calls to the removed method with calls to the PHPUnit native functionality.
Add the PHP 7.1 `void` return type to all `setUpBeforeClass()`, `setUp()`, `tearDown()` and `tearDownAfterClass()` method declarations.

This is a requirement as of PHPUnit 8.
Switch out uses of the `PHPUnit_Framework_TestCase` class (alias) for the namespaced `PHPUnit\Framework\TestCase` class.

Includes adding import `use` statements to that effect where relevant.
Switch out uses of the `PHPUnit_Framework_Exception` class (alias) for the namespaced `PHPUnit\Framework\Exception` class.

Includes adding import `use` statements to that effect where relevant.
Includes removing a reference to the old class name from the PHPCS config file.
Switch out uses of the `PHPUnit_Framework_ExpectationFailedException` class (alias) for the namespaced `PHPUnit\Framework\ExpectationFailedException` class.

Includes adding import `use` statements to that effect where relevant.
Switch out uses of the `PHPUnit_Framework_Error_Deprecated` class (alias) for the namespaced `PHPUnit\Framework\Error\Deprecated` class.

Includes adding import `use` statements to that effect where relevant.

Includes removing use of `@expectedException` annotations using the class as those annotations were deprecated in PHPUnit 8 and removed in PHPUnit 9.
Switch out uses of the `PHPUnit_Framework_Error_Notice` class (alias) for the namespaced `PHPUnit\Framework\Error\Notice` class.

Includes adding import `use` statements to that effect where relevant.

Includes removing use of `@expectedException` annotations using the class as those annotations were deprecated in PHPUnit 8 and removed in PHPUnit 9.
Switch out uses of the `PHPUnit_Framework_Error_Warning` class (alias) for the namespaced `PHPUnit\Framework\Error\Warning` class.

Includes adding import `use` statements to that effect where relevant.
Switch out uses of the `PHPUnit_Framework_Test` class (alias) for the namespaced `PHPUnit\Framework\Test` class.

Includes adding import `use` statements to that effect where relevant.
Switch out uses of the `PHPUnit_Framework_Warning` class (alias) for the namespaced `PHPUnit\Framework\Warning` class.

Includes adding import `use` statements to that effect where relevant.
Switch out uses of the `PHPUnit_Framework_AssertionFailedError` class (alias) for the namespaced `PHPUnit\Framework\AssertionFailedError` class.

Includes adding import `use` statements to that effect where relevant.
Switch out uses of the `PHPUnit_Framework_TestSuite` class (alias) for the namespaced `PHPUnit\Framework\TestSuite` class.

Includes adding import `use` statements to that effect where relevant.
Switch out uses of the `PHPUnit_Framework_TestListener` class (alias) for the namespaced `PHPUnit\Framework\TestListener` class.

Includes adding import `use` statements to that effect where relevant.
Switch out uses of the `PHPUnit_Framework_GlobalState` class (alias) for the namespaced `PHPUnit\Framework\GlobalState` class.

Includes adding import `use` statements to that effect where relevant.
Removes the use of `@expectedException` annotations in favour of using the `expectException()`, `expectExceptionMessage()` and/or `expectExceptionCode()` methods.

The annotations were deprecated in PHPUnit 8 and removed in PHPUnit 9.
…sString()`

... when used with strings.

Using `assertContains()` and `assertNotContains()` with string haystacks was soft deprecated in PHPUnit 7.5, hard deprecated in PHPUnit 8.0 and the functionality was removed in PHPUnit 9.0.

For string haystacks, the `assertStringContainsString()`, `assertStringContainsStringIgnoringCase()`, `assertStringNotContainsString()` and `assertStringNotContainsStringIgnoringCase()` were introduced as replacements in PHPUnit 7.5.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/7.5.20/ChangeLog-7.5.md#750---2018-12-07
* sebastianbergmann/phpunit#3422
…tIs[Not]Array()`

The `assertInternalType()` and `assertNotInternalType()` methods were soft deprecated in PHPUnit 7.5, hard deprecated in PHPUnit 8.0 and the functionality was removed in PHPUnit 9.0.

The more specific `assertIsArray()`, `assertIsNotArray()`, `assertIsBool()`, `assertIsNotBool()`, `assertIsFloat()`, `assertIsNotFloat()`, `assertIsInt()`, `assertIsNotInt()`, `assertIsNumeric()`, `assertIsNotNumeric()`, `assertIsObject()`, `assertIsNotObject()`, `assertIsResource()`, `assertIsNotResource()`, `assertIsString()`,
`assertIsNotString()`, `assertIsScalar()`, `assertIsNotScalar()`, `assertIsCallable()`, `assertIsNotCallable()`, `assertIsIterable()`, `assertIsNotIterable()` methods were introduced as replacements in PHPUnit 7.5.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/7.5.20/ChangeLog-7.5.md#750---2018-12-07
* sebastianbergmann/phpunit#3368
…Is[Not]Bool()`

The `assertInternalType()` and `assertNotInternalType()` methods were soft deprecated in PHPUnit 7.5, hard deprecated in PHPUnit 8.0 and the functionality was removed in PHPUnit 9.0.

The more specific `assertIsArray()`, `assertIsNotArray()`, `assertIsBool()`, `assertIsNotBool()`, `assertIsFloat()`, `assertIsNotFloat()`, `assertIsInt()`, `assertIsNotInt()`, `assertIsNumeric()`, `assertIsNotNumeric()`, `assertIsObject()`, `assertIsNotObject()`, `assertIsResource()`, `assertIsNotResource()`, `assertIsString()`,
`assertIsNotString()`, `assertIsScalar()`, `assertIsNotScalar()`, `assertIsCallable()`, `assertIsNotCallable()`, `assertIsIterable()`, `assertIsNotIterable()` methods were introduced as replacements in PHPUnit 7.5.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/7.5.20/ChangeLog-7.5.md#750---2018-12-07
* sebastianbergmann/phpunit#3368
…tIs[Not]Float()`

The `assertInternalType()` and `assertNotInternalType()` methods were soft deprecated in PHPUnit 7.5, hard deprecated in PHPUnit 8.0 and the functionality was removed in PHPUnit 9.0.

The more specific `assertIsArray()`, `assertIsNotArray()`, `assertIsBool()`, `assertIsNotBool()`, `assertIsFloat()`, `assertIsNotFloat()`, `assertIsInt()`, `assertIsNotInt()`, `assertIsNumeric()`, `assertIsNotNumeric()`, `assertIsObject()`, `assertIsNotObject()`, `assertIsResource()`, `assertIsNotResource()`, `assertIsString()`,
`assertIsNotString()`, `assertIsScalar()`, `assertIsNotScalar()`, `assertIsCallable()`, `assertIsNotCallable()`, `assertIsIterable()`, `assertIsNotIterable()` methods were introduced as replacements in PHPUnit 7.5.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/7.5.20/ChangeLog-7.5.md#750---2018-12-07
* sebastianbergmann/phpunit#3368
…s[Not]Int()`

The `assertInternalType()` and `assertNotInternalType()` methods were soft deprecated in PHPUnit 7.5, hard deprecated in PHPUnit 8.0 and the functionality was removed in PHPUnit 9.0.

The more specific `assertIsArray()`, `assertIsNotArray()`, `assertIsBool()`, `assertIsNotBool()`, `assertIsFloat()`, `assertIsNotFloat()`, `assertIsInt()`, `assertIsNotInt()`, `assertIsNumeric()`, `assertIsNotNumeric()`, `assertIsObject()`, `assertIsNotObject()`, `assertIsResource()`, `assertIsNotResource()`, `assertIsString()`,
`assertIsNotString()`, `assertIsScalar()`, `assertIsNotScalar()`, `assertIsCallable()`, `assertIsNotCallable()`, `assertIsIterable()`, `assertIsNotIterable()` methods were introduced as replacements in PHPUnit 7.5.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/7.5.20/ChangeLog-7.5.md#750---2018-12-07
* sebastianbergmann/phpunit#3368

Includes removing a few assignments made within function calls.
…rtIs[Not]Object()`

The `assertInternalType()` and `assertNotInternalType()` methods were soft deprecated in PHPUnit 7.5, hard deprecated in PHPUnit 8.0 and the functionality was removed in PHPUnit 9.0.

The more specific `assertIsArray()`, `assertIsNotArray()`, `assertIsBool()`, `assertIsNotBool()`, `assertIsFloat()`, `assertIsNotFloat()`, `assertIsInt()`, `assertIsNotInt()`, `assertIsNumeric()`, `assertIsNotNumeric()`, `assertIsObject()`, `assertIsNotObject()`, `assertIsResource()`, `assertIsNotResource()`, `assertIsString()`,
`assertIsNotString()`, `assertIsScalar()`, `assertIsNotScalar()`, `assertIsCallable()`, `assertIsNotCallable()`, `assertIsIterable()`, `assertIsNotIterable()` methods were introduced as replacements in PHPUnit 7.5.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/7.5.20/ChangeLog-7.5.md#750---2018-12-07
* sebastianbergmann/phpunit#3368
…sertIs[Not]Resource()`

The `assertInternalType()` and `assertNotInternalType()` methods were soft deprecated in PHPUnit 7.5, hard deprecated in PHPUnit 8.0 and the functionality was removed in PHPUnit 9.0.

The more specific `assertIsArray()`, `assertIsNotArray()`, `assertIsBool()`, `assertIsNotBool()`, `assertIsFloat()`, `assertIsNotFloat()`, `assertIsInt()`, `assertIsNotInt()`, `assertIsNumeric()`, `assertIsNotNumeric()`, `assertIsObject()`, `assertIsNotObject()`, `assertIsResource()`, `assertIsNotResource()`, `assertIsString()`,
`assertIsNotString()`, `assertIsScalar()`, `assertIsNotScalar()`, `assertIsCallable()`, `assertIsNotCallable()`, `assertIsIterable()`, `assertIsNotIterable()` methods were introduced as replacements in PHPUnit 7.5.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/7.5.20/ChangeLog-7.5.md#750---2018-12-07
* sebastianbergmann/phpunit#3368
…rtIs[Not]String()`

The `assertInternalType()` and `assertNotInternalType()` methods were soft deprecated in PHPUnit 7.5, hard deprecated in PHPUnit 8.0 and the functionality was removed in PHPUnit 9.0.

The more specific `assertIsArray()`, `assertIsNotArray()`, `assertIsBool()`, `assertIsNotBool()`, `assertIsFloat()`, `assertIsNotFloat()`, `assertIsInt()`, `assertIsNotInt()`, `assertIsNumeric()`, `assertIsNotNumeric()`, `assertIsObject()`, `assertIsNotObject()`, `assertIsResource()`, `assertIsNotResource()`, `assertIsString()`,
`assertIsNotString()`, `assertIsScalar()`, `assertIsNotScalar()`, `assertIsCallable()`, `assertIsNotCallable()`, `assertIsIterable()`, `assertIsNotIterable()` methods were introduced as replacements in PHPUnit 7.5.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/7.5.20/ChangeLog-7.5.md#750---2018-12-07
* sebastianbergmann/phpunit#3368
Removes the `$int_fields` array and the loop in favour of plain assertions.
…ta()`

The `$delta` parameter of the `assertEquals()` method was soft deprecated in PHPUnit 7.5, hard deprecated in PHPUnit 8.0 and the functionality was removed in PHPUnit 9.0.

The more specific `assertEqualsWithDelta()` (and `assertNotEqualsWithDelta()`) were introduced as replacements in PHPUnit 7.5.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/7.5.20/ChangeLog-7.5.md#750---2018-12-07
* sebastianbergmann/phpunit#3340
…bset()

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:
* https://github.com/sebastianbergmann/phpunit/blob/8.0.6/ChangeLog-8.0.md#800---2019-02-01
* sebastianbergmann/phpunit#3494
Use a trait to selectively polyfill a number of methods introduced in PHPUnit 8.4.

These new PHPUnit methods replace the use of the `expectException()` et al methods to test for expected PHP native errors, warnings and notices.

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 trait contains the complete set  of methods related to this change.

The polyfill methods themselves are effectively just a placeholder/wrapper, they will use the PHPUnit native functions to execute the assertions.

With the polyfill in place, the WP test suite can already be upgraded to use the _current_ way of expecting PHP native errors/warnings/notices.

Once the need to support PHPUnit < 8.4 is dropped, the polyfill can be removed and that is all that is needed at that time.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/8.4.3/ChangeLog-8.4.md#840---2019-10-04
* sebastianbergmann/phpunit#3775
... 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 methods have been polyfilled for WordPress.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/8.4.3/ChangeLog-8.4.md#840---2019-10-04
* sebastianbergmann/phpunit#3775
Use a trait to selectively polyfill a number of methods introduced in PHPUnit 9.1.

These new PHPUnit methods all replace already existing methods, effectively renaming the old methods.

The trait only contains polyfill methods for assertion methods currently used in the WordPress test suite, but can be expanded to cover the complete set of PHPUnit 9.1 renamed methods.

The polyfill methods themselves are effectively just a placeholder/wrapper, they will use the PHPUnit native functions to execute the assertions.

With the polyfill in place, the WP test suite can already be upgraded to use the _current_ name of these functions, rather than the old name.

Once the need to support PHPUnit < 9.1 is dropped, the polyfill can be removed and that is all that is needed at that time.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/9.1.5/ChangeLog-9.1.md#910---2020-04-03
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.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/9.1.5/ChangeLog-9.1.md#910---2020-04-03
* sebastianbergmann/phpunit#4076
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.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/9.1.5/ChangeLog-9.1.md#910---2020-04-03
* sebastianbergmann/phpunit#4085
* sebastianbergmann/phpunit#4088
…ression()`

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.

Ref:
* https://github.com/sebastianbergmann/phpunit/blob/9.1.5/ChangeLog-9.1.md#910---2020-04-03
* sebastianbergmann/phpunit#4085
* sebastianbergmann/phpunit#4088
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:
* https://github.com/sebastianbergmann/phpunit/blob/8.0.6/ChangeLog-8.0.md#802---2019-02-07
* sebastianbergmann/phpunit#3511
* sebastianbergmann/phpunit@6205f33
… assertion

The `assertContains()` method is intended to check if a _value_ exists in an array, not a _key_.

In this particular test, the `author_avatar_urls` is a key, not a value, so the `assertArrayHasKey()` method should be used instead.

PHPUnit was in older versions more tolerant with these type of quirks, but that is not the case anymore.
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff @jrfnl 💯

I've left a handful of trivial comments, nothing here in my opinion is blocking this from being merged/committed today as is 👨🏼‍🎤

$this->assertIsObject( $deleted_term );
$this->assertIsInt( $term );
$this->assertIsArray( $object_ids );
// Pesky string $this->assertIsInt( $tt_id );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Pesky string $this->assertIsInt( $tt_id );
$this->assertIsInt( $tt_id );

I think this comment can now be removed, it was added in jrfnl@819ea5c#diff-b9de4bf7426632e1758cc0dd9e7dff6cR627 and with the switch it would be worth including this assertion again as this line of code is being touched, if the test doesn't work then an updated comment to follow it up later maybe.

Note: Not a blocker

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That goes for a number of other commented out/disabled tests as well, however, I think that should be done in another ticket, so as not to confuse the issue.

This combination of patches addresses only the PHPUnit compatibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

Comment on lines -894 to +895
$this->assertContains( 2, $meta );
$this->assertContains( 8, $meta );
$this->assertContains( '2', $meta );
$this->assertContains( '8', $meta );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the assertion from integer to string here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1000 x yes. The values come back from WP as strings, not integers.

As per the commit message of the patch in which I made this change:

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:

Comment on lines -841 to +842
$this->assertContains( 2, $meta );
$this->assertContains( 8, $meta );
$this->assertContains( '2', $meta );
$this->assertContains( '8', $meta );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the assertion from integer to string here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above.

@@ -2,20 +2,6 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to keep this file at all now?

In composer.json only PHPUnit versions 7, 8 & 9 are being installed

Also, if removing this file also removing the file reference here at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question and one I intended to ask myself.

I've been wondering about removing this file, but couldn't figure out what the use of the PHPUnit_Util_Test class was (without digging in deep), As the test suite passes with it still in place, it is not a compatibility issue and can be addressed later.

@@ -1,10 +1,17 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phpunit-speedtrap supports PHP 7, 8, & 9, should we drop our custom implementation and load this via Composer now instead?

Copy link
Member Author

@jrfnl jrfnl Aug 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The tests when run in Travis use the Travis shipped PHPUnit phar and don't do a Composer install, so the speedtrap would not be available then.

(This is part of the problem of why the tests weren't running on PHP 8, which is addressed and explained in detail in Trac #50902).

@jrfnl
Copy link
Member Author

jrfnl commented Aug 16, 2020

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

@ntwb
Copy link
Member

ntwb commented Aug 16, 2020

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

@jrfnl
Copy link
Member Author

jrfnl commented Aug 16, 2020

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.

@ntwb
Copy link
Member

ntwb commented Aug 16, 2020

Whilst I agree, and would love to drop PHP 5.6 & 7.0, alienating 15% of the WordPress install base at this time I don’t see happening, if the site-health time can make significant inroads in the next 6 months it may be possible, but sadly I’m not optimistic this will get there in time 😢

@ntwb
Copy link
Member

ntwb commented Aug 17, 2020

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 🤞🏼

@jrfnl jrfnl closed this Aug 19, 2020
@jrfnl jrfnl deleted the trac-46149/make-test-suite-compatible-with-phpunit-8-9 branch August 19, 2020 12:29
dd32 added a commit to dd32/wordpress-develop that referenced this pull request Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants