WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 6 weeks ago

#41978 reviewing defect (bug)

`_delete_all_data()` does not delete attachment files

Reported by: Frank Klein Owned by: johnbillion
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.7
Component: Build/Test Tools Keywords: has-patch reporter-feedback needs-testing
Focuses: Cc:

Description

The _delete_all_data() function is called by WP_UnitTestCase::wpTearDownAfterClass(). The function deletes all the contents of the posts table in the tests database. However it does not delete any attachment files.

So when you create a shared attachment fixture in WP_UnitTestCase::wpSetUpBeforeClass(), _delete_all_data() will delete the attachment post in WP_UnitTestCase::wpTearDownAfterClass(), but leave the file.

There is no way to manually delete the attachment file in wpTearDownAfterClass(), because the attachment id no longer exists, and wp_delete_attachment() will therefore fail.

The solution: Instead of a direct SQL query, _delete_all_data() should use _delete_all_posts(), which accounts for attachments.

Attachments (1)

41978.patch (2.2 KB) - added by Frank Klein 2 months ago.

Download all attachments as: .zip

Change History (4)

@Frank Klein
2 months ago

#1 @Frank Klein
2 months ago

  • Keywords has-patch added

#2 @johnbillion
6 weeks ago

  • Keywords reporter-feedback needs-testing added
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to johnbillion
  • Status changed from new to reviewing
  • Version changed from 4.9 to 4.7

I'm concerned about the performance impact of this on the tests. I'm going to run some numbers.

Is this strictly necessary if all tests that deal with attachments delete their own attachment files? Ref #38264

#3 @Frank Klein
6 weeks ago

I'm concerned about the performance impact of this on the tests.

This change does add an extra SQL query for every test class, which means more database load, and therefore worse performance.

However on every test method run, WP_UnitTestCase::scan_user_uploads() is called. So as the uploads directory fills with files, this would then be a slower operation. So this should improve performance through that, especially when running the entire test suite.

So not sure which one has more impact here.

Is this strictly necessary if all tests that deal with attachments delete their own attachment files?

No, it is not.

To me this issue boils down to how we want the test framework to behave. Right now, it is inconsistent in how it deals with fixtures: It deletes database fixtures, but not file fixtures.

This change would make the fixture deletion more consistent in that it deletes all fixtures.

However one might approach this differently, and say that tests need to delete their attachment fixtures themselves. One must be careful though, as #38264 shows, to delete the attachments before calling parent::tearDownAfterClass(). This is not obvious for developers that aren't very familiar with the behaviour of the testing framework.

Both approaches do not solve the consistency issue. So either we automatically delete all fixtures, or no fixtures.

Personally I'd prefer that tests do delete all of their fixtures manually. I acknowledge that this would be a lot of code churn, but it would make the test suite more transparent. Because the current behaviour is weird.

So we'd need to find a consensus, implement it, and document it, so that the behaviour is well understood by contributing developers, and developers using the testing framework for their own code.

Note: See TracTickets for help on using tickets.