Bug/improvement: The pattern in the dominant-color-images plugin's test functions doesn't make sense

[nickchomey]

Bug Description

For example, the test_get_dominant_color_valid() function in the dominant-color-images plugin doesn't make sense to me:

public function test_get_dominant_color_valid( string $image_path, array $expected_color, bool $expected_transparency ): void {
	$mime_type = wp_check_filetype( $image_path )['type'];
	if ( ! wp_image_editor_supports( array( 'mime_type' => $mime_type ) ) ) {
		$this->markTestSkipped( "Mime type $mime_type is not supported." );
	}

	$attachment_id = self::factory()->attachment->create_upload_object( $image_path );
	wp_maybe_generate_attachment_metadata( get_post( $attachment_id ) );

	$dominant_color_data = dominant_color_get_dominant_color_data( $attachment_id );

	$this->assertNotWPError( $dominant_color_data );
	$this->assertContains( $dominant_color_data['dominant_color'], $expected_color );
	$this->assertSame( $dominant_color_data['has_transparency'], $expected_transparency );
}

It calls create_upload_object(), which ultimately calls dominant_color_get_dominant_color_data(). Here's the callstack.

dominant_color_get_dominant_color_data (performance/plugins/dominant-color-images/helper.php:160)
dominant_color_metadata (performance/plugins/dominant-color-images/hooks.php:32)
WP_Hook->apply_filters (/var/www/html/wp-includes/class-wp-hook.php:343)
apply_filters (/var/www/html/wp-includes/plugin.php:205)
wp_generate_attachment_metadata (/var/www/html/wp-admin/includes/image.php:750)
WP_UnitTest_Factory_For_Attachment->create_upload_object (/wordpress-phpunit/includes/factory/class-wp-unittest-factory-for-attachment.php:96)
Dominant_Color_Images\Tests\TestCase->test_get_dominant_color_valid (performance/plugins/dominant-color-images/tests/data/class-testcase.php:184)

Then it calls wp_maybe_generate_attachment_metadata(), which I put breakpoints in and confirmed that it never returns anything, because the metadata is already created. If, for whatever reason the metadata wasn't created, I suppose that would ultimately also call dominant_color_get_dominant_color_data() via the same hook sequence.

And then we call dominant_color_get_dominant_color_data() directly, which regenerates the dominant_color_data yet again.

So, triple redundancy... Moreover, the ultimate test assertion is from a direct call to the function, instead of testing if it is all wired up properly to automatically generate everything when creating an attachment. Seems like cheating.

It seems like it should look something like this:

public function test_get_dominant_color_valid( string $image_path, array $expected_color, bool $expected_transparency ): void {
	$mime_type = wp_check_filetype( $image_path )['type'];
	if ( ! wp_image_editor_supports( array( 'mime_type' => $mime_type ) ) ) {
		$this->markTestSkipped( "Mime type $mime_type is not supported." );
	}

	$attachment_id = self::factory()->attachment->create_upload_object( $image_path );
	$dominant_color_data = wp_get_attachment_metadata( $attachment_id );

	$this->assertNotWPError( $dominant_color_data );
	$this->assertContains( $dominant_color_data['dominant_color'], $expected_color );
	$this->assertSame( $dominant_color_data['has_transparency'], $expected_transparency );
}

Same thing for the other functions in that file: test_get_dominant_color_invalid, test_get_dominant_color_data_unsupported_mime_type, and test_get_dominant_color_none_images

If you agree, I'll submit a PR that fixes it.

[nickchomey]

Same pattern is used in https://github.com/WordPress/performance/blob/trunk/plugins/dominant-color-images/tests/test-dominant-color.php, except it calls dominant_color_metadata(), which then calls dominant_color_get_dominant_color_data().

And, strangely, the various WP_Errors from dominant_color_get_dominant_color_data are only used by tests as they are ultimately discarded by the dominant_color_metadata wrapper.