-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Ticket 49631 - Test coverage for media_sideload_image source_url meta #190
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
Changes from all commits
8dddfa2
5cc9c05
78330b2
6de360a
dadacc7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @group media | ||
| * @group admin | ||
| */ | ||
| class Tests_Admin_includesMedia extends WP_UnitTestCase { | ||
| /** @var \WP_Post */ | ||
| protected static $post; | ||
| /** @var \WP_Post */ | ||
| protected static $different_post; | ||
|
|
||
| public static function wpSetUpBeforeClass( \WP_UnitTest_Factory $factory ) { | ||
| self::$post = $factory->post->create_and_get(); | ||
| self::$different_post = $factory->post->create_and_get(); | ||
| } | ||
|
|
||
| public static function wpTearDownAfterClass() { | ||
| wp_delete_post( self::$post->ID, true ); | ||
| wp_delete_post( self::$different_post->ID, true ); | ||
| self::$post = null; | ||
| self::$different_post = null; | ||
| } | ||
|
|
||
| public function setUp() { | ||
| parent::setUp(); | ||
|
|
||
| add_filter( 'pre_http_request', [ $this, 'internal_fake_download_url' ], 10, 3 ); | ||
| } | ||
|
|
||
| public function tearDown() { | ||
| parent::tearDown(); | ||
|
|
||
| $this->remove_added_uploads(); | ||
| remove_filter( 'pre_http_request', [ $this, 'internal_fake_download_url' ] ); | ||
| } | ||
|
|
||
| /** | ||
| * Filters whether to preempt an HTTP request's return value. Mocking the | ||
| * response. Since ticket 49631 | ||
| * | ||
| * @param false $preempt Whether to preempt an HTTP request's return value. Default false. | ||
| * @param array $parsed_args HTTP request arguments. | ||
| * @param string $url The request URL. | ||
| * | ||
| * @return array | ||
| */ | ||
| public function internal_fake_download_url( $preempt, array $parsed_args, $url = '' ) { | ||
| // 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 [ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hook could return a: // An array containing 'headers', 'body', 'response', 'cookies', and 'filename' elementsThen the function 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. |
||
| 'response' => [ | ||
| 'code' => 200, | ||
| ], | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * @ticket 49631 | ||
| */ | ||
| public function test_media_sideload_image() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 👍 |
||
| $year = gmdate( 'Y' ); | ||
| $month = gmdate( 'm' ); | ||
|
|
||
| $image_source = 'http://' . WP_TESTS_DOMAIN . '/external/source/image1.jpg'; | ||
| $image = 'http://' . WP_TESTS_DOMAIN . "/wp-content/uploads/{$year}/{$month}/image1.jpg"; | ||
| $image_html = addslashes( $image ); | ||
|
|
||
| $media_html = media_sideload_image( $image_source, self::$post->ID ); | ||
|
|
||
| $this->assertNotWPError( $media_html ); | ||
| $this->assertRegExp( "~<img src='$image_html' alt='' \/>~", $media_html ); | ||
|
|
||
| $image_source = 'http://' . WP_TESTS_DOMAIN . '/external/source/image2.jpg'; | ||
| $image = 'http://' . WP_TESTS_DOMAIN . "/wp-content/uploads/{$year}/{$month}/image2.jpg"; | ||
|
|
||
| $media_src = media_sideload_image( $image_source, self::$different_post->ID, null, 'src' ); | ||
|
|
||
| $this->assertEquals( $image, $media_src ); | ||
|
|
||
| $image_source = 'http://' . WP_TESTS_DOMAIN . '/external/source/image3.jpg'; | ||
|
|
||
| $media_id = media_sideload_image( $image_source, self::$different_post->ID, null, 'id' ); | ||
| $attachment_meta_source_url = get_post_meta( $media_id, '_source_url', true ); | ||
|
|
||
| $this->assertInternalType( 'numeric', $media_id ); | ||
| $this->assertEquals( $image_source, $attachment_meta_source_url ); | ||
| } | ||
|
|
||
| } | ||
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 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.Uh oh!
There was an error while loading. Please reload this page.
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.
Check the teardown on the Unit Test
Tests_Post_Attachmentsthey do the same to clear the file system. Theme Dir they remove filter on tearDown.More specific this method
$this->remove_added_uploads();