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 placeholder support to code analyzer scan #37608

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

jonathansadowski
Copy link
Contributor

Submission Review Guidelines:

Changes proposed in this Pull Request:

This PR updates the code analyzer scan command to also support placeholders. In addition, this PR fixes an issue I discovered while writing the testing instructions for this PR. Prior to this change, only the first do_action or apply_filter changed in a file would be detected. If there were multiple actions or filters added to a single file, the subsequent ones would be ignored.

Partially addresses #35754

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Fork this repository, checkout locally, and make sure to run pnpm i.
  2. Merge these changes to your fork's trunk.
  3. Create a new branch (for example test/template-version) in your fork.
  4. Make changes to a file in plugins/woocommerce/templates, but do not update the @version tag in the file that you change.
  5. Commit previous change to your test branch and push back to your fork.
  6. Switch to the tools/code-analyzer directory.
  7. Run the following command: pnpm run analyzer scan templates "YOURBRANCH" "trunk" --since "8.0.0".
  8. Verify that you see a warning that the template may require a version bump.
  9. Update the file that you previous edited, and change the @version tag to 8.0.0. Commit and push changes.
  10. Again run pnpm run analyzer scan templates "YOURBRANCH" "trunk" --since "8.0.0".
  11. Verify that you do not see a warning. Instead you should see a notice that a version bump was found.
  12. Update the file that you previously edited again, this time changing the @version tag to x.x.x. Commit and push changes.
  13. Again run pnpm run analyzer scan templates "YOURBRANCH" "trunk" --since "8.0.0".
  14. You should again see a warning that a version bump may be required.
  15. Run pnpm run analyzer scan templates "YOURBRANCH" "trunk" --allowPlaceholder --since "8.0.0"
  16. Verify that you do not see a warning. Instead you should see a notice that a version bump placeholder was found.
  17. Make sure actions are enabled in your fork.
  18. Create a new PR to your fork (make sure the PR isn't to woocommerce/woocommerce) using your test branch. Verify that the pr-highlight-changes.yml workflow runs, and passes, and that there's a notice about a version bump placeholder being found.
  19. Create a new branch (for example test/hook-version) in your fork.
  20. Add a do_action or apply_filter call to a php file in the woocommerce plugin. Include the docblock and @since 8.0.0. For example:
/**
 * Does a thing.
 *
 * @since 8.0.0
 */
do_action( 'wc_some_thing' );
  1. Commit and push the change.
  2. Run pnpm run analyzer scan hooks "YOURBRANCH" "trunk" --since "8.0.0".
  3. Verify that the hook is detected with the version of 8.0.0.
  4. Add a do_action or apply_filter call to a php file in the woocommerce plugin. Include the docblock and @since x.x.x. You may want to use the same file you used in the previous step to verify that this PR also addresses the issue where previously only a single hook was detected in each file. For example:
/**
 * Does a different thing.
 *
 * @since x.x.x
 */
do_action( 'wc_some_thing_else' );
  1. Commit and push the change.
  2. Run pnpm run analyzer scan hooks "YOURBRANCH" "trunk" --since "8.0.0".
  3. You shouldn't see the hook with @since x.x.x because the --allowPlaceholder flag was not included.
  4. Repeat the previous command adding --allowPlaceholder. You should see the placeholder hook.

@jonathansadowski jonathansadowski requested review from a team and psealock and removed request for a team April 6, 2023 18:53
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

Hi @psealock,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

Hi @psealock,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

Test Results Summary

Commit SHA: 1294de6

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 51s
E2E Tests1860010019613m 25s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

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

I ran into the error on step 22, so haven't proceeded. This is looking great so far.

One question I have when x.x.x is used.

You shouldn't see the hook with @SInCE x.x.x because the --allowPlaceholder flag was not included.

Should it error out when --allowPlaceholder isn't used? I'm not sure the answer because I suppose it depends how the tool is supposed to be used. Is there a time when we want to ignore an introduced x.x.x? Maybe we should not have the flag at all and surface the change regardless?

filePath,
name,
hookType,
description,
changeType,
version,
ghLink,
isPlaceholder,
Copy link
Contributor

Choose a reason for hiding this comment

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

On Step 22 I get an error here when running pnpm run analyzer scan hooks "test/hook-version" "trunk" --since "8.0.0".

ReferenceError: isPlaceholder is not defined
    at scanForHookChanges (/Users/paulsealock/Workspace/psealock-woocommerce/tools/code-analyzer/src/lib/hook-changes.ts:115:7)
    at async scanChangesForHooks (/Users/paulsealock/Workspace/psealock-woocommerce/tools/code-analyzer/src/lib/scan-changes.ts:87:22)
    at async Command.<anonymous> (/Users/paulsealock/Workspace/psealock-woocommerce/tools/code-analyzer/src/commands/analyzer/analyzer-scan.ts:83:26)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants