Make WordPress Core

Opened 5 years ago

Last modified 5 years ago

#50209 new enhancement

Remove all uploads between test methods

Reported by: frank-klein's profile Frank Klein Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.0
Component: Build/Test Tools Keywords: needs-patch
Focuses: Cc:

Description

Commit r29120 added a curious feature to the WordPress testing framework: selective uploads deletion between test runs.

What it means is that the first time that WP_UnitTestCase::setUp() is called for a test case, it will create a list of all files contained in the uploads directory. This list, stored in the static $ignore_files property, allows to selectively delete files uploaded during tests.

This code was added so that tests can be run against a development installation of WordPress. In opposition to running tests against a throwaway installation, as should be the case.

There are three issues with this behavior:

  1. Scanning the uploads directory is slow. If you have too many files in that directory, the test runner will "hang" before running the first test. Ultimately it will result in a memory exhaustion error.
  2. Attachments are not properly deleted. _delete_all_data() runs a SQL query that deletes the database entries of attachments, but not the associated files. Since the function is called before wpTearDownAfterClass runs, there's no way to do manual cleanups in tests. The next test case that gets booted up will add the remaining files to the list of files to ignore, making it impossible to do selective uploads deletion. See #41978.
  3. There is no automatic clean up of uploads between test methods or test cases, leaving files lingering around. Even after all tests have been run.

In short this approach is slow, half-baked, and unneeded. We should therefore remove it, and have uploads be cleared the same as any other fixture.

Since this behaviour has been around for years now, we can't unfortunately just remove it, so I propose that we use a feature flag for disabling it.

Change History (4)

#1 follow-up: @johnbillion
5 years ago

Could we fix this by specifying a different uploads directory during test runs that gets nuked upon completion?

#2 in reply to: ↑ 1 @Frank Klein
5 years ago

Using a different uploads directory would solve this problem. We would need to document this change well though. Some developers might rely on the default directory name in their tests.

In terms of cleanup, we need to clean uploads:

  1. After every test: to ensure that all tests start with a clean slate.
  2. Before every test suite run: to ensure that if cleanup didn't happen due to test runner issues, the first test still has a clean slate. Because if a previous test run got stopped due to a PHP error, no cleanup would have happened.

#3 follow-up: @johnbillion
5 years ago

  • Keywords needs-patch added

We could change the directory only when WP_RUN_CORE_TESTS is true, so plugins using the test framework aren't affected.

Agreed on points 1 and 2.

#4 in reply to: ↑ 3 @Frank Klein
5 years ago

The goal is to make this improvement available to all developers that want it. Therefore we should not tie it to the WP_RUN_CORE_TESTS constant, as the behaviours tied to this constant hardly make sense beyond Core.

Note: See TracTickets for help on using tickets.