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.

Sign up for GitHub

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

Jump to bottom

Improve PHP cross-version support and testing #166

Merged
merged 24 commits into from May 16, 2023
Merged

Improve PHP cross-version support and testing #166

merged 24 commits into from May 16, 2023

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented May 12, 2023 •
edited

Description of the Change

  • Fixes concurrency setting for GitHub Actions workflows
  • Uses test matrix to test from PHP 5.6 upwards
  • Moves dev dependencies requiring a higher PHP version to separate folder
    • Ensures that production dependencies can be installed and used on PHP 5.6+ without issues
  • Move phpunit tests to subfolder
    • Otherwise phpunit loads phpstan files too
  • Fixes errors reported by PHPCS about missing use statements

To-do

  • Investigate failing test on WP 6.0

Closes #164

Credits

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@spacedmonkey
Copy link
Member

See #165 for limiting php version 7.2+

swissspidy
swissspidy commented May 15, 2023
View reviewed changes
tests/phpunit/Checker/Preparations/Force_Single_Plugin_Preparation_Tests.php
Comment on lines +26 to 30
$preparation = new Force_Single_Plugin_Preparation( 'foo-plugin/foo-plugin.php' );

$this->expectException( 'Exception' );
$this->expectExceptionMessage( 'Invalid plugin akismet/akismet.php: Plugin file does not exist.' );
$this->expectExceptionMessage( 'Invalid plugin foo-plugin/foo-plugin.php: Plugin file does not exist.' );
$preparation->prepare();
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was inaccurate because in some environments like wp-env Akismet does exist, so there wouldn't be an exception.

It's more robust to use a slug that definitely does not exist.

@swissspidy swissspidy marked this pull request as ready for review May 15, 2023 15:23
@swissspidy swissspidy changed the title CI: Add test matrix for unit tests Improve PHP cross-version support and testing May 15, 2023
jjgrainger
jjgrainger approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@jjgrainger jjgrainger left a comment

Choose a reason for hiding this comment

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

Thanks @swissspidy looks great, approved

spacedmonkey
spacedmonkey reviewed May 16, 2023
View reviewed changes
build-cs/.gitignore Outdated Show resolved Hide resolved
@spacedmonkey spacedmonkey mentioned this pull request May 16, 2023
Closed
4 tasks
spacedmonkey
spacedmonkey reviewed May 16, 2023
View reviewed changes
composer.json
"lint": "phpcs --standard=phpcs.xml.dist",
"format": "phpcbf --standard=phpcs.xml.dist",
"lint": [
"composer --working-dir=build-cs install",
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this another script, maybe pre-cs

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 seems overkill do me, I don't see harm in this tiny bit of repetition. This way it's more connected with the next script and more readable, plus I don't see a reason why someone would want to run this command on its own.

spacedmonkey
spacedmonkey reviewed May 16, 2023
View reviewed changes
Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

This PR is nearly there, just a couple of questions and nitpics. After that, i would approve.

mukeshpanchal27
mukeshpanchal27 reviewed May 16, 2023
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks, @swissspidy. It looks good to me. I have left some minor suggestions (nitpicks). I noticed there is one pending task mentioned in the PR description. Will it be addressed as part of this PR or in a follow-up PR?

.github/workflows/php-test.yml Outdated Show resolved Hide resolved
.github/workflows/php-test.yml Outdated Show resolved Hide resolved
spacedmonkey
spacedmonkey reviewed May 16, 2023
View reviewed changes
composer.json Outdated Show resolved Hide resolved
spacedmonkey
spacedmonkey approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

Approved with some nicpics.

@swissspidy
Copy link
Member Author

It looks good to me. I have left some minor suggestions (nitpicks). I noticed there is one pending task mentioned in the PR description. Will it be addressed as part of this PR or in a follow-up PR?

Great question! IMHO that's better addressed in a follow-up issue / PR because it requires some investigation. It could be a bad test or an actual bug in the plugin.

@swissspidy swissspidy mentioned this pull request May 16, 2023
Merged
3 tasks
mukeshpanchal27
mukeshpanchal27 approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks, @swissspidy, for the update. The changes look good to me.

felixarntz
felixarntz approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Awesome work @swissspidy!

@felixarntz felixarntz merged commit 2867796 into trunk May 16, 2023
14 checks passed
@felixarntz felixarntz deleted the fix/github-actions-improvements branch May 16, 2023 15:53
@mukeshpanchal27
Copy link
Member

It looks good to me. I have left some minor suggestions (nitpicks). I noticed there is one pending task mentioned in the PR description. Will it be addressed as part of this PR or in a follow-up PR?

Great question! IMHO that's better addressed in a follow-up issue / PR because it requires some investigation. It could be a bad test or an actual bug in the plugin.

@swissspidy, can you please open a follow-up issue to ensure it doesn't get missed?

This was referenced May 17, 2023
Closed
Closed
@swissspidy
Copy link
Member Author

Opened #169

@swissspidy swissspidy mentioned this pull request May 17, 2023
Open
1 task
@mukeshpanchal27 mukeshpanchal27 mentioned this pull request Oct 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter left review comments

@spacedmonkey spacedmonkey spacedmonkey approved these changes

@jjgrainger jjgrainger jjgrainger approved these changes

@felixarntz felixarntz felixarntz approved these changes

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run PHPUnit unit tests again different versions of PHP
6 participants
@swissspidy @spacedmonkey @mukeshpanchal27 @westonruter @jjgrainger @felixarntz