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

Add test coverage for AMP_Theme_Support #1034

Merged
merged 23 commits into from
Apr 3, 2018
Merged

Add test coverage for AMP_Theme_Support #1034

merged 23 commits into from
Apr 3, 2018

Conversation

westonruter
Copy link
Member

Fixes #868.

@westonruter westonruter added this to the v0.7 milestone Mar 22, 2018
@westonruter westonruter force-pushed the add/test-coverage branch 4 times, most recently from 02a2088 to af899c2 Compare March 26, 2018 18:38
Ryan Kienstra and others added 15 commits March 27, 2018 16:05
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.
Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Some Minor Points

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

@@ -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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 );

@@ -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' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to make testing easier.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please review this empty line?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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!

@kienstra kienstra changed the title [WIP] Add test coverage for AMP_Theme_Support Add test coverage for AMP_Theme_Support Apr 2, 2018
@westonruter westonruter merged commit dc51778 into 0.7 Apr 3, 2018
@westonruter westonruter deleted the add/test-coverage branch April 3, 2018 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants