WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 21 months ago

#38264 closed defect (bug) (fixed)

Tests: Uploads aren't deleted after running individual tests

Reported by: johnbillion Owned by: johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: needs-testing has-patch
Focuses: Cc:

Description

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.

Attachments (2)

38264.patch (502 bytes) - added by atanasangelovdev 22 months ago.
38264.2.patch (1.0 KB) - added by atanasangelovdev 21 months ago.
Refreshed patch with fix for Tests_Post_Thumbnail_Template as well

Download all attachments as: .zip

Change History (13)

#1 @johnbillion
2 years ago

  • Component changed from Build/Test Tools to Media
  • Keywords needs-unit-tests added; needs-testing removed

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


23 months ago

#3 @desrosj
23 months ago

  • Milestone changed from Awaiting Review to 4.9

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

#4 @joemcgill
23 months ago

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.

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


22 months ago

#6 @atanasangelovdev
22 months ago

  • Keywords needs-testing added; needs-patch needs-unit-tests removed

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.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


21 months ago

#8 @desrosj
21 months ago

  • Keywords has-patch added

#9 @desrosj
21 months ago

  • Version set to 4.4

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

#10 @birgire
21 months ago

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.

Last edited 21 months ago by birgire (previous) (diff)

@atanasangelovdev
21 months ago

Refreshed patch with fix for Tests_Post_Thumbnail_Template as well

#11 @johnbillion
21 months ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from new to closed

In 41693:

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

Props atanasangelovdev

Fixes #38264

Note: See TracTickets for help on using tickets.