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

Improve page cache detection test #6849

Merged
merged 35 commits into from
Jan 28, 2022
Merged

Conversation

dhaval-parekh
Copy link
Collaborator

@dhaval-parekh dhaval-parekh commented Jan 20, 2022

Summary

Fixes #6830

This PR improves the mechanism to detect page caching availability on websites.

image

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@dhaval-parekh dhaval-parekh marked this pull request as ready for review January 24, 2022 14:24
@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2022

Plugin builds for 1d25cc4 are ready 🛎️!

src/Admin/SiteHealth.php Show resolved Hide resolved
src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
src/Admin/SiteHealth.php Show resolved Hide resolved
src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
src/Admin/SiteHealth.php Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #6849 (844cd16) into develop (b311b57) will increase coverage by 0.06%.
The diff coverage is 97.27%.

❗ Current head 844cd16 differs from pull request most recent head 0d97691. Consider uploading reports for the commit 0d97691 to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #6849      +/-   ##
=============================================
+ Coverage      78.46%   78.52%   +0.06%     
- Complexity      6735     6764      +29     
=============================================
  Files            202      202              
  Lines          20321    20395      +74     
=============================================
+ Hits           15944    16015      +71     
- Misses          4377     4380       +3     
Flag Coverage Δ
php 78.52% <97.27%> (+0.06%) ⬆️
unit 78.52% <97.27%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Admin/SiteHealth.php 84.56% <97.16%> (+1.94%) ⬆️
src/Admin/OptionsMenu.php 94.73% <100.00%> (+0.03%) ⬆️

@westonruter westonruter dismissed their stale review January 27, 2022 18:01

Much improved! Thank you.

@westonruter
Copy link
Member

E2E tests seem to be stalling. I don't think they're related to this PR. cc @delawski

@westonruter
Copy link
Member

Here are the scenarios I'm testing:

Failure to Do Loopback Request

With this plugin active:

add_action('template_redirect', function () {
	status_header( 500 );
	exit;
});

image

No Page Caching with Slow Response Time

With this plugin active:

add_action( 'wp_footer', function () {
	sleep(1);
} );

image

No Page Caching with Good Response Time

No special plugins active.

image

Page Caching Header(s) with Bad Response Time

Test plugin:

add_filter(
	'wp_headers',
	function ( $headers ) {
		$headers['etag'] = '"foo"';
		$headers['last-modified'] = gmdate('r');
		return $headers;
	}
);

add_action( 'wp_footer', function () {
	sleep(1);
} );

Note that since page caching response headers are present, the lack of a page caching plugin is not mentioned (since it's likely an external page cache is being used):

image

Page Caching Header with Good Response Time

Plugin:

add_filter(
	'wp_headers',
	function ( $headers ) {
		$headers['etag'] = '"foo"';
		$headers['last-modified'] = gmdate('r');
		return $headers;
	}
);

Note that since page caching response headers are present, the lack of a page caching plugin is not mentioned (since it's likely an external page cache is being used):

image

Page Caching Plugin with Good Response Time

With WP Super Cache active and with the following plugin active:

add_action( 'wp_footer', function () {
	sleep(1);
} );

Note that since the median time is used, the 1-second response time is disregarded:

image

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

This is a huge improvement. Way more robust.

The only thing outstanding are the failing E2E tests.

Comment on lines 397 to 398
if ( empty( $page_cache_detail['response_time'] ) ) {
$page_cache_test_summary[] = '<span class="dashicons dashicons-dismiss text-error"></span> ' . __( 'Server response time could not be determined. Verify that loopback requests are working.', 'amp' );
Copy link
Member

Choose a reason for hiding this comment

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

Note: I don't believe this scenario is ever possibly reached.

@delawski delawski force-pushed the bug/6830-page-cache-detection branch from 8ff39b5 to c7dd6ab Compare January 28, 2022 13:10
@delawski delawski force-pushed the bug/6830-page-cache-detection branch from 686de21 to 4a4c490 Compare January 28, 2022 13:43
@delawski
Copy link
Collaborator

I've looked into the E2E tests issue and wasn't able to find the reason for it. Since I wasn't able to reproduce the issue locally, I had to play a guessing game on this.

First, I tried changing the selector that fails in multiple unit tests (90d9a16). It didn't help.

Then, I thought that maybe AMP plugin is not activated on the test environment for whatever reason (maybe new WordPress 5.9 or the recent update of Gutenberg npm packages?). I've added a rule to try activating the plugin from within E2E tests (4a4c490). It didn't help as well.

Lastly, I've rebased the branch onto develop to see if by any luck it helps but it didn't.

One more thing I did was to issue a new PR for another feature I just completed working on #6859. I branched off of develop yesterday so it's very fresh. The E2E tests are passing on that other branch so I'm totally disoriented right now.

I don't have more time to investigate the issue further today.

@westonruter westonruter added this to the v2.2.1 milestone Jan 28, 2022
@westonruter
Copy link
Member

I'm gonna push up some commits to debug this. I'll be doing some force-pushes to beware.

@westonruter westonruter force-pushed the bug/6830-page-cache-detection branch from d323217 to 188e0cd Compare January 28, 2022 20:45
@westonruter
Copy link
Member

It turns out the E2E tests were working perfectly! They were catching an actual problem which I've fixed in 188e0cd. The issue is it wasn't accounting for \AmpProject\AmpWP\Admin\SiteHealth::get_page_cache_detail() returning a WP_Error (as when loopback requests fail), and this resulted in the Settings page failing to load. So I've added the check to ensure it's an array value and that fixed the tests.

@delawski Thank you for wrestling with this in any case!

@westonruter westonruter changed the title Improve page cache detection mechanism. Improve page cache detection test Jan 28, 2022
@westonruter westonruter merged commit f2fca89 into develop Jan 28, 2022
@westonruter westonruter deleted the bug/6830-page-cache-detection branch January 28, 2022 22:19
westonruter added a commit that referenced this pull request Jan 28, 2022
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page caching is not detected (wp-rocket)
3 participants