Added tests for described Exoskeleton functionality#8
Added tests for described Exoskeleton functionality#8anigeluk wants to merge 20 commits intoAutomattic:masterfrom
Conversation
…revent dying during testing
mdbitz
left a comment
mdbitz
left a comment
There was a problem hiding this comment.
Hi Sarah:
I want through the tests with a fine tooth comb :) I really like the coverage these tests give but we could clean it up a little further for the merge.
| /** | ||
| * Test adding a valid rule with multiple methods | ||
| * | ||
| * @dataProvider validRuleProvider |
There was a problem hiding this comment.
This test validates the case of having comma separated method provided. Using a dataProvider doesn't provide any additional benefit. I would suggest switching to getValidRuleHelper.
| /** | ||
| * Pre test setup | ||
| */ | ||
| function setUp() { |
There was a problem hiding this comment.
Setup isn't providing setup of any data. We should be cleaning rule sets as part of TearDown vs setUp e.g. cleanup after vs before a test.
| /** | ||
| * Check that exoskeleton will not overwrite an already existing rule | ||
| * | ||
| * @dataProvider validRuleProvider |
There was a problem hiding this comment.
DataProvider usage here doesn't provide additional coverage we could remove this as additional executions on different rules don't provide benefit.
| * Test adding a valid rule for a custom route | ||
| * Exoskeleton makes no check for existence of custom route. | ||
| * | ||
| * @dataProvider validRuleProvider |
There was a problem hiding this comment.
coverage is same if change this to a single test without a dataProvider
| parent::setUp(); | ||
|
|
||
| // Clear existing rules. | ||
| $instance = Exoskeleton::get_instance(); |
There was a problem hiding this comment.
Since this is an instance, we should do clean up in tearDown vs the setUp
| /** | ||
| * Check that exoskeleton validates rules with an invalid rule | ||
| * | ||
| * @dataProvider validRuleProvider |
There was a problem hiding this comment.
Best Practices is to use datProviders for multiple test points. Using them for a single item is better suited for a helper/util function so that it is easier to understand the flow.
| * | ||
| * @var array Three valid rule definitions | ||
| */ | ||
| protected static $three_valid_rules; |
There was a problem hiding this comment.
per my review this is no longer utilized. If that is true we should clean it up/remove.
There was a problem hiding this comment.
You are correct :) I should have removed this when switching to data providers.
|
Thanks for taking the time to implement my suggestions. These look good to merge I'm just going to leave it open for Josh to make the final call. |
| @header( "Retry-After: $retry_after" ); | ||
|
|
||
| // Do not die if testing. | ||
| if ( defined( 'PHPUNIT_EXOSKELETON_TESTING' ) ) { |
There was a problem hiding this comment.
Does it make sense to calculate the retry after headers in a different function that would be easier to test? Then we could just output the headers and exit here without altering any logic for tests.
|
I think it would be good to avoid changing the logic for tests, but otherwise looks good to me. |
3 participants
There are failing tests in place for functionality described in the readme which isn't currently supported by the existing code.
I have not been able to test the 429 header and the retry-after headers are successfully set in my testing environment.