Skip to content

Conversation

@b1ink0
Copy link
Contributor

@b1ink0 b1ink0 commented Jun 18, 2025

Summary

Fixes #2030

Relevant technical choices

This PR updates the Site Health "Enqueued Scripts" and "Enqueued Styles" tests by combining them into a single "Blocking Assets" Site Health test. It now accurately detects and reports only truly render-blocking scripts and styles. This is achieved by performing an unauthenticated loopback request to the home page and analyzing the resulting front-end HTML with the WP_HTML_Tag_Processor.

image

@b1ink0 b1ink0 added this to the performance-lab n.e.x.t milestone Jun 18, 2025
@b1ink0 b1ink0 requested a review from westonruter June 18, 2025 19:27
@b1ink0 b1ink0 added [Type] Bug An existing feature is broken [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only labels Jun 18, 2025
Comment on lines 114 to 117
$path = perflab_aea_get_path_from_resource_url( $href );
if ( '' === $path ) {
continue;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, our audit only scans assets located in the WordPress content directory. This raises an question,
how should the audit treat scripts and styles served from a CDN? Should these third-party resources be included in the render-blocking report, excluded entirely, or perhaps flagged separately so we can distinguish external blocking assets from local ones?

I think we’ll also need to consider how to measure their sizes efficiently. One approach could be, sending an HTTP HEAD request to the CDN-hosted URL to check for a Content‑Length header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

From a performance perspective, CDN-served assets could be even worse for performance since they require a new TCP connection, now that browsers don't reuse cached resources across origins. So we definitely should be including them in the render-blocking report.

Sending an HTTP HEAD request for all resources regardless of whether they are on the same origin or not makes sense to me. If the request returns in a 404 then this would be important to report as well.

Once we have the report, then a future enhancement would be digging in to find the theme/plugin responsible for the resource being added in the first place. The AMP plugin implements a lot of this, and it was getting extracted a separate package via https://github.com/GoogleChromeLabs/wp-origination but that effort got stalled and was abandoned. See also #1095.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we consider a case where a HEAD request does not return a content length due to server configuration? If so, should we then make a GET request?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, but that might be overkill.

Copy link
Member

Choose a reason for hiding this comment

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

OK, sounds good. In that case, let's not do the HEAD request at all and only do GET. The assets should all be relatively small (a few hundred KB at maximum), so it shouldn't be a problem to just go ahead and download them to check the byte size of the body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in aa50fe4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sending an HTTP HEAD request for all resources regardless of whether they are on the same origin or not makes sense to me. If the request returns in a 404 then this would be important to report as well.

Now that the GET request is sent, should only 404 errors be added to the report, or should any errors that occur during the request be added to the report?

Copy link
Contributor Author

@b1ink0 b1ink0 Jul 4, 2025

Choose a reason for hiding this comment

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

Once we have the report, then a future enhancement would be digging in to find the theme/plugin responsible for the resource being added in the first place. The AMP plugin implements a lot of this, and it was getting extracted a separate package via https://github.com/GoogleChromeLabs/wp-origination but that effort got stalled and was abandoned. See also #1095.

So does it make sense to add a table in the blocking scripts/styles site health test showing the origin of each blocking asset, or should this be part of Optimization Detective as you mentioned in #1095?

cc: @westonruter

Copy link
Member

Choose a reason for hiding this comment

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

I think this is related to #2059 (comment).

I think instead of just saying whether the sum of of the bytes for blocking assets is above a given threshold, that it would be better to list out the assets in a table with a sum at the end. It wouldn't necessarily be able to identify the theme/plugin responsible for adding the script or stylesheet, but in cases where the script/style is bundled with the theme/plugin then this would be obvious.

Comment on lines 110 to 112
if ( ! is_string( $href ) || false !== strpos( $href, 'wp-includes' ) ) {
continue;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently skip any URL containing wp-includes (i.e. core assets). Since the goal is to surface all render-blocking resources, should we remove that exclusion and include core scripts and styles in the audit as well?

Copy link
Member

Choose a reason for hiding this comment

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

I think this exclusion should be removed, yes.

Comment on lines 80 to 86
// Process blocking inline scripts.
if ( ! is_string( $src ) ) {
$script_size = mb_strlen( $processor->get_modifiable_text(), '8bit' );
if ( false !== $script_size ) {
$assets['scripts'][] = array(
'src' => 'inline',
'size' => $script_size,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently each inline SCRIPT is reported as its own entry, do we want to continue treating every inline script separately, or would it make sense to aggregate inline scripts into a single summary?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think inline scripts need to be counted since the render blocking is not significant compared to blocking external scripts.

@codecov
Copy link

codecov bot commented Jun 20, 2025

Codecov Report

❌ Patch coverage is 99.58159% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.21%. Comparing base (4c8dc6f) to head (a700783).
⚠️ Report is 75 commits behind head on trunk.

Files with missing lines Patch % Lines
...cludes/site-health/audit-enqueued-assets/hooks.php 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk    #2059      +/-   ##
==========================================
+ Coverage   67.69%   68.21%   +0.51%     
==========================================
  Files          93       93              
  Lines        7845     7947     +102     
==========================================
+ Hits         5311     5421     +110     
+ Misses       2534     2526       -8     
Flag Coverage Δ
multisite 68.21% <99.58%> (+0.51%) ⬆️
single 35.56% <46.86%> (-0.75%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@b1ink0 b1ink0 requested a review from westonruter August 12, 2025 06:16
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.

@b1ink0 Sorry for the delay in reviewing this. I've made another round of updates. Please review the additional changes, and if you approve, feel free to merge!

$this->expectException( WPAjaxDieContinueException::class );
$this->_handleAjax( 'health-check-enqueued-blocking-assets-test' );
$response = json_decode( $this->_last_response, true );
$this->assertFalse( $response['success'] );
Copy link
Member

Choose a reason for hiding this comment

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

Todo: verify this assertion is actually getting called, given the exception being thrown above. Do the same in the other tests for this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After testing, I can confirm that everything after this->_handleAjax is not getting called it's exiting early. After some digging, I found that if we want to run some assertions afterward, we need to catch the exception manually.

ref: https://infinum.com/handbook/wordpress/automated-testing-in-wordpress/basic-tests-using-phpunit/integration-testing#:~:text=or%2C%20if%20you%20need%20to%20run%20some%20assertions%20afterward%20you%27d%20catch%20the%20exception%20it%20manually

Like this:

diff --git a/plugins/performance-lab/tests/includes/site-health/audit-enqueued-assets/test-audit-enqueued-assets-helper.php b/plugins/performance-lab/tests/includes/site-health/audit-enqueued-assets/test-audit-enqueued-assets-helper.php
index 9f2eb1b0..12e88284 100644
--- a/plugins/performance-lab/tests/includes/site-health/audit-enqueued-assets/test-audit-enqueued-assets-helper.php
+++ b/plugins/performance-lab/tests/includes/site-health/audit-enqueued-assets/test-audit-enqueued-assets-helper.php
@@ -299,8 +299,11 @@ class Test_Audit_Enqueued_Assets_Helper extends WP_Ajax_UnitTestCase {
 	public function test_perflab_aea_enqueued_ajax_blocking_assets_test_unauthenticated_with_nonce(): void {
 		$this->add_filter_to_mock_front_page_loopback_request();
 		$_GET['_wpnonce'] = wp_create_nonce( 'health-check-site-status' );
-		$this->expectException( WPAjaxDieContinueException::class );
-		$this->_handleAjax( 'health-check-enqueued-blocking-assets-test' );
+		try {
+			$this->_handleAjax( 'health-check-enqueued-blocking-assets-test' );
+		} catch ( WPAjaxDieContinueException $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch
+			// Expected exception for unauthenticated access.
+		}
 		$response = json_decode( $this->_last_response, true );
 		$this->assertFalse( $response['success'] );
 	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added fix suggestions for all the AJAX tests please verify them.

Copy link
Member

Choose a reason for hiding this comment

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

@b1ink0 Thanks for digging into that! I've iterated on your suggestions and came up with 46b4df4.

Comment on lines 287 to 291
$this->expectException( WPAjaxDieStopException::class );
$this->_handleAjax( 'health-check-enqueued-blocking-assets-test' );
$response = json_decode( $this->_last_response, true );
$this->assertArrayHasKey( 'success', $response );
$this->assertFalse( $response['success'] );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we need to check for '-1', as it seems WordPress returns '-1' when no nonce is present. We also need to use try and catch to check the output see my comment #2059 (comment).

ref: https://github.com/WordPress/wordpress-develop/blob/197f0a71ad27d0688b6380c869aeaf92addd1451/src/wp-includes/pluggable.php#L1360-L1365

Suggested change
$this->expectException( WPAjaxDieStopException::class );
$this->_handleAjax( 'health-check-enqueued-blocking-assets-test' );
$response = json_decode( $this->_last_response, true );
$this->assertArrayHasKey( 'success', $response );
$this->assertFalse( $response['success'] );
try {
$this->_handleAjax( 'health-check-enqueued-blocking-assets-test' );
} catch ( WPAjaxDieStopException $e ) {
$this->assertEquals( '-1', $e->getMessage() );
}

@westonruter
Copy link
Member

Strange. Tests pass for me locally, but they fail on GHA with at Test_Audit_Enqueued_Assets_Helper::test_perflab_aea_enqueued_ajax_blocking_assets_test_authorized.

@b1ink0
Copy link
Contributor Author

b1ink0 commented Aug 17, 2025

Strange. Tests pass for me locally, but they fail on GHA with at Test_Audit_Enqueued_Assets_Helper::test_perflab_aea_enqueued_ajax_blocking_assets_test_authorized.

I tested locally, and the tests fail on the multisite test test-php-multisite:performance-lab locally as well.

@westonruter
Copy link
Member

@b1ink0 Ah, so we would need to create a superadmin for the test to pass successfully I bet, since Site Health is not available except in the network admin, as I recall.

Co-authored-by: Aditya Dhade <[email protected]>
@westonruter
Copy link
Member

These pains with Ajax testing make me realize that in the future we should go with using the REST API for Site Health tests instead 😅

@westonruter
Copy link
Member

@b1ink0 Nice. And that fixed coverage to be 100% for that file.

@westonruter westonruter merged commit 84002b1 into WordPress:trunk Aug 17, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Bug An existing feature is broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Site Health tests for enqueued scripts and styles are inaccurate

2 participants