Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions tests/phpunit/tests/admin/includesMedia.php
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() {

Copy link
Copy Markdown
Member

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.

@killua99 killua99 Mar 23, 2020

Copy link
Copy Markdown
Author

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_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();

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 [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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' elements

Then 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.

'response' => [
'code' => 200,
],
];
}

/**
* @ticket 49631
*/
public function test_media_sideload_image() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 );
}

}