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 Speculation Rules Plugin #1845

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugins/speculation-rules/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
if ( ! defined( 'ABSPATH' ) ) {
exit; // Exit if accessed directly.
}
// @codeCoverageIgnoreEnd

(
/**
Expand Down Expand Up @@ -83,3 +82,4 @@ static function ( string $version ): void {
require_once __DIR__ . '/settings.php';
}
);
// @codeCoverageIgnoreEnd
148 changes: 148 additions & 0 deletions plugins/speculation-rules/tests/test-speculation-rules-settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,152 @@ public function test_plsr_add_settings_action_link(): void {
plsr_add_settings_action_link( $default_action_links )
);
}

/**
* @covers ::plsr_get_mode_labels
* @covers ::plsr_get_eagerness_labels
* @covers ::plsr_get_setting_default
* @covers ::plsr_register_setting
*/
public function test_register_settings(): void {
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 better name this test_plsr_register_setting in accordance with the primary method it tests.

Suggested change
public function test_register_settings(): void {
public function test_plsr_register_setting(): void {

plsr_register_setting();
$settings = plsr_get_setting_default();
$this->assertArrayHasKey( 'mode', $settings );
Copy link
Member

Choose a reason for hiding this comment

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

If we check this, we should probably also check for the eagerness key.

Suggested change
$this->assertArrayHasKey( 'mode', $settings );
$this->assertArrayHasKey( 'mode', $settings );
$this->assertArrayHasKey( 'eagerness', $settings );

// Test default settings applied correctly.
$default_settings = plsr_get_setting_default();
$this->assertEquals( $default_settings, get_option( 'plsr_speculation_rules' ) );
}

/**
* @covers ::plsr_get_stored_setting_value
*/
public function test_get_stored_setting_value(): void {
update_option(
'plsr_speculation_rules',
array(
'mode' => 'prefetch',
'eagerness' => 'moderate',
)
);
$settings = plsr_get_stored_setting_value();
$this->assertEquals(
array(
'mode' => 'prefetch',
'eagerness' => 'moderate',
),
$settings
);

// Test default when no option is set.
delete_option( 'plsr_speculation_rules' );
$settings = plsr_get_stored_setting_value();
$this->assertEquals( plsr_get_setting_default(), $settings );
}

/**
* Function to test sanitize_setting() with various inputs.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Function to test sanitize_setting() with various inputs.
* Function to test sanitize_setting() with various inputs.
*
* @covers ::plsr_sanitize_setting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I kept the @covers annotation just like suggested changes however that's resulting in reduced code coverage.
If covers annotation is not there it automatically finds the function that is being tested.
However if we are specifying like this then coverage reduced. Also tested with ClassName::FunctionName annotation.

  • Without coverage annotation :
image image
  • With coverage annotation :
image image

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.

Strange. I don't know why that would be.

Nevertheless, I see that the test_plsr_sanitize_setting() test is already covering plsr_sanitize_setting. Therefore, do we even need this test_sanitize_setting test? Should its test cases not be put into the data_plsr_sanitize_setting data provider?

*/
public function test_sanitize_setting(): void {
Copy link
Member

Choose a reason for hiding this comment

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

See above:

Suggested change
public function test_sanitize_setting(): void {
public function test_plsr_sanitize_setting(): void {

$input = array(
'mode' => 'prerender',
'eagerness' => 'eager',
);
$sanitized = plsr_sanitize_setting( $input );
$this->assertEquals( $input, $sanitized );

$input = array(
'mode' => 'invalid_mode',
'eagerness' => 'conservative',
);
$sanitized = plsr_sanitize_setting( $input );
$this->assertEquals( 'prerender', $sanitized['mode'] );

$input = array(
'mode' => 'prefetch',
'eagerness' => 'invalid_eagerness',
);
$sanitized = plsr_sanitize_setting( $input );
$this->assertEquals( 'moderate', $sanitized['eagerness'] );

$input = 'invalid_input';
$sanitized = plsr_sanitize_setting( $input );
$this->assertEquals( plsr_get_setting_default(), $sanitized );
}

/**
* @covers ::plsr_add_setting_ui
*/
public function test_add_setting_ui(): void {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function test_add_setting_ui(): void {
public function test_plsr_add_setting_ui(): void {

do_action( 'load-options-reading.php' );// phpcs:ignore WordPress.NamingConventions.ValidHookName.UseUnderscores

// Check if the settings section has been added.
global $wp_settings_sections;
$this->assertArrayHasKey( 'reading', $wp_settings_sections );
$this->assertArrayHasKey( 'plsr_speculation_rules', $wp_settings_sections['reading'] );

// Check the output of the callback function for the section.
$output = get_echo( $wp_settings_sections['reading']['plsr_speculation_rules']['callback'] );
$this->assertStringContainsString( 'This section allows you to control how URLs that your users navigate to are speculatively loaded to improve performance.', $output );
}

/**
* Data provider for testing plsr_render_settings_field.
*
* @return array<string, array<mixed>> Data for testing settings fields.
*/
public function data_provider_to_test_render_settings_field(): array {
return array(
'mode' => array(
array(
'field' => 'mode',
'title' => 'Speculation Mode',
'description' => 'Prerendering will lead to faster load times than prefetching.',
),
array(
'mode' => 'prefetch',
'eagerness' => 'moderate',
),
'name="plsr_speculation_rules[mode]"',
'value="prefetch"',
'Prerendering will lead to faster load times than prefetching.',
),
'eagerness' => array(
array(
'field' => 'eagerness',
'title' => 'Eagerness',
'description' => 'The eagerness setting defines the heuristics based on which the loading is triggered.',
),
array(
'mode' => 'prefetch',
'eagerness' => 'moderate',
),
'name="plsr_speculation_rules[eagerness]"',
'value="moderate"',
'The eagerness setting defines the heuristics based on which the loading is triggered.',
),
);
}

/**
* Test rendering of settings fields using data provider.
*
* @dataProvider data_provider_to_test_render_settings_field
* @param array<mixed> $args Arguments for the settings field.
* @param array<mixed> $stored_settings Stored settings values.
* @param string $name_check HTML name attribute check.
* @param string $value_check HTML value attribute check.
* @param string $description_check Description check.
*/
public function test_render_settings_field( array $args, array $stored_settings, string $name_check, string $value_check, string $description_check ): void {
// Simulate getting stored settings.
update_option( 'plsr_speculation_rules', $stored_settings );

// Capture the output of the settings field rendering.
$output = get_echo( 'plsr_render_settings_field', array( $args ) );

// Check for the presence of form elements.
$this->assertStringContainsString( $name_check, $output );
$this->assertStringContainsString( $value_check, $output );
$this->assertStringContainsString( $description_check, $output );
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ private function require_uninstall(): void {

/**
* Test option deletion.
*
* @covers ::plsr_delete_plugin_option
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this? The function is located in uninstall.php so when that file is being required, then that function is indeed being loaded and executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned here #1845 (comment)
for some functions adding @Covers annotation is reducing code coverage and exactly why it's happening is still not known. I thought it might be a case where some functions are not accessible (private or protected) but tested that as well, it's happening for public functions as well.

Without adding @covers ::plsr_delete_plugin_option

image image

After adding @covers ::plsr_delete_plugin_option

image image

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.

Strange!

*/
public function test_delete_plugin_option(): void {
unregister_setting( 'reading', 'plsr_speculation_rules' );
Expand Down
2 changes: 1 addition & 1 deletion plugins/speculation-rules/uninstall.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

// If uninstall.php is not called by WordPress, bail.
if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) {
exit;
exit;// @codeCoverageIgnore
}

// For a multisite, delete the option for all sites (however limited to 100 sites to avoid memory limit or timeout problems in large scale networks).
Expand Down
Loading