[wp-trac] [WordPress Trac] #41978: `_delete_all_data()` does not delete attachment files

[WordPress Trac]

#41978: `_delete_all_data()` does not delete attachment files
-------------------------------------------------+-------------------------
 Reporter:  Frank Klein                          |       Owner:
     Type:  defect (bug)                         |  johnbillion
 Priority:  normal                               |      Status:  reviewing
Component:  Build/Test Tools                     |   Milestone:  5.0
 Severity:  normal                               |     Version:  4.7
 Keywords:  has-patch reporter-feedback needs-   |  Resolution:
  testing                                        |     Focuses:
-------------------------------------------------+-------------------------

Comment (by Frank Klein):

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

--
Ticket URL: <https://core.trac.wordpress.org/ticket/41978#comment:3>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform