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

Add Plugin Check workflow #20808

Open
wants to merge 20 commits into
base: trunk
Choose a base branch
from

Conversation

adamsilverstein
Copy link
Contributor

@adamsilverstein adamsilverstein commented Oct 30, 2023

Fixes #20809

Context

Run the plugin checking tool for each commit, annotating code with any issues.

Summary

This PR adds a GitHub action that will run whenever a Pull Request is opened or updated against your main branch. The results of the plugin check will be added as annotations to the code section of the PR, indicating the exact issue raised at the line it was found.
This initial implementation enables all checks available in the Plugin Check tool. These can be restricted to specific checks or check categories using the checks and categories attribute respectively.

This PR can be summarized in the following changelog entry:

  • New GitHub action: run the plugin checking tool for each commit, annotating code with any issues.

Relevant technical choices:

  • Using the Plugin Test tool will ensure code remains up to date with the latest best practices. Leveraging the action used in the PR ensures a simple implementation that will use the current core check tool.

Test instructions

The results of running the plugin check should be available on this PR.

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Block/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

QA can test this PR by following these steps:

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities.
  • During testing, I had activated all plugins that Yoast SEO provides integrations for.
  • I have added unit tests to verify the code works as intended.
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.
  • I have checked that the base branch is correctly set.

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label.
  • I have added my hours to the WBSO document.

Fixes #

@adamsilverstein adamsilverstein marked this pull request as ready for review October 30, 2023 17:11
@adamsilverstein
Copy link
Contributor Author

I see the initial check raised a bunch of flags for hidden files that I'm sure you will want to keep (see files tab), checking to see how best to skip "Hidden files are not permitted." warnings

@adamsilverstein
Copy link
Contributor Author

Note: looks like the action will need some slight updates to work correctly from the build folder which is what I'm trying to get working with the last set of commits. I will update here with progress!

@adamsilverstein
Copy link
Contributor Author

Note: looks like the action will need some slight updates to work correctly from the build folder which is what I'm trying to get working with the last set of commits. I will update here with progress!

I was able to get the build work by copying over some steps from the deploy.yml action (the copied part could potentially be split out to avoid duplicating). The plugin now builds to the artifact directory and the plugin cehck runs against the built version.

Running the plugin check against the artifact directory seems to raise some potentially interesting flags, let me know what you think.

@westonruter
Copy link
Contributor

@swissspidy Shouldn't the plugin check action be showing a failure rather than ✅?

@swissspidy

This comment was marked as resolved.

adamsilverstein and others added 2 commits November 6, 2023 15:46
Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com>
@adamsilverstein
Copy link
Contributor Author

There are three remaining errors to review (and some warnings):
Error: Setting suppress_filters to true is prohibited.
Error: The Stable Tag in your readme file does not match the version in your main plugin file.
Error: Do not use Localhost/127.0.0.1 in your code.

trademarks
late_escaping
plugin_updater
plugin_review_phpcs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
plugin_review_phpcs
plugin_review_phpcs
plugin_readme

Addresses the The Stable Tag in your readme file does not match the version in your main plugin file. warning.

Given that this is a development repo for the next release, the stable tag will never match, so it doesn't really make sense to check it.

This also skips some other checks within Plugin_Readme_Check (such as for valid license), but for an established plugin like this I think it's fine.

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.

Add Plugin Check workflow
4 participants