Add unit tests for the WP Upgrader class #1751
Conversation
Small code adjustments to have better type safety.
|
Hi @moorscode! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
There was a problem hiding this comment.
Thanks for submitting this PR @moorscode! A few thoughts are included in this review to improve the performance and standards within the tests.
| @@ -232,6 +232,10 @@ | |||
|
|
|||
| define( 'REST_TESTS_IMPOSSIBLY_HIGH_NUMBER', 99999999 ); | |||
|
|
|||
| if ( ! defined( 'FS_CHMOD_FILE' ) ) { | |||
| define( 'FS_CHMOD_FILE', 0644 ); | |||
There was a problem hiding this comment.
To make sure we're vetting all new globals, can you explain the rationale behind 1) Setting this global and 2) Doing so in the bootstrap file as opposed to the test class file? Is it needed and is it needed here?
| * Tests_Admin_wpUpgrader class. | ||
| * | ||
| * @group admin | ||
| * @group upgrading |
There was a problem hiding this comment.
There is another group in the test suite called upgrade. Would this be more appropriate than upgrading, or should they be separate groups?
| /** | ||
| * The init method should set up the class and skin. | ||
| */ | ||
| public function test_init() { |
There was a problem hiding this comment.
There are many similar assertions in this test method. Consider using a data provider for the assertNotEmpty() assertions.
For test methods that contain multiple assertions (in this case, assertNotEmpty() and two instances assertIsInt(), a message should be supplied so that we know which assertion failed. See here. The message parameter exists for both assertNotEmpty() and assertIsInt().
This also applies to the following additional test methods:
test_download_package_pre_download_argumentstest_download_package_empty_packagetest_download_package_wp_error
| * | ||
| * @return array[][] | ||
| */ | ||
| public function delete_temp_backup_invalid_arguments_dataprovider() { |
There was a problem hiding this comment.
Data providers should be prefixed with data_ and do not need the _dataprovider suffix. See here.
This also applies to the restore_temp_backup_invalid_arguments_dataprovider data provider.
| $callback = function ( $reply ) { | ||
| return ! $reply; | ||
| }; | ||
|
|
||
| add_filter( 'upgrader_pre_download', $callback, 10, 1 ); |
There was a problem hiding this comment.
This callback can be added directly to the add_filter() call.
- As
10and1are the default values for their respective arguments$priorityand$accepted_args, these are not necessary. See here. - As the callback does not use
$this, it should also be a static closure to improve performance. See here. remove_filter()does not need to be called on these, as the sharedtear_down()method will reset filters between each test. See here.
See an example of how this could be moved to a closure in add_filter():
add_filter(
'upgrader_pre_download',
static function( $reply ) {
return ! $reply;
}
);This also applies to the callbacks in the following test methods:
test_download_package_pre_download_arguments(notstaticas you need to use$this)test_download_packagetest_download_package_wp_error
This PR was created during the contributor day at Yoast on october 8th.
The focus of me and https://github.com/karlijnbok was to create test coverage for the WP Upgrader classes.
We've chosen the WP_Upgrader class to start off with, as this is a base class for different more specific upgrader classes.
As we both did not have a lot of recent experience in these classes, we decided to start with the simplest methods and work our way up.
We've added some small code adjustments to have better type safety.
The overal line-coverage of the class has increased to 24%.
We did not run branch or path coverage yet.
The
restore_temp_backuphas not fully been covered yet, running a coverage report will show this.Trac ticket: https://core.trac.wordpress.org/ticket/54245
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.