`_delete_all_data()` does not delete attachment files

[Frank Klein]

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.

[johnbillion]

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

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

[pento]

This ticket needs further discussion and a decision.

I'm adding this to see if 41978.patch will pass the current testing set 8 years later. From there I will be trying to test this.

Trac ticket: https://core.trac.wordpress.org/ticket/41978

[SirLouen]

Maybe I'm wrong, but shouldn't attachments be handled on setup instead of on wrap up?

The problem with handling this on wrap up is that if the test fails for any reason, it could leave leftovers that might hang future runs (it happened to me twice in the past 2 months)

For the DB it's not a big deal, just leftovers that rarely conflict with future runs (we never check for the exact record I of the DB entry, it doesn't make any sense). But file leftovers can interfere, and it's too sensitive to be left for a wrap up function that might never run.

cc @johnbillion @frank-klein