Enhance/5450 twg tag infra #5565
Conversation
kuasha420
force-pushed
the
enhance/5450-twg-tag-infra
branch
from
a853289 to
3562c44
Compare
There was a problem hiding this comment.
Thanks, @kuasha420. This is a good start, but there are things that we need to improve before merging this PR. Please, check my comments and make appropriate changes.
| public function test_content_placeholder() { | ||
| remove_all_filters( 'the_content' ); | ||
|
|
||
| $this->thank_with_google->register(); | ||
| do_action( 'template_redirect' ); | ||
|
|
||
| $mock_content = '<p>Hello World</p>'; | ||
|
|
||
| $output = apply_filters( 'the_content', $mock_content ); | ||
|
|
||
| // Snippet should not be present on non singular pages. | ||
| $this->assertEquals( $output, $mock_content ); | ||
|
|
||
| $post_ID = $this->factory()->post->create(); | ||
| $this->go_to( get_permalink( $post_ID ) ); | ||
|
|
||
| $output = apply_filters( 'the_content', '<p>Hello World</p>' ); | ||
|
|
||
| $this->assertStringContainsString( 'Thank with Google snippet added by Site Kit', $output ); | ||
| $this->assertStringContainsString( '<button twg-button', $output ); |
There was a problem hiding this comment.
This should be moved into a new test file for the Web_Tag class and more tests should be added to cover all three use cases.
There was a problem hiding this comment.
I have moved it to separate file and added more tests. However, the tests are failing on PHP 5.6 and/or WP 4.7. Can you help me with it?
There was a problem hiding this comment.
@kuasha420, not sure why it doesn't work, maybe try to add 'post_status' => 'publish', when you generate a new post?
tests/phpunit/integration/Modules/Thank_With_Google/Web_TagTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks, @kuasha420. One more suggestion and this is good to go. I'll apply it by myself.
| public function set_publication_id( $publication_id ) { | ||
| $this->publication_id = $publication_id; | ||
| } |
There was a problem hiding this comment.
This method is unnecessary. The publication ID for this tag is the "main" ID to pass into the constructor, so it should rather be looked up from the property in the parent class. It's a bit redundant to set it here too.
There was a problem hiding this comment.
Good catch. I have created a follow up PR to address this: #5616
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist