Skip to content

Introduce database focus area, rename JavaScript focus area to JS & CSS, and phase out Site Health focus area #566

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
Merged
merged 19 commits into from
Nov 7, 2022
Merged

Introduce database focus area, rename JavaScript focus area to JS & CSS, and phase out Site Health focus area #566

merged 19 commits into from
Nov 7, 2022

Conversation

@mukeshpanchal27
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 commented Oct 21, 2022 •
edited
Loading

Summary

PR that updates focus areas in the codebase

  • Update perflab_get_focus_areas() to return the latest focus areas
  • Rename javascript focus area to css-and-js (CSS & JS), including the directory in modules
  • Move site-health modules into their new focus area directories
  • Implement logic in perflab_get_module_settings() to migrate all old ("legacy") module slugs to the new ones (e.g. if you have site-health/audit-autoloaded-options in the DB, it should become database/audit-autoloaded-options)

Fixes #554

Relevant technical choices

To fix the unit test you have to update default-enabled-modules.php using https://make.wordpress.org/performance/handbook/performance-lab/releasing-the-plugin/#update-list-of-default-enabled-modules.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure labels Oct 21, 2022
@mukeshpanchal27 mukeshpanchal27 added this to the 1.7.0 milestone Oct 21, 2022
@mukeshpanchal27
Copy link
Member Author

mukeshpanchal27 commented Oct 21, 2022

To fix the unit test related to default-enabled-modules.php I needs to update 7bdda84 file manually. You can update it using https://make.wordpress.org/performance/handbook/performance-lab/releasing-the-plugin/#update-list-of-default-enabled-modules.

@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review October 21, 2022 07:15
tillkruss
tillkruss reviewed Oct 21, 2022
View reviewed changes
felixarntz
felixarntz reviewed Oct 25, 2022
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.

@mukeshpanchal27 This looks solid overall, I mostly have minor feedback on updating the css-and-js focus area to js-and-css, and a few things to improve. The most important thing missing would be to add test coverage for the module settings migration logic.

@felixarntz felixarntz changed the title Updates focus areas in the codebase Introduce database focus area, rename JavaScript focus area to JS & CSS, and phase out Site Health focus area Oct 25, 2022
mukeshpanchal27
mukeshpanchal27 commented Nov 1, 2022
View reviewed changes
tests/load-tests.php
Comment on lines +113 to +114
$this->assertArrayNotHasKey( $legacy_module_slug, $settings, 'The settings do not contain the old legacy module slug in the database' );
$this->assertArrayHasKey( $current_module_slug, $settings, 'The settings contain an updated module slug in the database' );
Copy link
Member Author

@mukeshpanchal27 mukeshpanchal27 Nov 1, 2022

Choose a reason for hiding this comment

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

As good habit like WP core,

Test methods with multiple assertions need a message parameter for each assertion, per Core Handbook - Writing PHPUnit Tests - Using Assertions.

mehulkaklotar
mehulkaklotar requested changes Nov 1, 2022
View reviewed changes
felixarntz
felixarntz approved these changes Nov 1, 2022
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.

@mukeshpanchal27 Looks great! I'll LGTM this already, but please address the two minor remaining quirks (there strictly not a blocker though).

mukeshpanchal27 and others added 3 commits November 2, 2022 09:45
@mukeshpanchal27
Add docblock for data_legacy_modules()
9a65537
@mukeshpanchal27
Correct indentation
3e48687
@mukeshpanchal27 @felixarntz
Update tests/load-tests.php
e57024f 
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@mukeshpanchal27
Copy link
Member Author

mukeshpanchal27 commented Nov 2, 2022

@felixarntz I have addressed the feedback. Github shows the unit test related error for Fatal error: Uncaught Error: Class 'WP_Image_Editor_Imagick' not found in /var/www/html/wp-content/plugins/performance/tests/testdata/modules/images/webp-uploads/class-wp-image-doesnt-support-webp.php:14 https://github.com/WordPress/performance/actions/runs/3374928930/jobs/5601071400 I think it was introduce because of latest WP update. I think all new PR changes fails now.

@adamsilverstein Do we needs to open new issue?

@mukeshpanchal27 mukeshpanchal27 mentioned this pull request Nov 2, 2022
Merged
3 tasks
@felixarntz
Copy link
Member

felixarntz commented Nov 2, 2022

@mukeshpanchal27 Thanks! Looks like there's already #572, let's prioritize getting that through so that then we can refresh this PR here to make tests pass again.

mehulkaklotar
mehulkaklotar approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@mehulkaklotar mehulkaklotar left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 LGTM! 🚀

@felixarntz felixarntz merged commit 5ca3283 into trunk Nov 7, 2022
@felixarntz felixarntz deleted the fix/554-update-focus-areas-codebase branch November 7, 2022 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tillkruss tillkruss tillkruss left review comments

@felixarntz felixarntz felixarntz approved these changes

@mehulkaklotar mehulkaklotar mehulkaklotar approved these changes

@jjgrainger jjgrainger Awaiting requested review from jjgrainger

@manuelRod manuelRod Awaiting requested review from manuelRod

@westonruter westonruter Awaiting requested review from westonruter

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein

@kirtangajjar kirtangajjar Awaiting requested review from kirtangajjar

@mitogh mitogh Awaiting requested review from mitogh

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey

@getsource getsource Awaiting requested review from getsource

@JustinyAhin JustinyAhin Awaiting requested review from JustinyAhin

Assignees

No one assigned

Labels

Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature

Projects

None yet

Milestone

1.7.0

Development

Successfully merging this pull request may close these issues.

Revamp available focus areas

5 participants

@mukeshpanchal27 @felixarntz @tillkruss @mehulkaklotar