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

Fix failing PHPUnit tests #7419

Merged
merged 7 commits into from
Jan 25, 2023
Merged

Fix failing PHPUnit tests #7419

merged 7 commits into from
Jan 25, 2023

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Jan 24, 2023

Summary

  • Add a step to keep the PHPUnit version to 9.3 on PHP 7.3, 7.4, and 8.0 in the workflow.
  • Fix embeds URL for Tumblr.

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2023

Plugin builds for 3be7426 are ready 🛎️!

@thelovekesh thelovekesh marked this pull request as draft January 24, 2023 10:17
@thelovekesh
Copy link
Collaborator Author

@westonruter While updating the phpunit version in Build, test & measure workflow, the PHPUnit version gets updated to 9.5.28 which is PHP 8.1 compatible, and throws an error like Error: Call to undefined function enum_exists().

enum_exists() gets introduced in PHP 8.1.

To avoid this error we will need to setup PHPUnit v9.3 here instead of letting it decide by the composer with yoast/phpunit-polyfills.

composer update --ignore-platform-reqs --no-interaction --no-scripts yoast/phpunit-polyfills --with-dependencies

@westonruter
Copy link
Member

the PHPUnit version gets updated to 9.5.28 which is PHP 8.1 compatible

In other words, it requires PHP 8.1 as a minimum?

SGTM regardless.

@thelovekesh thelovekesh force-pushed the fix/php-tests branch 2 times, most recently from 7d3fc3f to 721e15a Compare January 25, 2023 05:39
@thelovekesh thelovekesh marked this pull request as ready for review January 25, 2023 05:51
@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #7419 (7507dc7) into develop (7f9b28b) will increase coverage by 12.07%.
The diff coverage is n/a.

❗ Current head 7507dc7 differs from pull request most recent head 3be7426. Consider uploading reports for the commit 3be7426 to get more accurate results

Impacted file tree graph

@@              Coverage Diff               @@
##             develop    #7419       +/-   ##
==============================================
+ Coverage      64.76%   76.83%   +12.07%     
- Complexity         0     6864     +6864     
==============================================
  Files             67      281      +214     
  Lines           1189    27232    +26043     
==============================================
+ Hits             770    20925    +20155     
- Misses           419     6307     +5888     
Flag Coverage Δ
javascript 64.76% <ø> (ø)
php 77.39% <ø> (?)
unit 77.39% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...udes/sanitizers/class-amp-pwa-script-sanitizer.php 100.00% <0.00%> (ø)
.../Transformer/AmpSchemaOrgMetadataConfiguration.php 0.00% <0.00%> (ø)
src/Cli/TransformerCommand.php 0.00% <0.00%> (ø)
src/Optimizer/AmpWPConfiguration.php 100.00% <0.00%> (ø)
src/Exception/InvalidEventProperties.php 96.29% <0.00%> (ø)
src/BackgroundTask/RecurringBackgroundTask.php 100.00% <0.00%> (ø)
includes/sanitizers/class-amp-form-sanitizer.php 18.42% <0.00%> (ø)
includes/embeds/class-amp-meetup-embed-handler.php 22.22% <0.00%> (ø)
...des/embeds/class-amp-crowdsignal-embed-handler.php 75.00% <0.00%> (ø)
includes/sanitizers/class-amp-iframe-sanitizer.php 96.40% <0.00%> (ø)
... and 204 more

@thelovekesh
Copy link
Collaborator Author

@westonruter This PR need to be merged before proceeding to any other PR that runs the PHPUnit tests workflow.

elif [[ $PHP_VERSION == "7.3" || $PHP_VERSION == "7.4" || $PHP_VERSION == "8.0" ]]; then
echo "Installing PHPUnit 9.3"
composer require --dev --ignore-platform-reqs phpunit/phpunit:"=9.3" --with-dependencies
elif [[ $WP_VERSION == "latest" || $WP_VERSION == "trunk" || $PHP_VERSION == "8.1" || $PHP_VERSION == "8.2" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

What about when PHP 8.3 comes out?

Copy link
Member

Choose a reason for hiding this comment

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

I guess this will depend on us updating the matrix, so nevermind.

@westonruter westonruter added this to the v2.4 milestone Jan 25, 2023
elif [[ $WP_VERSION == "latest" || $WP_VERSION == "trunk" || $PHP_VERSION == "7.4" ]]; then
elif [[ $PHP_VERSION == "7.3" || $PHP_VERSION == "7.4" || $PHP_VERSION == "8.0" ]]; then
echo "Installing PHPUnit 9.3"
composer require --dev --ignore-platform-reqs phpunit/phpunit:"=9.3" --with-dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Should this rather be:

Suggested change
composer require --dev --ignore-platform-reqs phpunit/phpunit:"=9.3" --with-dependencies
composer require --dev --ignore-platform-reqs phpunit/phpunit:"=9.3.*" --with-dependencies

As this would account for bug fixes for that branch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch. It should rather be composer require --dev --ignore-platform-reqs phpunit/phpunit:"9.3.*" --with-dependencies as "=9.3.*" is an invalid version string.

@westonruter westonruter enabled auto-merge January 25, 2023 19:18
@westonruter westonruter merged commit 2462614 into develop Jan 25, 2023
@westonruter westonruter deleted the fix/php-tests branch January 25, 2023 19:26
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants