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 test coverage for Optimization Detective #1817

Merged
merged 9 commits into from
Jan 23, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,18 @@ public function add_link( array $attributes, ?int $minimum_viewport_width = null
* @return LinkAttributes[] Prepared links with adjacent-duplicates merged together and media attributes added.
*/
private function get_prepared_links(): array {
$links_by_rel = array_values( $this->links_by_rel );
if ( count( $links_by_rel ) === 0 ) {
// This condition is needed for PHP 7.2 and PHP 7.3 in which array_merge() fails if passed a spread empty array: 'array_merge() expects at least 1 parameter, 0 given'.
return array();
}

return array_merge(
...array_map(
function ( array $links ): array {
return $this->merge_consecutive_links( $links );
},
array_values( $this->links_by_rel )
$links_by_rel
)
);
}
Expand Down
26 changes: 15 additions & 11 deletions plugins/optimization-detective/class-od-strict-url-metric.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
* This is used exclusively in the REST API endpoint for capturing new URL Metrics to prevent invalid additional data from being
* submitted in the request. For URL Metrics which have been stored the looser OD_URL_Metric class is used instead.
*
* @phpstan-import-type JSONSchema from OD_URL_Metric
*
* @since 0.6.0
* @access private
*/
Expand All @@ -28,7 +30,7 @@
*
* @since 0.6.0
*
* @return array<string, mixed> Schema.
* @return JSONSchema Schema.
*/
public static function get_json_schema(): array {
return self::set_additional_properties_to_false( parent::get_json_schema() );
Expand All @@ -43,34 +45,36 @@
* @since 0.6.0
* @see rest_default_additional_properties_to_false()
*
* @param mixed $schema Schema.
* @return mixed Processed schema.
* @phpstan-param JSONSchema $schema
*
* @param array<string, mixed> $schema Schema.
* @return JSONSchema Processed schema.
*/
private static function set_additional_properties_to_false( $schema ) {
if ( ! isset( $schema['type'] ) ) {
return $schema;
}

private static function set_additional_properties_to_false( array $schema ): array {
$type = (array) $schema['type'];

if ( in_array( 'object', $type, true ) ) {
if ( isset( $schema['properties'] ) ) {
foreach ( $schema['properties'] as $key => $child_schema ) {
$schema['properties'][ $key ] = self::set_additional_properties_to_false( $child_schema );
if ( isset( $child_schema['type'] ) ) {
$schema['properties'][ $key ] = self::set_additional_properties_to_false( $child_schema );
}
}
}

if ( isset( $schema['patternProperties'] ) ) {
foreach ( $schema['patternProperties'] as $key => $child_schema ) {
$schema['patternProperties'][ $key ] = self::set_additional_properties_to_false( $child_schema );
if ( isset( $child_schema['type'] ) ) {
$schema['patternProperties'][ $key ] = self::set_additional_properties_to_false( $child_schema );

Check warning on line 68 in plugins/optimization-detective/class-od-strict-url-metric.php

View check run for this annotation

Codecov / codecov/patch

plugins/optimization-detective/class-od-strict-url-metric.php#L67-L68

Added lines #L67 - L68 were not covered by tests
Comment on lines -49 to +68
Copy link
Member

Choose a reason for hiding this comment

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

What's the rationale for this change? How was the previous implementation problematic?

Copy link
Member Author

Choose a reason for hiding this comment

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

There was no code coverage for the return in:

		if ( ! isset( $schema['type'] ) ) {
			return $schema;
		}

In practice, the $schema should always have a type specified given it is enforced by \OD_URL_Metric::extend_schema_with_optional_properties(). At the entrypoint where this method is called in \OD_Strict_URL_Metric::get_json_schema() it is absolutely defined, which the new typing makes clear. But there is less certainty about the nested properties, so this just moves the isset(...['type']) check down below, which has the same effect but also improves coverage.

}
}
}

$schema['additionalProperties'] = false;
}

if ( in_array( 'array', $type, true ) ) {
if ( isset( $schema['items'] ) ) {
if ( isset( $schema['items'], $schema['items']['type'] ) ) {
$schema['items'] = self::set_additional_properties_to_false( $schema['items'] );
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,9 +271,12 @@ public function add_url_metric( OD_URL_Metric $new_url_metric ): void {
return;
}
}
// @codeCoverageIgnoreStart
// In practice this exception should never get thrown because create_groups() creates groups from a minimum of 0 to a maximum of PHP_INT_MAX.
throw new InvalidArgumentException(
esc_html__( 'No group available to add URL Metric to.', 'optimization-detective' )
);
// @codeCoverageIgnoreEnd
}

/**
Expand All @@ -296,6 +299,8 @@ public function get_group_for_viewport_width( int $viewport_width ): OD_URL_Metr
return $group;
}
}
// @codeCoverageIgnoreStart
// In practice this exception should never get thrown because create_groups() creates groups from a minimum of 0 to a maximum of PHP_INT_MAX.
throw new InvalidArgumentException(
esc_html(
sprintf(
Expand All @@ -305,6 +310,7 @@ public function get_group_for_viewport_width( int $viewport_width ): OD_URL_Metr
)
)
);
// @codeCoverageIgnoreEnd
} )();

$this->result_cache[ __FUNCTION__ ][ $viewport_width ] = $result;
Expand Down
15 changes: 14 additions & 1 deletion plugins/optimization-detective/class-od-url-metric.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@
* viewport: ViewportRect,
* elements: ElementData[]
* }
* @phpstan-type JSONSchema array{
* type: string|string[],
* items?: mixed,
* properties?: array<string, mixed>,
* patternProperties?: array<string, mixed>,
* required?: bool,
* minimum?: int,
* maximum?: int,
* pattern?: non-empty-string,
* additionalProperties?: bool,
* format?: non-empty-string,
* readonly?: bool,
* }
*
* @since 0.1.0
* @access private
Expand Down Expand Up @@ -161,7 +174,7 @@ public function set_group( OD_URL_Metric_Group $group ): void {
*
* @todo Cache the return value?
*
* @return array<string, mixed> Schema.
* @return JSONSchema Schema.
*/
public static function get_json_schema(): array {
/*
Expand Down
4 changes: 4 additions & 0 deletions plugins/optimization-detective/hooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
}
// @codeCoverageIgnoreEnd

// The addition of the following hooks is tested in Test_OD_Hooks::test_hooks_added() and Test_OD_Storage_Post_Type::test_add_hooks().

// @codeCoverageIgnoreStart
add_action( 'init', 'od_initialize_extensions', PHP_INT_MAX );
Comment on lines +15 to 18
Copy link
Member

Choose a reason for hiding this comment

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

Too bad, I guess there is no way to manually indicate that this is covered by the tests for the hooks added?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the only way may be to have a test that removes all filters/actions, and then does a require on the hooks.php file. If after doing so all of the filters/actions have been added, then we know it worked. This would work for Optimization Detective, but it wouldn't work for other plugins that actually define functions in hooks.php. So maybe we should consider that later as part of #1789.

Note that the coverage of uninstall.php is handled similarly: https://github.com/WordPress/performance/blob/trunk/plugins/optimization-detective/tests/test-uninstall.php

add_filter( 'template_include', 'od_buffer_output', PHP_INT_MAX );
OD_URL_Metrics_Post_Type::add_hooks();
Expand All @@ -20,3 +23,4 @@
add_filter( 'site_status_tests', 'od_add_rest_api_availability_test' );
add_action( 'admin_init', 'od_maybe_run_rest_api_health_check' );
add_action( 'after_plugin_row_meta', 'od_render_rest_api_health_check_admin_notice_in_plugin_row', 30 );
// @codeCoverageIgnoreEnd
4 changes: 2 additions & 2 deletions plugins/optimization-detective/storage/rest-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ function od_handle_rest_request( WP_REST_Request $request ) {
);
} catch ( InvalidArgumentException $exception ) {
// Note: This should never happen because an exception only occurs if a viewport width is less than zero, and the JSON Schema enforces that the viewport.width have a minimum of zero.
return new WP_Error( 'invalid_viewport_width', $exception->getMessage() );
return new WP_Error( 'invalid_viewport_width', $exception->getMessage() ); // @codeCoverageIgnore
}
if ( $url_metric_group->is_complete() ) {
return new WP_Error(
Expand Down Expand Up @@ -279,7 +279,7 @@ function od_handle_rest_request( WP_REST_Request $request ) {
* @since 0.8.0
* @access private
*
* @param int $cache_purge_post_id Cache purge post ID.
* @param positive-int $cache_purge_post_id Cache purge post ID.
*/
function od_trigger_page_cache_invalidation( int $cache_purge_post_id ): void {
$post = get_post( $cache_purge_post_id );
Expand Down
41 changes: 41 additions & 0 deletions plugins/optimization-detective/tests/storage/test-rest-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ static function ( array $properties ) use ( $property_name ): array {
* @covers ::od_register_endpoint
* @covers ::od_handle_rest_request
* @covers ::od_trigger_page_cache_invalidation
* @covers OD_Strict_URL_Metric::set_additional_properties_to_false
* @covers OD_URL_Metric_Store_Request_Context::__construct
*/
public function test_rest_request_good_params( Closure $set_up ): void {
$stored_context = null;
Expand Down Expand Up @@ -137,6 +139,28 @@ function ( OD_URL_Metric_Store_Request_Context $context ) use ( &$stored_context
}
}

/**
* Test good params.
*
* @dataProvider data_provider_to_test_rest_request_good_params
*
* @covers ::od_register_endpoint
* @covers ::od_handle_rest_request
* @covers OD_Strict_URL_Metric::set_additional_properties_to_false
*/
public function test_rest_request_good_params_but_post_save_failed( Closure $set_up ): void {
$valid_params = $set_up();

add_filter( 'wp_insert_post_empty_content', '__return_true' ); // Cause wp_insert_post() to return WP_Error.

$request = $this->create_request( $valid_params );
$response = rest_get_server()->dispatch( $request );

$error = $response->as_error();
$this->assertInstanceOf( WP_Error::class, $error );
$this->assertSame( 'unable_to_store_url_metric', $error->get_error_code() );
}

/**
* Data provider for test_rest_request_bad_params.
*
Expand Down Expand Up @@ -271,6 +295,7 @@ static function ( $params ) use ( $valid_params ) {
*
* @covers ::od_register_endpoint
* @covers ::od_handle_rest_request
* @covers OD_Strict_URL_Metric::set_additional_properties_to_false
*
* @dataProvider data_provider_invalid_params
*
Expand Down Expand Up @@ -652,6 +677,22 @@ static function ( string $hook, ...$args ) use ( &$all_hook_callback_args ): voi
$this->assertTrue( $found, 'Expected save_post to have been fired for the post queried object.' );
}

/**
* Test od_trigger_page_cache_invalidation() for an invalid post.
*
* @covers ::od_trigger_page_cache_invalidation
*/
public function test_od_trigger_page_cache_invalidation_invalid_post_id(): void {
wp_delete_post( 1, true );
$before_clean_post_cache_count = did_action( 'clean_post_cache' );
$before_transition_post_status_count = did_action( 'transition_post_status' );
$before_save_post_count = did_action( 'save_post' );
od_trigger_page_cache_invalidation( 1 );
$this->assertSame( $before_clean_post_cache_count, did_action( 'clean_post_cache' ) );
$this->assertSame( $before_transition_post_status_count, did_action( 'transition_post_status' ) );
$this->assertSame( $before_save_post_count, did_action( 'save_post' ) );
}

/**
* Populate URL Metrics.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<html lang="en">
<head>
<meta charset="utf-8">
<title>...</title>
</head>
<body>
<div id="page">
<img src="https://example.com/image.jpg" alt="" width="100" height="100">
</div>
</body>
</html>

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php
return static function (): void {

add_action(
'od_register_tag_visitors',
static function ( OD_Tag_Visitor_Registry $registry ): void {
$registry->register(
'img-preload',
static function ( OD_Tag_Visitor_Context $context ): bool {
if ( 'IMG' === $context->processor->get_tag() ) {
$context->link_collection->add_link(
array(
'rel' => 'preload',
'as' => 'image',
'href' => $context->processor->get_attribute( 'src' ),
)
);
}
return false;
}
);
}
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class Test_OD_Element extends WP_UnitTestCase {
/**
* Tests construction.
*
* @covers ::__construct
* @covers ::get
* @covers ::get_url_metric
* @covers ::get_url_metric_group
Expand Down
Loading
Loading