Tests: Uploads aren't deleted after running individual tests

[johnbillion]

Steps to reproduce:

1. Run phpunit and verify the tests pass.
2. Run phpunit --filter test_wp_get_attachment_image_should_use_wp_get_attachment_metadata and verify the tests pass.
3. Run phpunit --filter test_wp_get_attachment_image_should_use_wp_get_attachment_metadata a second time and observe that the tests fail.

View the contents of your site's uploads directory and you'll observe that the 2016/10/test-image-large.png file and its resized versions haven't been deleted, and get created anew each time the test runs.

The same thing happens when running any test on its own that adds an attachment. The reason the test_wp_get_attachment_image_should_use_wp_get_attachment_metadata test fails the second time is because of its overly strict assumption about the contents of the <img> tag. This failure caused me to notice the fact that the upload file wasn't being deleted.

[desrosj]

Was able to reproduce this. Would be good to fix this for 4.9.

[joemcgill]

I've noticed this too and would love to see us address it. In some of these cases, I even wonder if uploading files is necessary or if we could simply populate the DB with the data we need, which would also decrease test time.

[atanasangelovdev]

I spent some time investigating this and what I found was that the test in question does not clean up it's uploaded file but since it is ran alongside other tests that remove all uploads it happens to work.

My patch resolves the issue but I don't really like that it deletes a post that will end up deleted due to database transactions/wiping anyway (this may not really be something worth thinking about - let me know).

On a related note, Tests_Post_Thumbnail_Template suffers from the same issue - should I refresh the patch including a fix for it as well or should that be posted in a separate ticket with it's own patch?

---

Other (failed) attemps:

• Since the file was uploaded in wpSetUpBeforeClass, my first attempt was to use remove_added_uploads() in wpTearDownAfterClass which was unsuccessful since remove_added_uploads is an instance method while wpTearDownAfterClass is static (even if I add a static equivalent it calls $this->rmdir() which calls $this->files_in_dir() and down the rabbit hole we go ...)
• My second attempt included moving the attachment creation to setUp and it's deletion on tearDown which as you would probably guess led to a slow test.

[desrosj]

@atanasangelovdev Thanks for the patch! It works as advertised.

I think deleting the post is fine. I looked around at tests already committed. The REST API tests do a really good job of deleting posts after each test or test class, and Tests_Comment_WpAllowComment also deletes posts after.

I also think fixing Tests_Post_Thumbnail_Template in this patch is fine. Feel free to update your patch.

[birgire]

Just stumbled on this issue when writing a test yesterday, for a patch in #22101, for gallery shortcode output in feeds.

https://core.trac.wordpress.org/ticket/22101#comment:11

... I noticed that the test in test_wp_get_attachment_image_should_use_wp_get_attachment_metadata() 
can only run once, because the filename gets an increasing index, like test-image-large-{$i}-150x150.png 
in the srcset, where $i > 2 is the number of times we run the /tests/phpunit/tests/media.php tests. Looks 
like we might need a better cleanup there?
I tried to avoid this in the above test by not referring to the exact filename for self::$large_id, as 
it would also get the same increasing index for repeated test runs.

Was just about to create a ticket for this, when I found this one ;-)

I just tested the patch in 38264.patch and it seems to solve the problem. Thanks @atanasangelovdev.

[atanasangelovdev]

Refreshed patch with fix for Tests_Post_Thumbnail_Template as well

[johnbillion]

In 41693:

Build/Test tools: Correctly delete attachment files used in the media and post thumbnail tests.

Props atanasangelovdev

Fixes #38264