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

Build/Tests: make the tests compatible with phpunit 8 and 9 (Trac 46149) #1551

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Aug 7, 2021

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


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.

@jrfnl jrfnl force-pushed the trac-46149/make-tests-compatible-with-phpunit8-9 branch from 02ec3eb to 7c51837 Compare August 7, 2021 00:47
jrfnl added 11 commits August 7, 2021 02:49
> 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()`.

Ref: 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.
... by renaming all declared fixture methods, and calls to parent versions of those fixture methods, from camelCase to snake_case.
…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

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.
... 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 chosing 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.
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
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.
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](https://github.com/johnkary/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:
* 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: https://phpunit.readthedocs.io/en/stable/risky-tests.html#test-execution-timeout
**`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.
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.
... 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:
* https://github.com/sebastianbergmann/phpunit/blob/8.4.3/ChangeLog-8.4.md#840---2019-10-04
* sebastianbergmann/phpunit#3775
As the minimum supported PHPUnit version has been upped to PHPUnit 5.7.21, these `@requires` tags are now redundant.
@jrfnl jrfnl force-pushed the trac-46149/make-tests-compatible-with-phpunit8-9 branch from 7c51837 to d2ae139 Compare August 7, 2021 00:49
@jrfnl
Copy link
Member Author

jrfnl commented Aug 7, 2021

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

@jrfnl jrfnl closed this Aug 7, 2021
@jrfnl jrfnl deleted the trac-46149/make-tests-compatible-with-phpunit8-9 branch August 7, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant