WPHookHelper::get_hook_name_param(): add basic tests#2727
Conversation
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
@rodrigoprimo Thanks for creating this PR. I've left some small suggestions inline, but, and more importantly, a principle question about how the tests for this method are set up.
| * @dataProvider dataGetHookNameParam | ||
| * | ||
| * @param string $functionName The function name to test. | ||
| * @param array<int, mixed> $parameters The parameters array to pass to the method. |
There was a problem hiding this comment.
| * @param array<int, mixed> $parameters The parameters array to pass to the method. | |
| * @param array<int|string, array<string>> $parameters The parameters array to pass to the method. |
| * | ||
| * @param string $functionName The function name to test. | ||
| * @param array<int, mixed> $parameters The parameters array to pass to the method. | ||
| * @param array<int, mixed>|false $expectedResult The expected return value. |
There was a problem hiding this comment.
| * @param array<int, mixed>|false $expectedResult The expected return value. | |
| * @param array<string>|false $expectedResult The expected return value. |
| * | ||
| * @see testGetHookNameParam() | ||
| * | ||
| * @return array<string, array<string, array<int, mixed>|false|string>> |
There was a problem hiding this comment.
| * @return array<string, array<string, array<int, mixed>|false|string>> | |
| * @return array<string, array<string, array<int|string, string|array<string>>|false|string>> |
| // Note: The parameter arrays use a simplified format instead of the real PassedParameters::getParameters() | ||
| // output as most of the array entries are not relevant for the method under test. |
There was a problem hiding this comment.
Instead of emulating the return value from PassedParameters::getParameters(), why not test the method with a case file and then let the test code call PassedParameters::getParameters() before calling WPHookHelper::get_hook_name_param() ?
| 'hook_name_param_missing' => array( | ||
| 'functionName' => 'apply_filters_ref_array', | ||
| 'parameters' => array( 2 => array( '$arg' ) ), | ||
| 'expectedResult' => false, | ||
| ), |
There was a problem hiding this comment.
This is an unrealistic test case as you can never have a parameters array with a second (positional) parameter, but not a first.
"Hook name param missing" is something which can only occur in function calls with named parameters.
|
Thanks for the review, Juliette!
I reworked the tests in a new commit to address your feedback:
I also addressed the other comments you left. The unrealistic "hook name param missing" case was replaced with a named-parameter call that omits the hook name parameter, and the docblock types were updated to reflect the new structure after refactoring the tests. |
2 participants
Description
In preparation for supporting PHPCS 4.0, this PR adds unit tests for the
WPHookHelper::get_hook_name_param()method.Suggested changelog entry
N/A
Related issues/external references
Related to: #2272