close
The Wayback Machine - https://web.archive.org/web/20221207193135/https://github.com/WordPress/performance/pull/263
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 Site Health check for Full Page Cache usage #263

Merged

Conversation

manuelRod
Copy link
Contributor

@manuelRod manuelRod commented Mar 24, 2022

Summary

Fixes #220
Trac #54423

Relevant technical choices

As @westonruter proposed, I've adapted and migrated the full page caching Site Health test from AMP to the performance plugin.

Checklist

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

@manuelRod manuelRod marked this pull request as ready for review Mar 30, 2022
@camaran
Copy link

camaran commented Mar 31, 2022

please add
'x-aruba-cache' => $cache_hit_callback,
for new aruba hispeed cache service

@bethanylang
Copy link
Contributor

bethanylang commented Apr 1, 2022

@manuelRod Since I'm working on updating the language on the other Site Health reports, I took a look at the copy for this one as well and left some thoughts. Let me know if you have any questions!

@manuelRod
Copy link
Contributor Author

manuelRod commented Apr 4, 2022

Thanks for that @bethanylang, where I can find those thoughts?

Copy link
Member

@westonruter westonruter left a comment

Overall looks good to me, but it looked good to me in the AMP plugin when I merged it, so I'm probably not the best to review for merging here.

modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
@manuelRod manuelRod added [Focus] Site Health LEGACY LEGACY – Issues related to the Site Health focus area [Type] Feature A new feature within an existing module labels Apr 12, 2022
@manuelRod manuelRod added this to the 1.0.0 milestone Apr 12, 2022
@manuelRod
Copy link
Contributor Author

manuelRod commented Apr 12, 2022

please add 'x-aruba-cache' => $cache_hit_callback, for new aruba hispeed cache service

added

Copy link
Member

@felixarntz felixarntz left a comment

@manuelRod Overall the code here looks solid. I left a few minor things for some cleanup, particularly I'd suggest to remove two of the functions that are only used in one place and actually make the code more complex to follow rather than less because of splitting how the $result is generated.

modules/site-health/audit-full-page-cache/helper.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/helper.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/helper.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/helper.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/helper.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
@felixarntz
Copy link
Member

felixarntz commented Apr 13, 2022

@bethanylang Would be great to get your review here specifically on the copy in the Site Health check.

@felixarntz felixarntz requested a review from bethanylang Apr 13, 2022
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
Copy link
Member

@felixarntz felixarntz left a comment

@manuelRod This looks solid and should be close to merge, I mostly have some nit-picks around documentation. Please make sure to update all version references for new code in this PR to n.e.x.t, per our guidelines.

Last but not least, based on #310, please refresh this PR against latest trunk and add the corresponding entries for this new module to the CODEOWNERS file, with at least yourself as the module owner.

@manuelRod manuelRod requested a review from JustinyAhin as a code owner May 4, 2022
.github/CODEOWNERS Outdated Show resolved Hide resolved
@felixarntz
Copy link
Member

felixarntz commented May 10, 2022

@manuelRod @westonruter Given that this is an entirely new module and the 1.1.0 release is about to be finalized with the release coming next week, I think with the remaining iterations to make this won't give us enough time for testing prior to the release, so I think this would be better to include in the following 1.2.0 release. Let's make sure we finalize the PR soon so that it can be merged into trunk shortly after 1.1.0, to have time for testing and potential further iterations.

@felixarntz felixarntz modified the milestones: 1.1.0, 1.2.0 May 10, 2022
@akshitsethi
Copy link
Contributor

akshitsethi commented May 25, 2022

I tested the module and so far it looks good to me. Multiple scenarios (looback request failed, without cache, with cache plugin) were tested and I've attached the screenshot for each below:

Loopback failure

Screenshot 2022-05-25 at 4 43 40 PM

Without cache

Screenshot 2022-05-25 at 5 00 36 PM

Cache enabled

Screenshot 2022-05-25 at 5 05 32 PM

I'll test some more scenarios and update it over here.

@felixarntz felixarntz added the [Module] Audit Full Page Cache Issues for the Audit Full Page Cache Health Check module label May 25, 2022
Copy link
Member

@felixarntz felixarntz left a comment

@manuelRod Looks good to me, great stuff!

I left a few minor points on documentation and indentation, please address these. None of these are too critical though, so I'm already going to mark this as approved from my end.

modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-full-page-cache/load.php Outdated Show resolved Hide resolved
@felixarntz felixarntz requested a review from bethanylang May 26, 2022
manuelRod and others added 4 commits May 30, 2022
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Copy link
Member

@felixarntz felixarntz left a comment

Great, thanks @manuelRod!

@bethanylang You previously requested changes regarding the copy. Can you please give this another pass and approve once it's good from your end?

Copy link
Contributor

@bethanylang bethanylang left a comment

Language LGTM!

@bethanylang
Copy link
Contributor

bethanylang commented Jun 3, 2022

@felixarntz @manuelRod Apologies if I was a blocker here; just approved on my side. Looks like this is good to go for 1.2.0! 🥳

@felixarntz
Copy link
Member

felixarntz commented Jun 3, 2022

Thanks @bethanylang!

@felixarntz felixarntz merged commit 6c3e795 into WordPress:trunk Jun 3, 2022
5 checks passed
[Focus] Site Health LEGACY automation moved this from Review to Done Jun 3, 2022
@felixarntz felixarntz changed the title Add Site Health test for full page caching Add Site Health check for Full Page Cache usage Jun 3, 2022
@manuelRod
Copy link
Contributor Author

manuelRod commented Jun 7, 2022

awesome, thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Site Health LEGACY LEGACY – Issues related to the Site Health focus area [Module] Audit Full Page Cache Issues for the Audit Full Page Cache Health Check module Needs Review Anything that requires code review [Type] Feature A new feature within an existing module
Development

Successfully merging this pull request may close these issues.

Add Site Health test for full page caching (advanced cache)
7 participants