Skip to content

Improve PHP cross-version support and testing#166

Merged
felixarntz merged 24 commits intotrunkfrom
fix/github-actions-improvements
May 16, 2023
Merged

Improve PHP cross-version support and testing#166
felixarntz merged 24 commits intotrunkfrom
fix/github-actions-improvements

Conversation

@swissspidy
Copy link
Copy Markdown
Member

@swissspidy swissspidy commented May 12, 2023 •
edited
Loading

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
Copy Markdown
Member

spacedmonkey commented May 12, 2023

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
Copy Markdown
Member Author

@swissspidy swissspidy May 15, 2023

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
Copy Markdown
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
spacedmonkey
spacedmonkey reviewed May 16, 2023
View reviewed changes
@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
Copy Markdown
Member

@spacedmonkey spacedmonkey May 16, 2023

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
Copy Markdown
Member Author

@swissspidy swissspidy May 16, 2023

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
Copy Markdown
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
Copy Markdown
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?

spacedmonkey
spacedmonkey reviewed May 16, 2023
View reviewed changes
spacedmonkey
spacedmonkey approved these changes May 16, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

swissspidy commented May 16, 2023

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
Copy Markdown
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
Copy Markdown
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
@felixarntz felixarntz deleted the fix/github-actions-improvements branch May 16, 2023 15:53
westonruter
westonruter reviewed May 16, 2023
View reviewed changes
@mukeshpanchal27
Copy link
Copy Markdown
Member

mukeshpanchal27 commented May 17, 2023

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
Copy Markdown
Member Author

swissspidy commented May 17, 2023

Opened #169

@swissspidy swissspidy mentioned this pull request May 17, 2023
Closed
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