Improve test coverage for Optimization Detective #1817
Conversation
westonruter
added
[Type] Enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #1817 +/- ##
==========================================
+ Coverage 60.83% 65.42% +4.58%
==========================================
Files 86 85 -1
Lines 6756 6738 -18
==========================================
+ Hits 4110 4408 +298
+ Misses 2646 2330 -316
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -346,6 +363,9 @@ public function test_add_link( array $links_args, string $expected_html, string | |||
| } | |||
|
|
|||
| $collection = new OD_Link_Collection(); | |||
|
|
|||
| $this->assertNull( $collection->get_response_header() ); | |||
There was a problem hiding this comment.
This call was added to improve test coverage and it exposed error in PHP 7.2/7.3 which is fixed in 921c2d3
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
@westonruter This looks great, thanks for all the additional coverage! Just a few non-blocking thoughts.
| 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 ); |
There was a problem hiding this comment.
What's the rationale for this change? How was the previous implementation problematic?
There was a problem hiding this comment.
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.
| // 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 ); |
There was a problem hiding this comment.
Too bad, I guess there is no way to manually indicate that this is covered by the tests for the hooks added?
There was a problem hiding this comment.
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
… update/od-test-coverage
westonruter
added
[Plugin] Optimization Detective
Summary
This is part of #1789:
@coversAnnotations