Prevent effective asset cache headers audit from running on local/development environments #2035

hbhalodia · 2025-06-03T08:16:54Z

Summary

Fixes #2031

Relevant technical choices

Update the site_status_tests filter to unset the effective_asset_cache_headers for local and development env.
Tests are not updated as unable to mock the wp_get_environment_type to support for 2 different scenarios.

…nment

github-actions · 2025-06-03T08:17:04Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: hbhalodia <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: b1ink0 <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

hbhalodia · 2025-06-03T08:26:28Z

Hi @westonruter, Test fails because it is not able to mimic the production enviornment, If we have any workaround to check the both scenarios, I am happy to update the tests. Ref - #2031 (comment)

b1ink0 · 2025-06-03T17:21:49Z

Hi @hbhalodia, I think removing the entire test would be the only solution, as wp_get_environment_type will always return local or development on the CI/CD, which will prevent the site health test from being added at all.

hbhalodia · 2025-06-04T03:49:16Z

Hi @b1ink0, I have removed the tests for now.

Thank You,

codecov · 2025-06-04T03:57:29Z

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 68.02%. Comparing base (e741539) to head (f56f9d6).
Report is 8 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...ite-health/effective-asset-cache-headers/hooks.php	20.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk    #2035      +/-   ##
==========================================
- Coverage   68.06%   68.02%   -0.05%     
==========================================
  Files          92       92              
  Lines        7626     7627       +1     
==========================================
- Hits         5191     5188       -3     
- Misses       2435     2439       +4

Flag	Coverage Δ
multisite	`68.02% <20.00%> (-0.05%)`	⬇️
single	`36.98% <20.00%> (-0.05%)`	⬇️

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.

westonruter · 2025-06-04T06:03:32Z

I think removing the entire test would be the only solution, as wp_get_environment_type will always return local or development on the CI/CD, which will prevent the site health test from being added at all.

@b1ink0 @hbhalodia What about adding @runInSeparateProcess to the test so it can define the constant before the assertion?

hbhalodia · 2025-06-04T07:05:19Z

I think removing the entire test would be the only solution, as wp_get_environment_type will always return local or development on the CI/CD, which will prevent the site health test from being added at all.

@b1ink0 @hbhalodia What about adding @runInSeparateProcess to the test so it can define the constant before the assertion?

Hi @westonruter, I can try that but how can I set 2 different values for the same constant? like we need to first test for local that key should not exists, and then again set the constant to production and check the key should exists. So that we can cover all the scenarios?

I checked this -

performance/plugins/optimization-detective/tests/test-uninstall.php

Lines 18 to 20 in 05444d4

    
           if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) { 
        
           	define( 'WP_UNINSTALL_PLUGIN', 'Yes' ); 
        
           }

,

But still finding it complex on how to accomodate both scenarios?

OR, if I am thinking correctly,

Would add the normal test case with local scenario on current file as expected, and create a new file with @runInSeparateProcess and just call this one test again with mocking of constant to production?

b1ink0 · 2025-06-04T09:07:34Z

@westonruter @runInSeparateProcess will not solve the issue, as the WP_ENVIRONMENT_TYPE constant is defined in /wordpress-phpunit/wp-tests-config.php in the Docker container. This config is generated by the wp-env package, and by default, it sets WP_ENVIRONMENT_TYPE to local. You can see that here: (https://github.com/WordPress/gutenberg/blob/a5e3592f0a4e66757f21754bfafe6c6ac7c83aea/packages/env/lib/config/parse-config.js#L99). As a result, WP_ENVIRONMENT_TYPE will be defined even before the test loads, which means the constant will still be defined even when @runInSeparateProcess is used.

This could only be overridden by modifying the .wp-env.json, but doing so would cause all tests to run in a defined environment, which is not the right approach.

The only other option I can think of is using the wp_get_development_mode function instead of wp_get_environment_type, as it allows its result to be overridden using the global variable $GLOBALS['_wp_tests_development_mode']. However, to use this, we also need to define the WP_RUN_CORE_TESTS constant, since this global is only supposed to be used in core tests.
Ref: (https://github.com/WordPress/wordpress-develop/blob/45cf47811a8adae99897ec4cceaf7fd3acbabe38/src/wp-includes/load.php#L328-L331).

If we go with this, then if you use wp-env for development, this site health test will not be displayed in your Site Health, because in this repo’s .wp-env.json, the WP_DEVELOPMENT_MODE constant is set to plugin.

For me, I mostly use LocalWP for development, so I have manually set the WP_DEVELOPMENT_MODE constant to plugin in the wp-config.php.

If we go with wp_get_development_mode, then the tests could be written like this:

patch

diff --git a/plugins/performance-lab/includes/site-health/effective-asset-cache-headers/hooks.php b/plugins/performance-lab/includes/site-health/effective-asset-cache-headers/hooks.php
index cc083eee..07de32f1 100644
--- a/plugins/performance-lab/includes/site-health/effective-asset-cache-headers/hooks.php
+++ b/plugins/performance-lab/includes/site-health/effective-asset-cache-headers/hooks.php
@@ -32,7 +32,7 @@ function perflab_effective_asset_cache_headers_add_test( array $tests ): array {
 	 *
 	 * GH Issue: https://github.com/WordPress/performance/issues/2031
 	 */
-	if ( in_array( wp_get_environment_type(), array( 'local', 'development' ), true ) ) {
+	if ( in_array( wp_get_development_mode(), array( 'plugin' ), true ) ) {
 		unset( $tests['direct']['effective_asset_cache_headers'] );
 	}
 
diff --git a/plugins/performance-lab/tests/includes/site-health/effective-asset-cache-headers/test-effective-asset-cache-headers.php b/plugins/performance-lab/tests/includes/site-health/effective-asset-cache-headers/test-effective-asset-cache-headers.php
index b35cd092..9b104180 100644
--- a/plugins/performance-lab/tests/includes/site-health/effective-asset-cache-headers/test-effective-asset-cache-headers.php
+++ b/plugins/performance-lab/tests/includes/site-health/effective-asset-cache-headers/test-effective-asset-cache-headers.php
@@ -31,16 +31,47 @@ class Test_Effective_Asset_Cache_Headers extends WP_UnitTestCase {
 	/**
 	 * Test that the effective caching headers test is added to the site health tests.
 	 *
+	 * @runInSeparateProcess
+	 * @preserveGlobalState disabled
+	 *
+	 * @dataProvider data_provider_test_perflab_effective_asset_cache_headers_add_test
 	 * @covers ::perflab_effective_asset_cache_headers_add_test
+	 *
+	 * @param string $wp_development_mode The development mode to test against. Can be 'plugin' or '' (non-development mode).
 	 */
-	public function test_perflab_effective_asset_cache_headers_add_test(): void {
+	public function test_perflab_effective_asset_cache_headers_add_test( string $wp_development_mode ): void {
+		if ( ! defined( 'WP_RUN_CORE_TESTS' ) ) {
+			define( 'WP_RUN_CORE_TESTS', 'YES' );
+		}
+
+		$GLOBALS['_wp_tests_development_mode'] = $wp_development_mode;
+
 		$tests = array(
 			'direct' => array(),
 		);
 
 		$tests = perflab_effective_asset_cache_headers_add_test( $tests );
 
-		$this->assertArrayNotHasKey( 'effective_asset_cache_headers', $tests['direct'] );
+		if ( 'plugin' === $wp_development_mode ) {
+			// If in development mode, the test should not be added.
+			$this->assertArrayNotHasKey( 'effective_asset_cache_headers', $tests['direct'] );
+		} else {
+			$this->assertArrayHasKey( 'effective_asset_cache_headers', $tests['direct'] );
+			$this->assertEquals( 'Effective Caching Headers', $tests['direct']['effective_asset_cache_headers']['label'] );
+			$this->assertEquals( 'perflab_effective_asset_cache_headers_assets_test', $tests['direct']['effective_asset_cache_headers']['test'] );
+		}
+	}
+
+	/**
+	 * Data provider for test_perflab_effective_asset_cache_headers_add_test.
+	 *
+	 * @return array<array<string>> Data provider.
+	 */
+	public function data_provider_test_perflab_effective_asset_cache_headers_add_test(): array {
+		return array(
+			array( 'non_development_mode' => '' ),
+			array( 'development_mode' => 'plugin' ),
+		);
 	}
 
 	/**

westonruter · 2025-06-04T13:43:44Z

Oh, I wasn't aware the constant was already defined up front. That does complicate things. Would it be feasible to add runkit as a dependency for running this test and if so utilize https://www.php.net/manual/en/function.runkit7-constant-remove.php to remove the constant? If runkit isn't available then the test could be skipped.

b1ink0 · 2025-06-04T17:44:50Z

Hi @westonruter,

Adding runkit as a dependency seems a bit heavy for this specific use case.

There are a few technical challenges:

The wp-env Docker environment doesn’t include runkit by default, so we’d need custom Docker setup.
We’d have to ensure it works reliably across all our GitHub Actions workflows and testing environments.
Since PHP 8 support in runkit7 is still incomplete and experimental, it doesn’t make sense to add it right now.
Installing it would require significant changes to our testing infrastructure and a more complicated setup with wp-env.

While runkit could be helpful for testing hard-to-mock code (like constants or native functions), it feels like overkill for this particular PR.

For now, I’d lean toward removing the test or using wp_get_development_mode as I suggested in the #2035 (comment) .

westonruter · 2025-06-04T21:06:46Z

plugins/performance-lab/includes/site-health/effective-asset-cache-headers/hooks.php

 	return $tests;
 }
-add_filter( 'site_status_tests', 'perflab_effective_asset_cache_headers_add_test' );
+add_filter( 'site_status_tests', 'perflab_effective_asset_cache_headers_add_test', 100 );


Why the change to priority 100?

Need to make sure it would run after all the filters, I thought it would be filter somewhere else as well, but priority 100 can be removed though.

westonruter · 2025-06-04T21:07:08Z

plugins/performance-lab/includes/site-health/effective-asset-cache-headers/hooks.php

@@ -26,6 +26,16 @@ function perflab_effective_asset_cache_headers_add_test( array $tests ): array {
 		'label' => __( 'Effective Caching Headers', 'performance-lab' ),
 		'test'  => 'perflab_effective_asset_cache_headers_assets_test',
 	);
+
+	/**


Suggested change

/**

/*

westonruter · 2025-06-04T21:09:09Z

plugins/performance-lab/includes/site-health/effective-asset-cache-headers/hooks.php

+	 * Static assets are expected to not have effective cache headers in non-production environments.
+	 *
+	 * GH Issue: https://github.com/WordPress/performance/issues/2031
+	 */
+	if ( in_array( wp_get_environment_type(), array( 'local', 'development' ), true ) ) {
+		unset( $tests['direct']['effective_asset_cache_headers'] );
+	}


Instead of unsetting something that was set, why not just short-circuit the function to avoid setting it in the first place, or rather wrap the setting with the if statement, so the entire function body can be:

if ( ! in_array( wp_get_environment_type(), array( 'local', 'development' ), true ) ) { $tests['direct']['effective_asset_cache_headers'] = array( 'label' => __( 'Effective Caching Headers', 'performance-lab' ), 'test' => 'perflab_effective_asset_cache_headers_assets_test', ); } return $tests;

Yeah, it can be work and even more instead of wrapping it in if, we can bail early like,

/* * Bail early. */ if ( in_array( wp_get_environment_type(), array( 'local', 'development' ), true ) ) { return $tests; } $tests['direct']['effective_asset_cache_headers'] = array( 'label' => __( 'Effective Caching Headers', 'performance-lab' ), 'test' => 'perflab_effective_asset_cache_headers_assets_test', ); return $tests;

westonruter · 2025-06-04T21:11:43Z

For now, I’d lean toward removing the test

@b1ink0 Thanks for that feedback.

So removing test_perflab_effective_asset_cache_headers_add_test_is_attached_to_site_status_tests? That seems fine to me. As long as we are keeping the actual tests for perflab_effective_asset_cache_headers_assets_test() then testing whether or not the test is added is not important.

b1ink0 · 2025-06-05T08:30:51Z

So removing test_perflab_effective_asset_cache_headers_add_test_is_attached_to_site_status_tests? That seems fine to me. As long as we are keeping the actual tests for perflab_effective_asset_cache_headers_assets_test() then testing whether or not the test is added is not important.

Only the test that checks whether the Site Health test was added needs to be removed. The other tests are not affected by this change, as it calls the functions directly to test them and doesn't rely on the filter.

westonruter · 2025-06-05T15:46:42Z

plugins/performance-lab/includes/site-health/effective-asset-cache-headers/hooks.php

+	if ( in_array( wp_get_environment_type(), array( 'local', 'development' ), true ) ) {
+		return $tests;
+	}
+
 	$tests['direct']['effective_asset_cache_headers'] = array(
 		'label' => __( 'Effective Caching Headers', 'performance-lab' ),
 		'test'  => 'perflab_effective_asset_cache_headers_assets_test',
 	);
+


I think this is slightly better as it means there is only one return $tests:

Suggested change

if ( in_array( wp_get_environment_type(), array( 'local', 'development' ), true ) ) {

return $tests;

}

$tests['direct']['effective_asset_cache_headers'] = array(

'label' => __( 'Effective Caching Headers', 'performance-lab' ),

'test' => 'perflab_effective_asset_cache_headers_assets_test',

);

if ( ! in_array( wp_get_environment_type(), array( 'local', 'development' ), true ) ) {

$tests['direct']['effective_asset_cache_headers'] = array(

'label' => __( 'Effective Caching Headers', 'performance-lab' ),

'test' => 'perflab_effective_asset_cache_headers_assets_test',

);

}

Okay, would update this to use only 1 return statement.

This is now fixed and function is updated.

Thank You,

westonruter

Thanks for working on this!

hbhalodia added 2 commits June 3, 2025 11:53

Remove static asset caching from site health for local and dev envior…

490d093

…nment

Revert update test case function

48271bf

hbhalodia requested review from mukeshpanchal27 and felixarntz as code owners June 3, 2025 08:16

b1ink0 added [Type] Bug An existing feature is broken [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only labels Jun 3, 2025

Remove the test case as it cannot be cover all scenarios

284b7f3

Add the test case to run for the local env

85573d7

westonruter added this to the performance-lab n.e.x.t milestone Jun 4, 2025

westonruter reviewed Jun 4, 2025

View reviewed changes

Short circuit function and remove filter priority

d6565a0

hbhalodia requested a review from westonruter June 5, 2025 04:51

westonruter reviewed Jun 5, 2025

View reviewed changes

hbhalodia and others added 2 commits June 6, 2025 10:05

Merge branch 'trunk' into feat/issue-2031

5413da2

Update the function to use only one return statement

f56f9d6

hbhalodia requested a review from westonruter June 6, 2025 04:38

westonruter approved these changes Jun 6, 2025

View reviewed changes

westonruter merged commit fe89427 into WordPress:trunk Jun 6, 2025
18 checks passed

westonruter changed the title ~~Effective asset cache headers audit should not fail when on local/development environments~~ Prevent effective asset cache headers audit from running on local/development environments Jun 6, 2025

Prevent effective asset cache headers audit from running on local/development environments #2035

Prevent effective asset cache headers audit from running on local/development environments #2035

Conversation

hbhalodia commented Jun 3, 2025

Summary

Relevant technical choices

Uh oh!

github-actions bot commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hbhalodia commented Jun 3, 2025

Uh oh!

b1ink0 commented Jun 3, 2025

Uh oh!

hbhalodia commented Jun 4, 2025

Uh oh!

codecov bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

westonruter commented Jun 4, 2025

Uh oh!

hbhalodia commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

b1ink0 commented Jun 4, 2025

Uh oh!

westonruter commented Jun 4, 2025

Uh oh!

b1ink0 commented Jun 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Jun 4, 2025

Uh oh!

b1ink0 commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 3, 2025 •

edited

Loading

codecov bot commented Jun 4, 2025 •

edited

Loading

hbhalodia commented Jun 4, 2025 •

edited

Loading