-
Notifications
You must be signed in to change notification settings - Fork 0
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
test : Add tests for Core Image
Block
#30
Conversation
1322f45
to
6da38bc
Compare
60ba97e
to
11e0a17
Compare
tests/unit/CoreImageTest.php
Outdated
final class CoreImageTest extends PluginTestCase { | ||
public $instance; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final class CoreImageTest extends PluginTestCase { | |
public $instance; | |
final class CoreImageTest extends PluginTestCase { | |
tests/unit/CoreImageTest.php
Outdated
* Get the query for the CoreImage block. | ||
* | ||
* @param string $attributes The attributes to add to query. | ||
*/ | ||
public function query( $attributes = '' ): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new param isnt in use, lets remove it.
(If it was in use, there'd be a longer discussion whether the ingenuity here outweights a general desire to not obfuscate whats getting tested. Either way, smart approach 🙌)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, i was actually using it to return a valid query for every untested attribute ( ) check by just passing that label to it. but later i thought to instead use the query in each case to make it more readable
tests/unit/CoreImageTest.php
Outdated
* - scale | ||
* - lightbox | ||
*/ | ||
public function test_retrieve_core_untested_attributes(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Break this into two separate tests please.
At the top of each, gate it and $this->markTestSkipped()
if its not the right WordPress version (like its done elsewhere), instead of wrapping the tests inside conditional logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just need to split the lightbox
(WP6.4) test from the WP6.3 ones, a bit of cleanup, and an updated PR description, and this should be good to go.
Change the PR from draft to ready for review once the above is done and I'll give my final signoff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Go ahead and open this up on WPE
May I proceed to merge this pull request ? @justlevine 😃 |
Tracking wpengine#306