Ticket 49631 - Test coverage for media_sideload_image source_url meta - #190
Ticket 49631 - Test coverage for media_sideload_image source_url meta#190killua99 wants to merge 5 commits into
Conversation
joemcgill
left a comment
joemcgill
left a comment
There was a problem hiding this comment.
This is starting to look really good, thanks for working on it. I've left a few comments that I think need to be adjusted before this is ready.
| self::$different_post = $factory->post->create_and_get(); | ||
| } | ||
|
|
||
| public function wpTearDownAfterClass() { |
There was a problem hiding this comment.
I don't think this teardown is necessary. WordPress should handle cleaning up these fixtures itself.
There was a problem hiding this comment.
Actually analyzing others wpTearDownAfterClass I've to be more radical and delete the post I did create.
| add_filter( 'pre_http_request', [ $this, 'internal_fake_download_url' ], 10, 3 ); | ||
| } | ||
|
|
||
| public function tearDown() { |
There was a problem hiding this comment.
This teardown is also not needed. The reason you need to add the filter in setUp() is because it runs before every test method and filters get reset between each test method so the filter will already be removed, and WP should clean up uploads itself, I believe.
There was a problem hiding this comment.
Check the teardown on the Unit Test Tests_Post_Attachments they do the same to clear the file system. Theme Dir they remove filter on tearDown.
More specific this method $this->remove_added_uploads();
| // Just need an image content string to fill the new image, keeping the same mime type. | ||
| file_put_contents( $parsed_args['filename'], file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); | ||
|
|
||
| return [ |
There was a problem hiding this comment.
This seems incorrect to me. Based on my reading on where this filter should be called, I think that this would need to return more than just a response code in order to test this accurately unless I'm missing something.
There was a problem hiding this comment.
The hook could return a:
// An array containing 'headers', 'body', 'response', 'cookies', and 'filename' elementsThen the function download_url does need a 200 response only to continue.
if ( 200 != $response_code ) {So only by shocking (applying a short-circuit as class-http.php said) on the request I mock a 200 response and just copy the content into the tmp-name file where the process will continue and save the attachment and added to the post.
| /** | ||
| * @ticket 49631 | ||
| */ | ||
| public function test_media_sideload_image() { |
There was a problem hiding this comment.
This should be broken up into several separate test methods—one for each assertion being made—or to refactor this to work with a data provider so you can run multiple independent tests by passing different inputs to a single test method.
Also, through each of these assertions, the "expected" value should be passed as the first parameter and the value being tested should be second.
There was a problem hiding this comment.
I agree this could be broken in several tests. I thought was for the same purpose, test the return value of the function and the meta of the post attached.
I'm pushing a change with the latest assert that I mix the order 👍
|
Closing in favour of #13397 |
3 participants
Trac ticket: https://core.trac.wordpress.org/ticket/49631