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: update test suite to allow for running the tests on PHPUnit 10 #817

Merged
merged 12 commits into from Jul 24, 2023

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jun 26, 2023

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Documentation improvement
  • Code quality improvement

Context

Update the test suite to allow for running the tests on PHPUnit 10.

Detailed Description

Composer: update the PHPUnit Polyfills

A PHPUnit 10 compatible version of the PHPUnit Polyfills has been released, so let's start using it.

Ref:

Transport/BaseTestCase::dataSNISupport(): remove unused and unnecessary method parameter

Data providers cannot take arguments, so this $options parameter is useless and should never have been declared. PHPUnit 10 will flag it, so let's remove it.

TestCase::textArrayToDataprovider(): make static

This method is used exclusively by data providers and as of PHPUnit 10, data providers should be static methods, so this method needs to be static too.

TestCase::transportProvider(): make static

This method is used exclusively as a data providers and as of PHPUnit 10, data providers should be static methods, so this method needs to be static too.

TestCase: split httpbin() method and make Session\ConstructorTest::dataValidUrl() method static

As of PHPUnit 10, data providers are (again) expected to be static methods.

One of the data providers is using the httpbin() method, but as the httpbin() method needs $this to handle the test skipping, the method cannot be made static.

This commit splits the httpbin() method into three methods:

  • A skipOnUnavailableHttpbinHost() method which contains the test skip check.
  • A static getHttpbinUrl() method which builds the URL.
  • The original httpbin() method which now calls the two new methods to maintain the same functionality as before.

By splitting the method in this way, we prevent having to change all method calls to the httpbin() method throughout the test suite and can still fix the data provider method using the httpbin() method to not use $this (which allows to make it static).

Tests: make dataproviders static

As of PHPUnit 10, data providers are (again) expected to be static methods.

This updates the test suite to respect that.

Includes removing the use of $this from select data providers.

Tests: replace assertObjectHasAttribute() with assertObjectHasProperty()

The assertObjectHasAttribute() method has been deprecated in PHPUnit 9 and removed in PHPUnit 10.

PHPUnit 10.1 introduces a replacement assertObjectHasProperty() method, which is polyfilled by the PHPUnit Polyfills.

This switches all uses of the old method to the new method name.

Tests: work around for MockBuilder::setMethods() removal

Please note:

  • The getMockBuilder() method is also deprecated now, so replacing with another method call on the MockBuilder class will only work in the short/medium term and in a future iteration, this will have to be refactored again.
  • The addMethods() method has been (soft) deprecated in PHPUnit 10.1 and is expected to be removed in PHPUnit 12.

Also see #814

Refs:

Tests: work around for removal of expectDeprecation et al

PHPUnit will no longer stop a test on deprecations/notices/warnings.
This also means that the PHPUnit native way to expect deprecations/notices/warnings etc is no longer supported.

This implements a work-around for the limited cases in which that functionality was used in this test suite.

Refs:

Curl/Fsockopen: add some extra defensive coding

The error_get_last() function can return null if no error occurred, in which case, the throw new Exception() statement will run into two new errors:

  • Trying to access array offset on value of type null for accessing $error['message'].
  • ... which then leads to a Exception::__construct(): Passing null to parameter #1 ($message) of type string is deprecated.

This commit adds some defensive coding to handle the hypothetical situation that error_get_last() would return null when a file could not be opened.

Note: this is actually a bug in PHPUnit 10 which breaks error_get_last().
We should be able to remove the extra defensive coding once the upstream bug has been fixed.

Upstream bug report: sebastianbergmann/phpunit#5428

PHPUnit: add separate configuration for PHPUnit 10

PHPUnit 10 makes significant changes to the configuration file.

Most notably:

  • In PHPUnit 9.3, the manner of specifying the code coverage configuration has changed.
  • In PHPUnit 10.0, a significant number of attributes of the <phpunit> element were removed or renamed.
  • In PHPUnit 10.1, the manner of specifying code coverage has changed again.

While there is a --migrate-configuration option available in PHPUnit, that doesn't get us a well enough converted configuration file, so instead use a separate phpunit10.xml.dist file for running the tests on PHPUnit 10 with an optimal configuration.

Includes:

  • Ignoring the new file for package archives.
  • Adding separate scripts to the composer.json file to make how to run the tests on PHPUnit 10 more obvious for contributors.
  • Updating the GH Actions workflows to take the new configuration file into account when appropriate.

jrfnl added 11 commits June 26, 2023 13:41
A PHPUnit 10 compatible version of the PHPUnit Polyfills has been released, so let's start using it.

Ref:
* https://github.com/Yoast/PHPUnit-Polyfills/releases/tag/2.0.0
…ry method parameter

Data providers cannot take arguments, so this `$options` parameter is useless and should never have been declared. PHPUnit 10 will flag it, so let's remove it.
This method is used exclusively by data providers and as of PHPUnit 10, data providers should be static methods, so this method needs to be `static` too.
This method is used exclusively as a data providers and as of PHPUnit 10, data providers should be static methods, so this method needs to be `static` too.
…dataValidUrl() method static

As of PHPUnit 10, data providers are (again) expected to be `static` methods.

One of the data providers is using the `httpbin()` method, but as the `httpbin()` method needs `$this` to handle the test skipping, the method cannot be made static.

This commit splits the `httpbin()` method into three methods:
* A `skipOnUnavailableHttpbinHost()` method which contains the test skip check.
* A `static` `getHttpbinUrl()` method which builds the URL.
* The original `httpbin()` method which now calls the two new methods to maintain the same functionality as before.

By splitting the method in this way, we prevent having to change all method calls to the `httpbin()` method throughout the test suite and can still fix the data provider method using the `httpbin()` method to not use `$this` (which allows to make it `static`).
As of PHPUnit 10, data providers are (again) expected to be `static` methods.

This updates the test suite to respect that.

Includes removing the use of `$this` from select data providers.
…erty()`

The `assertObjectHasAttribute()` method has been deprecated in PHPUnit 9 and removed in PHPUnit 10.

PHPUnit 10.1 introduces a replacement `assertObjectHasProperty()` method, which is polyfilled by the PHPUnit Polyfills.

This switches all uses of the old method to the new method name.
Please note:
* The `getMockBuilder()` method is also deprecated now, so replacing with another method call on the `MockBuilder` class will only work in the short/medium term and in a future iteration, this will have to be refactored again.
* The `addMethods()` method has been (soft) deprecated in PHPUnit 10.1 and is expected to be removed in PHPUnit 12.

Also see 814

Refs:
* sebastianbergmann/phpunit 3687#issuecomment-492537584
* sebastianbergmann/phpunit 3770
* sebastianbergmann/phpunit 3769
* sebastianbergmann/phpunit 4775
PHPUnit will no longer stop a test on deprecations/notices/warnings.
This also means that the PHPUnit native way to expect deprecations/notices/warnings etc is no longer supported.

This implements a work-around for the limited cases in which that functionality was used in this test suite.

Refs:
* https://phpunit.de/announcements/phpunit-10.html
* sebastianbergmann/phpunit 5062
The `error_get_last()` function can return `null` if no error occurred, in which case, the `throw new Exception()` statement will run into two new errors:
* `Trying to access array offset on value of type null` for accessing `$error['message']`.
* ... which then leads to a `Exception::__construct(): Passing null to parameter #1 ($message) of type string is deprecated`.

This commit adds some defensive coding to handle the hypothetical situation that `error_get_last()` would return `null` when a file could not be opened.

Note: this is actually a bug in PHPUnit 10 which breaks `error_get_last()`.
We should be able to remove the extra defensive coding once the upstream bug has been fixed.

Upstream bug report: sebastianbergmann/phpunit#5428
PHPUnit 10 makes significant changes to the configuration file.

Most notably:
* In PHPUnit 9.3, the manner of specifying the code coverage configuration has changed.
* In PHPUnit 10.0, a significant number of attributes of the `<phpunit>` element were removed or renamed.
* In PHPUnit 10.1, the manner of specifying code coverage has changed again.

While there is a `--migrate-configuration` option available in PHPUnit, that doesn't get us a well enough converted configuration file, so instead use a separate `phpunit10.xml.dist` file for running the tests on PHPUnit 10 with an optimal configuration.

Includes:
* Ignoring the new file for package archives.
* Adding separate scripts to the `composer.json` file to make how to run the tests on PHPUnit 10 more obvious for contributors.
* Updating the GH Actions workflows to take the new configuration file into account when appropriate.
@jrfnl jrfnl added this to the 2.1.0 milestone Jun 26, 2023
@jrfnl jrfnl requested a review from schlessera June 26, 2023 13:42
@jrfnl
Copy link
Member Author

jrfnl commented Jun 26, 2023

CodeCov is drunk ;-)

... as it is now less intuitive to run the unit tests via the Composer scripts, so improve the discoverability of the different scripts by adding descriptions for each script.
@schlessera
Copy link
Member

Re. failures:

@schlessera schlessera merged commit cb9da04 into develop Jul 24, 2023
27 of 29 checks passed
@schlessera schlessera deleted the feature/upgrade-phpunit-polyfills-phpunit-10 branch July 24, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants