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

Update phpunit for all tests and add testing for PHP 8.1 #1758

Closed
3 of 4 tasks
mikkamp opened this issue Nov 10, 2022 · 2 comments · Fixed by #1989
Closed
3 of 4 tasks

Update phpunit for all tests and add testing for PHP 8.1 #1758

mikkamp opened this issue Nov 10, 2022 · 2 comments · Fixed by #1989
Labels
type: enhancement The issue is a request for an enhancement. type: technical debt This issue/PR represents/solves the technical debt of the project.

Comments

@mikkamp
Copy link
Contributor

mikkamp commented Nov 10, 2022

User story

Previously we've stuck with phpunit 7.5.20 because we needed to remain compatible with older WordPress / WooCommerce tests. We use the yoast/phpunit-polyfills package to still be able to take advantage of newer functions introduced for unit testing.

However for testing with PHP 8.0 we use a workaround and upgrade to phpunit 9.5, with the limitation that it was unable to test with a WP version older than 5.9. Now that WP 6.1 is released, if we support WP versions L-2 we can run unit testing for 5.9 - 6.1 without needing to check older versions. This means we can upgrade the phpunit for all tests and no longer need the workaround.

Doing so will also enable us to setup unit testing for PHP 8.1, which could help identify issues like the one reported in #1745

Technical

  • Update the phpunit composer package
  • Remove workaround for PHP 8.0
  • Add phpunit testing for PHP 8.1
  • Update all documentation referring to unit testing
@mikkamp mikkamp added type: enhancement The issue is a request for an enhancement. type: technical debt This issue/PR represents/solves the technical debt of the project. labels Nov 10, 2022
@mikkamp
Copy link
Contributor Author

mikkamp commented Nov 10, 2022

I did some testing, and phpunit can be updated without any issues. However running phpunit with PHP 8.1 still poses some problems. There are some warnings coming from the rmccue library:

Deprecated: Return type of Requests_Cookie_Jar::offsetSet($key, $value) should either be compatible with ArrayAccess::offsetSet(mixed $offset, mixed $value): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in wp-content/plugins/google-listings-and-ads/vendor/rmccue/requests/library/Requests/Cookie/Jar.php on line 89
PHP Deprecated:  Return type of Requests_Cookie_Jar::offsetUnset($key) should either be compatible with ArrayAccess::offsetUnset(mixed $offset): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in wp-content/plugins/google-listings-and-ads/vendor/rmccue/requests/library/Requests/Cookie/Jar.php on line 102

Deprecated: Return type of Requests_Cookie_Jar::offsetUnset($key) should either be compatible with ArrayAccess::offsetUnset(mixed $offset): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in wp-content/plugins/google-listings-and-ads/vendor/rmccue/requests/library/Requests/Cookie/Jar.php on line 102
PHP Deprecated:  Return type of Requests_Cookie_Jar::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in wp-content/plugins/google-listings-and-ads/vendor/rmccue/requests/library/Requests/Cookie/Jar.php on line 111

It's a dependency of the package wp-cli/i18n-command which according to this issue doesn't have a resolution yet (as it requires support for older WP versions).

Even after an update we still receive the deprecated warnings:

composer update wp-cli/i18n-command --with-all-dependencies
  - Upgrading gettext/gettext (v4.8.6 => v4.8.7)
  - Upgrading gettext/languages (2.9.0 => 2.10.0)
  - Upgrading mck89/peast (v1.14.0 => v1.15.0)
  - Upgrading mustache/mustache (v2.14.1 => v2.14.2)
  - Upgrading symfony/finder (v5.4.8 => v5.4.11)
  - Upgrading symfony/polyfill-php80 (v1.26.0 => v1.27.0)
  - Upgrading wp-cli/i18n-command (v2.3.0 => v2.4.0)
  - Upgrading wp-cli/php-cli-tools (v0.11.13 => v0.11.16)
  - Upgrading wp-cli/wp-cli (v2.6.0 => v2.7.1)

Note that these are warnings so we can still complete testing, but it fails with some other errors which need to be looked at:

There were 2 failures:

1) Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\MerchantCenter\AccountServiceTest::test_existing_account
Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsInterface::get_merchant_id(): int was not expected to be called more than once.

wp-content/plugins/google-listings-and-ads/src/MerchantCenter/AccountService.php:484
wp-content/plugins/google-listings-and-ads/src/MerchantCenter/AccountService.php:140
wp-content/plugins/google-listings-and-ads/tests/Unit/MerchantCenter/AccountServiceTest.php:193

2) Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\MerchantCenter\AccountServiceTest::test_existing_account_api_error
Failed asserting that exception message 'rtrim(): Passing null to parameter #1 ($string) of type string is deprecated' contains 'error'.

@mikkamp
Copy link
Contributor Author

mikkamp commented Feb 3, 2023

Another blocker to running unit tests for PHP 8.1 is the following deprecation warning in the google/apiclient package: googleapis/google-api-php-client#2372 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement The issue is a request for an enhancement. type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant