Introduce database focus area, rename JavaScript focus area to JS & CSS, and phase out Site Health focus area by mukeshpanchal27 · Pull Request #566 · WordPress/performance

mukeshpanchal27 · Oct 21, 2022

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 · 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.

felixarntz

@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.

mukeshpanchal27 · Nov 1, 2022

+		$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' );


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.

felixarntz

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

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

mukeshpanchal27 · 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?

felixarntz · 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

@mukeshpanchal27 LGTM! 🚀

mukeshpanchal27 added 9 commits October 20, 2022 16:42

Update latest focus areas

4c754ff

Update codeowners

3596ca9

Remove Site Health from codeowners

8aa96b3

Relocate existing modules

f8ef434

Update focus areas

7a45a1b

Update unit tests

e2e42a5

Update codeowners

a0206c2

Remove Site Health

0d8c71a

Update legacy module code

44ffdaa

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 added this to the 1.7.0 milestone Oct 21, 2022

mukeshpanchal27 requested a review from jjgrainger October 21, 2022 06:59

Update default-enabled-modules.php

7bdda84

mukeshpanchal27 marked this pull request as ready for review October 21, 2022 07:15

mukeshpanchal27 requested review from JustinyAhin, adamsilverstein, felixarntz, getsource, kirtangajjar, manuelRod, mitogh, spacedmonkey, tillkruss and westonruter as code owners October 21, 2022 07:15

tillkruss reviewed Oct 21, 2022

View reviewed changes

Comment thread .github/CODEOWNERS Outdated

felixarntz reviewed Oct 25, 2022

View reviewed changes

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

Merge branch 'trunk' into fix/554-update-focus-areas-codebase

87b9162

mukeshpanchal27 added 4 commits November 1, 2022 10:14

Update focus area to js-and-css

9e11a91

Revert module name changes

32a1ca3

Addressed code review

dc1e048

Add unit test

ce593f4

mukeshpanchal27 requested review from felixarntz and mehulkaklotar November 1, 2022 07:58

mukeshpanchal27 commented Nov 1, 2022

View reviewed changes

mehulkaklotar requested changes Nov 1, 2022

View reviewed changes

Comment thread tests/load-tests.php

Comment thread tests/load-tests.php

felixarntz approved these changes Nov 1, 2022

View reviewed changes

Comment thread .github/CODEOWNERS Outdated

Comment thread tests/load-tests.php Outdated

mukeshpanchal27 and others added 3 commits November 2, 2022 09:45

Add docblock for data_legacy_modules()

9a65537

Correct indentation

3e48687

Update tests/load-tests.php

e57024f

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

mukeshpanchal27 requested a review from mehulkaklotar November 2, 2022 05:12

mukeshpanchal27 mentioned this pull request Nov 2, 2022

Load the WP_Image_Editor_Imagick class if it doesn't exist #572

Merged

3 tasks

mehulkaklotar approved these changes Nov 2, 2022

View reviewed changes

Merge branch 'trunk' into fix/554-update-focus-areas-codebase

678a8a4

mukeshpanchal27 requested a review from felixarntz November 7, 2022 11:56

felixarntz approved these changes Nov 7, 2022

View reviewed changes

felixarntz merged commit 5ca3283 into trunk Nov 7, 2022

felixarntz deleted the fix/554-update-focus-areas-codebase branch November 7, 2022 19:03

		$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' );

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

mukeshpanchal27 commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

Checklist

Uh oh!

mukeshpanchal27 commented Oct 21, 2022

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 commented Nov 2, 2022

Uh oh!

felixarntz commented Nov 2, 2022

Uh oh!

mehulkaklotar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mukeshpanchal27 commented Oct 21, 2022 •

edited

Loading