Add test coverage for AMP_Theme_Support#1034
Merged
westonruter merged 23 commits into0.7from Apr 3, 2018
Merged
Conversation
f76184c to
3c1f03d
Compare
02a2088 to
af899c2
Compare
…ed (and fail) for unknown reason See xwp/wp-dev-lib#270
af899c2 to
97dcdf1
Compare
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
suggested changes
Mar 29, 2018
Contributor
kienstra
left a comment
There was a problem hiding this comment.
Some Minor Points
Hi @DavidCramer,
Good work on these tests. Could you please address these minor points?
| * @return array Args to return. | ||
| */ | ||
| public static function amp_set_comments_walker( $args ) { | ||
| public static function set_comments_walker( $args ) { |
Contributor
There was a problem hiding this comment.
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 );| * 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' ) ) { |
Contributor
There was a problem hiding this comment.
Good idea to make testing easier.
| * @covers AMP_Theme_Support::start_output_buffering() | ||
| */ | ||
| public function test_start_output_buffering() { | ||
|
|
Contributor
There was a problem hiding this comment.
Could you please review this empty line?
| * @covers AMP_Theme_Support::finish_output_buffering() | ||
| */ | ||
| public function test_finish_output_buffering() { | ||
|
|
Contributor
There was a problem hiding this comment.
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.
Contributor
|
Request For Review Hi @westonruter, @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! |
kienstra
approved these changes
Apr 2, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #868.