Clear out and remove `/tests` directory by ShyamGadde · Pull Request #1804 · WordPress/performance

ShyamGadde · 2025-01-16T12:52:54Z

Summary

Fixes #1433

Relevant technical choices

tests/bootstrap.php has been relocated to /tools/phpunit for consistency with other configuration helpers.
Test images have been moved to the tests/data/images directories of the specific plugins that use them.
Optimization Detective test helpers have been moved to /plugins/optimization-detective/tests.

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

…tory Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

codecov · 2025-01-16T13:02:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.89%. Comparing base (3ce0f45) to head (a985976).
Report is 40 commits behind head on trunk.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk    #1804      +/-   ##
==========================================
+ Coverage   65.85%   65.89%   +0.03%     
==========================================
  Files          88       88              
  Lines        6851     6878      +27     
==========================================
+ Hits         4512     4532      +20     
- Misses       2339     2346       +7

Flag	Coverage Δ
multisite	`65.89% <ø> (+0.03%)`	⬆️
single	`38.58% <ø> (+0.11%)`	⬆️

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.

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

westonruter · 2025-01-24T17:05:33Z

This is still marked as a draft. Is there still work to do on it?

ShyamGadde · 2025-01-24T17:09:47Z

There's no further work needed on this PR—it's ready for review. Apologies for leaving it as a draft for so long.

github-actions · 2025-01-24T17:10:17Z

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: ShyamGadde <shyamgadde@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

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

felixarntz

@ShyamGadde Thanks, it's great to see our project structure finally cleaned up properly. I have just one question on the changes.

felixarntz · 2025-01-27T18:30:36Z

 		add_filter( 'wp_image_editors', '__return_empty_array' );
 		yield 'when no editor is present' => array(
-			self::factory()->attachment->create_upload_object( TESTS_PLUGIN_DIR . '/tests/data/images/car.jpeg' ),
+			self::factory()->attachment->create_upload_object( __DIR__ . '/data/images/car.jpeg' ),


Curious why this was changed throughout all these files? Does TESTS_PLUGIN_DIR no longer work? Shouldn't it still work for each plugin?

TESTS_PLUGIN_DIR is obsolete since it refers to the /tests as opposed to /plugins/*/tests, right? Now that the images have been moved into the respective plugins' tests directories, then there's no need for TESTS_PLUGIN_DIR.

I think it's a useful constant to have, that's why I don't think it should be obsolete. It would make sense for this to always point to the root directory of the plugin that is being tested - that is how it was working initially and would continue to be helpful.

I don't think that's how it was working initially. It is defined here:

performance/tests/bootstrap.php

Line 8 in 93257ee

define( 'TESTS_PLUGIN_DIR', dirname( __DIR__ ) );

So it was referring to the tests directory in the root of the performance repo, which now no longer exists.

So re-introduce that constant but have it point to the plugin currently being tested? Actually, I see in this PR it still exists:

performance/tools/phpunit/bootstrap.php

Line 8 in f829057

define( 'TESTS_PLUGIN_DIR', dirname( __DIR__, 2 ) );

But it is pointing to the repo root it seems. That seems like it should be changed to be something like REPO_ROOT_DIR. And then TESTS_PLUGIN_DIR could be re-introduced to be dynamic after here:

performance/tools/phpunit/bootstrap.php

Lines 44 to 59 in f829057

// Check if we use the plugin's test suite. If so, disable the PL plugin and only load the requested plugin.

$plugin_name = '';

foreach ( $_SERVER['argv'] as $index => $arg ) {

if (

'--testsuite' === $arg &&

isset( $_SERVER['argv'][ $index + 1 ] ) &&

file_exists( TESTS_PLUGIN_DIR . '/plugins/' . $_SERVER['argv'][ $index + 1 ] )

) {

$plugin_name = $_SERVER['argv'][ $index + 1 ];

}

}

// Set default plugin to load if not specified.

if ( '' === $plugin_name ) {

$plugin_name = 'performance-lab';

}

Like:

define( 'TESTS_PLUGIN_DIR', REPO_ROOT_DIR . "/plugins/$plugin_name/tests" );

Curious why this was changed throughout all these files? Does TESTS_PLUGIN_DIR no longer work? Shouldn't it still work for each plugin?

TESTS_PLUGIN_DIR has always pointed to /var/www/html/wp-content/plugins/performance rather than individual plugin directories. In the dominant-color-images plugin, the existing pattern used __DIR__ . <image> to reference image paths within its own directory. To maintain consistency, I followed the same approach across all relevant instances.

@westonruter I think what you're saying would be a good solution.

@ShyamGadde The original reason for TESTS_PLUGIN_DIR was to point to the plugin's directory - that was back when the whole repository was for only one plugin, the Performance Lab plugin. This is also reflected in the name TESTS_PLUGIN_DIR, which with its current usage doesn't make sense anymore. But it does have value as a concept of pointing to the plugin's directory. With the dominant-color-images plugin tests, they were probably using __DIR__ because their images had already been moved before. But having a dedicated constant pointing to the plugin directory and using that would be a better solution IMO.

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

TESTS_PLUGIN_DIR is now dynamic and points to each plugins tests directory. Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

felixarntz · 2025-01-27T23:57:40Z

 	$plugin_name = 'performance-lab';
 }

+define( 'TESTS_PLUGIN_DIR', REPO_ROOT_DIR . "/plugins/$plugin_name/tests" );


This constant should point to the root directory of the plugin, not the tests directory. That's the original purpose of the constant, and it IMO still makes sense as it simplifies to refer to anything within the plugin's directory, not just test-specific things. The only reason it's prefixed with TESTS_ is that the constant is test specific.

Suggested change

define( 'TESTS_PLUGIN_DIR', REPO_ROOT_DIR . "/plugins/$plugin_name/tests" );

define( 'TESTS_PLUGIN_DIR', REPO_ROOT_DIR . "/plugins/$plugin_name" );

Of course after this the references will need to be adjusted again. For many occurrences though, this effectively means leaving them like they used to be and making the PR diff much smaller. A huge part of the PR diff now is only because of this change to the TESTS_PLUGIN_DIR constant pointing to the plugin's tests dir.

Done in 70de425.

The only reason it's prefixed with TESTS_ is that the constant is test specific.

Ah, right. It's just the prefix. I was interpreting it as PLUGIN_TESTS_DIR. So if we wanted a tests/ dir constant then we could have TESTS_PLUGIN_TESTS_DIR 😅 (Not needed.)

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

ShyamGadde · 2025-01-28T04:38:19Z

 		return array(
 			'animated_gif'  => array(
-				'image_path'            => __DIR__ . '/images/animated.gif',
+				'image_path'            => TESTS_PLUGIN_DIR . '/tests/data/images/animated.gif',


Regarding this specific change, should this also be reverted to the original __DIR__ pattern?

I think it makes sense to consistently use TESTS_PLUGIN_DIR in those cases. This way all path strings after the constant are relative to the same root.

westonruter · 2025-01-28T18:23:36Z

+ * @noinspection PhpUnhandledExceptionInspection
+ */
+
+if ( ! trait_exists( 'Optimization_Detective_Test_Helpers' ) ) {


Is this if necessary?

I added this as I kept encountering the following error:

PHP Fatal error: Cannot declare trait Optimization_Detective_Test_Helpers, because the name is already in use in /var/www/html/wp-content/plugins/performance/plugins/optimization-detective/tests/class-optimization-detective-test-helpers.php on line 18

Adding this if condition resolved the issue. If there's a better way to address this, I'd be happy to update it.

Humm. Even with the require_once. I guess it has to do with Docker and how it mounts the directories in a way that PHP doesn't realize that the paths are the same?

What if lines like:

require_once __DIR__ . '/../../optimization-detective/tests/class-optimization-detective-test-helpers.php';

Are replaced with:

require_once TESTS_REPO_ROOT_DIR . '/plugins/optimization-detective/tests/class-optimization-detective-test-helpers.php';

Replacing it with TESTS_REPO_ROOT_DIR works perfectly and resolves the issue.

Done in b002bff.

westonruter · 2025-01-28T18:29:06Z

 */

-define( 'TESTS_PLUGIN_DIR', dirname( __DIR__ ) );
+define( 'REPO_ROOT_DIR', dirname( __DIR__, 2 ) );


Maybe we should prefix REPO_ROOT_DIR with TESTS_ just to be sure we are clear it is only for tests.

That makes sense. Updated it in 6df3ca2.

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

westonruter · 2025-01-28T19:02:20Z

@@ -7,3 +7,6 @@

 // Require the suggested plugin.
 require_once __DIR__ . '/../../optimization-detective/load.php';


Other places where __DIR__ . '../../' is used we can also replace with:

Suggested change

require_once __DIR__ . '/../../optimization-detective/load.php';

require_once TESTS_REPO_ROOT_DIR . '/plugins/optimization-detective/load.php';

Note that this manual loading of Optimization Detective is done for Embed Optimizer but not for Image Prioritizer because the latter has Optimization Detective as an explicit dependency, whereas the former it is a recommended dependency. The test bootstrap logic automatically loads any explicit dependencies.

Here are the relevant instances (updated):

plugins/optimization-detective/tests/test-uninstall.php:27: require __DIR__ . '/../../../plugins/optimization-detective/uninstall.php'; plugins/embed-optimizer/tests/bootstrap.php:9:require_once __DIR__ . '/../../optimization-detective/load.php'; plugins/speculation-rules/tests/test-speculation-rules-uninstall.php:27: require __DIR__ . '/../../../plugins/speculation-rules/uninstall.php';

Oh, the lines in auto-sizes are obsolete. They should have been removed with #1476

Oh, the lines in auto-sizes are obsolete. They should have been removed with #1476

Removed in 6421043

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

westonruter

Looks great!

felixarntz

@ShyamGadde Only a few non-blocking points where we could use a more specific constant, but I'm pre-approving. Thanks for the iterations, this looks good overall!

felixarntz · 2025-01-28T20:15:39Z

 */
 $load_plugin = static function ( string $plugin_name ) use ( &$load_plugin ): void {
-	$plugin_test_path = TESTS_PLUGIN_DIR . '/plugins/' . $plugin_name;
+	$plugin_test_path = TESTS_REPO_ROOT_DIR . '/plugins/' . $plugin_name;


See above:

Suggested change

$plugin_test_path = TESTS_REPO_ROOT_DIR . '/plugins/' . $plugin_name;

$plugin_test_path = TESTS_PLUGIN_DIR;

Alternatively, we could get rid of this variable entirely, since it's only referencing the constant as-is.

Actually, we can't do this. This function also needs to load plugin dependencies. I thought this at first myself.

Which function do you mean?

See this $load_plugin function here is used recursively. It loads any Requires Plugins.

I'm not sure when the $load_plugin approach was added, just seeing this now.

It doesn't make sense to me to use this approach, especially given the complexity you're describing. There is a special global to control which plugins should be active. For example:

$GLOBALS['wp_tests_options'] = array( 'active_plugins' => array( basename( TESTS_PLUGIN_DIR ) . '/load.php' ), );

Can we not use that approach? If the problem is that we're loading plugins from a custom subdirectory, I wonder whether we could maybe override WP_PLUGIN_DIR to be TESTS_REPO_ROOT_DIR . '/plugins'.

Overall, it certainly makes more sense to me to tie into WordPress's built-in plugin loading mechanism instead of trying to manually require files here (which BTW plugins_loaded is, strictly speaking, also too late for).

The current approach seems to have been working well, but what you propose seems good. Maybe do it another PR?

Makes sense, that would be a separate enhancement.

felixarntz · 2025-01-28T20:16:24Z

+			require_once TESTS_REPO_ROOT_DIR . '/plugins/' . $plugin_name . '/includes/admin/load.php';
+			require_once TESTS_REPO_ROOT_DIR . '/plugins/' . $plugin_name . '/includes/admin/server-timing.php';
+			require_once TESTS_REPO_ROOT_DIR . '/plugins/' . $plugin_name . '/includes/admin/plugins.php';


See above:

Suggested change

require_once TESTS_REPO_ROOT_DIR . '/plugins/' . $plugin_name . '/includes/admin/load.php';

require_once TESTS_REPO_ROOT_DIR . '/plugins/' . $plugin_name . '/includes/admin/server-timing.php';

require_once TESTS_REPO_ROOT_DIR . '/plugins/' . $plugin_name . '/includes/admin/plugins.php';

require_once TESTS_PLUGIN_DIR . '/includes/admin/load.php';

require_once TESTS_PLUGIN_DIR . '/includes/admin/server-timing.php';

require_once TESTS_PLUGIN_DIR . '/includes/admin/plugins.php';

Actually we can't do this. See #1804 (comment)

What do you mean? These files don't require any dependencies.

Plugin dependencies (i.e. Image Prioritizer depending on Optimization Detective)

I don't understand. Specific files are being loaded here that only matter for performance-lab.

Oh right. At least these should be wrapped in a condition so they only are loaded if the $plugin_name is performance-lab

Oh, but that already it the case. This logic is wrapped in:

if ( 'performance-lab' === $plugin_name ) {

Co-authored-by: Felix Arntz <flixos90@gmail.com>

ShyamGadde added 4 commits January 16, 2025 09:52

Move bootstrap.php to tools/phpunit dir

1eeeb61

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

Ignore PHPCS errors in test files

2d62b91

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

Move test images to plugin specific directories

3a724c8

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

Move Optimization Detective test helpers to its plugin specific direc…

8679cba

…tory Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

ShyamGadde added 2 commits January 21, 2025 15:08

Remove non-existent tests/multisite.xml from workflow paths

e3f3b3e

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

Merge branch 'trunk' into update/remove-root-tests-dir

ca4f56a

Merge branch 'trunk' into update/remove-root-tests-dir

f829057

ShyamGadde marked this pull request as ready for review January 24, 2025 17:10

ShyamGadde requested review from adamsilverstein, felixarntz, joemcgill, mukeshpanchal27, pbearne, thelovekesh and westonruter as code owners January 24, 2025 17:10

felixarntz reviewed Jan 27, 2025

View reviewed changes

westonruter and others added 4 commits January 27, 2025 11:08

Restore EOF EOL

2fa9d60

Merge branch 'trunk' into update/remove-root-tests-dir

7098a50

Rename TESTS_PLUGIN_DIR to REPO_ROOT_DIR to reflect its actual intent

fcaa10e

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

Re-introduce TESTS_PLUGIN_DIR

9f563d5

TESTS_PLUGIN_DIR is now dynamic and points to each plugins tests directory. Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

ShyamGadde requested a review from felixarntz January 27, 2025 21:32

felixarntz reviewed Jan 27, 2025

View reviewed changes

Adjust TESTS_PLUGIN_DIR to point to plugin root directory

70de425

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

ShyamGadde commented Jan 28, 2025

View reviewed changes

westonruter reviewed Jan 28, 2025

View reviewed changes

ShyamGadde added 4 commits January 29, 2025 00:08

Merge branch 'trunk' into update/remove-root-tests-dir

2a647be

Prefix REPO_ROOT_DIR with TESTS_ for clarity

6df3ca2

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

Fix PHP fatal error caused by duplicate trait declaration

b002bff

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

Fix missing constants for PHPStan

a4b05d2

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

westonruter reviewed Jan 28, 2025

View reviewed changes

westonruter and others added 3 commits January 28, 2025 11:10

Remove obsolete bootstrap logic missed in WordPress#1476

6421043

Replace relative paths with TESTS_REPO_ROOT_DIR

c29d670

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

Fix incorrect plugin path

6d26270

Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>

westonruter approved these changes Jan 28, 2025

View reviewed changes

westonruter requested a review from felixarntz January 28, 2025 19:37

felixarntz approved these changes Jan 28, 2025

View reviewed changes

Use TESTS_PLUGIN_DIR constant in uninstall tests

a985976

Co-authored-by: Felix Arntz <flixos90@gmail.com>

westonruter merged commit d94977f into WordPress:trunk Jan 28, 2025

	// Check if we use the plugin's test suite. If so, disable the PL plugin and only load the requested plugin.
	$plugin_name = '';
	foreach ( $_SERVER['argv'] as $index => $arg ) {
	if (
	'--testsuite' === $arg &&
	isset( $_SERVER['argv'][ $index + 1 ] ) &&
	file_exists( TESTS_PLUGIN_DIR . '/plugins/' . $_SERVER['argv'][ $index + 1 ] )
	) {
	$plugin_name = $_SERVER['argv'][ $index + 1 ];
	}
	}

	// Set default plugin to load if not specified.
	if ( '' === $plugin_name ) {
	$plugin_name = 'performance-lab';
	}

	define( 'TESTS_PLUGIN_DIR', REPO_ROOT_DIR . "/plugins/$plugin_name/tests" );
	define( 'TESTS_PLUGIN_DIR', REPO_ROOT_DIR . "/plugins/$plugin_name" );

		@@ -7,3 +7,6 @@

		// Require the suggested plugin.
		require_once __DIR__ . '/../../optimization-detective/load.php';

	require_once __DIR__ . '/../../optimization-detective/load.php';
	require_once TESTS_REPO_ROOT_DIR . '/plugins/optimization-detective/load.php';

	$plugin_test_path = TESTS_REPO_ROOT_DIR . '/plugins/' . $plugin_name;
	$plugin_test_path = TESTS_PLUGIN_DIR;

Uh oh!

Conversation

ShyamGadde commented Jan 16, 2025

Summary

Relevant technical choices

codecov Bot commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

westonruter commented Jan 24, 2025

ShyamGadde commented Jan 24, 2025

github-actions Bot commented Jan 24, 2025

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

3 participants

codecov Bot commented Jan 16, 2025 •

edited

Loading

felixarntz Jan 27, 2025 •

edited

Loading

westonruter Jan 28, 2025 •

edited

Loading

westonruter Jan 28, 2025 •

edited

Loading