Add test coverage for AMP_Theme_Support #1034

westonruter · 2018-03-22T01:19:47Z

Fixes #868.

… interface

…support class

…ed (and fail) for unknown reason See xwp/wp-dev-lib#270

@todo

Test init() and redirect_canonical_amp(). @todo continue with these, and consider a better way to test redirect_canonical_amp(). That uses wp_safe_redirect, which throws an error.

is_customize_preview_iframe() and register_paired_hooks().

Test add_hooks, send_header(), and register_content_embed_handlers().

Test methods related to AMP templates, and get_current_canonical_url().

Test 2 methods: get_comment_form_state_id() and filter_comment_form_defaults().

There's a check for is_customize_preview() around add_action(). But the callback dequeue_customize_preview_scripts() has its own check for is_customize_preview_iframe(). And that has a check for is_customize_preview(). So this ensures that the body of the callback never runs. Maybe I misread the intent of this, though. This also adds unit tests.

These variables are only used once, so they can simply be passed to the array.

There's already a phpcs:disable at the top of the method. And a phpcs:enable at the bottom.

kienstra

Some Minor Points

Hi @DavidCramer,
Good work on these tests. Could you please address these minor points?

kienstra · 2018-03-28T22:12:40Z

includes/class-amp-theme-support.php

@@ -631,7 +629,7 @@ public static function register_content_embed_handlers() {
 	 * @param array $args the args for the comments list..
 	 * @return array Args to return.
 	 */
-	public static function amp_set_comments_walker( $args ) {
+	public static function set_comments_walker( $args ) {


It looks like this method's name is now different from that in the add_action() call:

add_filter( 'wp_list_comments_args', array( __CLASS__, 'amp_set_comments_walker' ), PHP_INT_MAX );

kienstra · 2018-03-28T22:13:22Z

includes/class-amp-theme-support.php

@@ -999,7 +997,7 @@ public static function start_output_buffering() {
 		 * Sites with New Relic will need to specially configure New Relic for AMP:
 		 * https://docs.newrelic.com/docs/browser/new-relic-browser/installation/monitor-amp-pages-new-relic-browser
 		 */
-		if ( extension_loaded( 'newrelic' ) ) {
+		if ( function_exists( 'newrelic_disable_autorum' ) ) {


Good idea to make testing easier.

kienstra · 2018-03-28T22:28:23Z

tests/test-class-amp-theme-support.php

+	 * @covers AMP_Theme_Support::start_output_buffering()
+	 */
+	public function test_start_output_buffering() {
+


Could you please review this empty line?

kienstra · 2018-03-28T22:29:09Z

tests/test-class-amp-theme-support.php

+	 * @covers AMP_Theme_Support::finish_output_buffering()
+	 */
+	public function test_finish_output_buffering() {
+


Could you please remove this empty line, and the empty line at the bottom of the function, before the closing }? And please do the same for test_filter_customize_partial_render().

There was a minor conflict in: test-class-amp-theme-support.php. Retain both edits.

Before, merging in 0.7, there was an amp-default <link>. But test_validate_non_amp_theme() also output that. And that stored the script in $wp_styles->done. So it wasn't output again, and the test failed. So reset $wp_styles, so it gets re-instantiated.

kienstra · 2018-03-31T00:46:14Z

Request For Review

Hi @westonruter,
No hurry, as this is a holiday. But could you please review this PR for #868?

@DavidCramer's and your commits passed my review. But it'd be great to have you review my commits, if you have time.

This commit is the only substantive change to a non-test file.

Thanks, and have a great weekend!

westonruter added this to the v0.7 milestone Mar 22, 2018

westonruter force-pushed the add/test-coverage branch from f76184c to 3c1f03d Compare March 22, 2018 04:07

postphotos added this to Definition in v1.0 Mar 22, 2018

westonruter added 4 commits March 26, 2018 11:02

Fix phpunit warnings regarding use of @cover fornon-existing class or…

ce66658

… interface

Add stubs for missing tests; re-order tests to mirror order in theme …

1fe5527

…support class

Remove needless amp prefix from set_comments_walker method

7232ab5

Add test for AMP_Theme_Support::set_comments_walker()

b3c6402

westonruter force-pushed the add/test-coverage branch 4 times, most recently from 02a2088 to af899c2 Compare March 26, 2018 18:38

Skip printing paths-scope because it causes push builds to be truncat…

97dcdf1

…ed (and fail) for unknown reason See xwp/wp-dev-lib#270

westonruter force-pushed the add/test-coverage branch from af899c2 to 97dcdf1 Compare March 26, 2018 18:44

Ryan Kienstra and others added 15 commits March 27, 2018 16:05

Add 2 PHPUnit tests for AMP_Theme_Support.

4c17c67

Test init() and redirect_canonical_amp(). @todo continue with these, and consider a better way to test redirect_canonical_amp(). That uses wp_safe_redirect, which throws an error.

Test 2 more methods in AMP_Theme_Support.

4d70aa9

is_customize_preview_iframe() and register_paired_hooks().

Test 3 more methods in AMP_Theme_Support.

ac6cac1

Test add_hooks, send_header(), and register_content_embed_handlers().

Test 3 more methods in AMP_Theme_Support.

92ada3b

Test methods related to AMP templates, and get_current_canonical_url().

Add comment form-related PHPUnit tests.

93bf043

Test 2 methods: get_comment_form_state_id() and filter_comment_form_defaults().

Remove test for renamed method, as test already exists.

f90c40a

Test start and finish output buffers.

912ea33

Add test for partial render and flesh out buffer tests.

e4dedbb

Correct assertion on has_action for shutdown

ce68491

use function_exists instead of extension_loaded to simplify testing.

9caf29b

define newrelic_disable_autorum to allow testing

cdb6bba

more coverage for intercept_post_request_redirect

1b75fae

Remove extra variable declarations.

2540e86

These variables are only used once, so they can simply be passed to the array.

Remove extra phpcs:ignore comments.

86f13fb

There's already a phpcs:disable at the top of the method. And a phpcs:enable at the bottom.

kienstra suggested changes Mar 29, 2018

View reviewed changes

Address issues from CR

b365e00

Merge in 0.7, resolve conflicts.

9e87ea6

There was a minor conflict in: test-class-amp-theme-support.php. Retain both edits.

kienstra mentioned this pull request Mar 30, 2018

Issue #868: Add Comment Unit Tests for PR #909 #1049

Merged

kienstra changed the title ~~[WIP] Add test coverage for AMP_Theme_Support~~ Add test coverage for AMP_Theme_Support Apr 2, 2018

kienstra approved these changes Apr 2, 2018

View reviewed changes

kienstra assigned westonruter Apr 2, 2018

westonruter merged commit dc51778 into 0.7 Apr 3, 2018

westonruter deleted the add/test-coverage branch April 3, 2018 06:55

kienstra removed this from Definition in v1.0 Apr 3, 2018

kienstra added this to Ready for Merging in v0.7 Apr 3, 2018

MackenzieHartung added the Sprint 2 label Apr 9, 2018

kevincoleman moved this from Ready for Merging to Production Release in v0.7 May 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test coverage for AMP_Theme_Support #1034

Add test coverage for AMP_Theme_Support #1034

westonruter commented Mar 22, 2018

kienstra left a comment

kienstra Mar 28, 2018

kienstra Mar 28, 2018

kienstra Mar 28, 2018

kienstra Mar 28, 2018

kienstra commented Mar 31, 2018

Add test coverage for AMP_Theme_Support #1034

Add test coverage for AMP_Theme_Support #1034

Conversation

westonruter commented Mar 22, 2018

kienstra left a comment

Choose a reason for hiding this comment

kienstra Mar 28, 2018

Choose a reason for hiding this comment

kienstra Mar 28, 2018

Choose a reason for hiding this comment

kienstra Mar 28, 2018

Choose a reason for hiding this comment

kienstra Mar 28, 2018

Choose a reason for hiding this comment

kienstra commented Mar 31, 2018