Unit Testing: Checking if images already exists before starting tests

[SirLouen]

I've noticed that if for any reason, during testing, a file like test-image-large.jpg is created in the /wp-content/uploads/<current_year>/<current_month>, every additional run of test_wp_get_attachment_image_should_use_wp_get_attachment_metadata will simply fail (and start creating new files like test-image-large-1.jpg, test-image-large-2.jpg, etc., failing permanently)

I was doing some unit testing for another function with dependencies on media, and I started seeing this function failing and I could not see where it was coming until I managed to find the culprit

Reproduction Instructions

1. Create a file called test-image-large.jpg in the directory: /wp-content/uploads/<current_year>/<current_month>/

For example: if you are testing in June 2025, then it will be:
/wp-content/uploads/2025/06/test-image-large.jpg

2. Run the Unit tests for media:

npm run test:php -- --group media

3. Check if it throws any errors.

Testing instructions provided in OP

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

[abcd95]

Test Report

Description

This report validates that the patch works as expected.

Patch tested: ​PR 8899

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.25
• Server: nginx/1.27.2
• Database: mysqli (Server: 8.4.4 / Client: mysqlnd 8.2.25)
• Browser: Chrome 137.0.0.0
• OS: macOS
• Theme: Twenty Seventeen 3.9

Results

✅ No errors occur when running tests with npm run test:php -- --group media

Contrary to when running the same tests without the patch, facing the failure -

There was 1 failure:

1) Tests_Media::test_wp_get_attachment_image_should_use_wp_get_attachment_metadata
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<img width="999" height="999" src="http://example.org/wp-content/uploads/2025/06/test-image-testsize-999x999.jpg" class="attachment-testsize size-testsize" alt="" decoding="async" loading="lazy" srcset="http://example.org/wp-content/uploads/2025/06/test-image-testsize-999x999.jpg 999w, http://example.org/wp-content/uploads/2025/06/test-image-large-150x150.jpg 150w" sizes="auto, (max-width: 999px) 100vw, 999px" />'
+'<img width="999" height="999" src="http://example.org/wp-content/uploads/2025/06/test-image-testsize-999x999.jpg" class="attachment-testsize size-testsize" alt="" decoding="async" loading="lazy" srcset="http://example.org/wp-content/uploads/2025/06/test-image-testsize-999x999.jpg 999w, http://example.org/wp-content/uploads/2025/06/test-image-large-1-150x150.jpg 150w" sizes="auto, (max-width: 999px) 100vw, 999px" />'

[riccardodicurti]

Test Report

Patch tested: ​PR 8899
Ticket: #63532

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.25 64bit
• OS: macOS
• Server: Docker (default wordpress-develop environment)
• Database: MySQL
• Theme: Twenty Twenty-Five 1.2
• Test command: npm run test:php -- --group media

Results

✔️ Without manually creating the file test-image-large.jpg in the uploads directory, all media tests pass.

❌ If the file wp-content/uploads/2025/06/test-image-large.jpg exists prior to running the tests (e.g., created via touch), the test suite reports multiple failures and notices, such as:

• Notice: exif_imagetype(): Error reading from ...
• Undefined array key "sizes"
• Test failures due to unexpected file names like test-image-large-1.jpg

Perhaps it's worth reviewing the patch again to ensure the tests work correctly in all environments, including when conflicting files already exist.

[riccardodicurti]

I made a mistake in closing the ticket

[SergeyBiryukov]

Hi there, thanks for the ticket!

I think a safer approach would perhaps be to delete the pre-existing file, otherwise there might be a collision if a test expects an attachment with the same name but different data.

Ideally, each test class should be responsible for deleting any leftover attachments, see [41693] / #38264.

[SergeyBiryukov]

Related: #50209

[SirLouen]

Replying to SergeyBiryukov:

Hi there, thanks for the ticket!

I think a safer approach would perhaps be to delete the pre-existing file, otherwise there might be a collision if a test expects an attachment with the same name but different data.

Ideally, each test class should be responsible for deleting any leftover attachments, see [41693] / #38264.

Totally agree

I was considering that option last night. My approach is the safest option but I don't love it. The thing is that it could simply serve as a failover in case in this or other tests related to media (not in media.php are using this helper function, but definitely I believe that also cleaning upload data before tests is critical as pointed out in #50209. But this work requires more investigation

For now I think this could be introduced for the Setup function. I might consider this ticket for this afternoon during WCEU (otherwise I will review and add a new commit)

@SergeyBiryukov do you think that the failover I've already implemented is a good idea?

[johnbillion]

What's the cause of a pre-existing image existing in the first place? Does that happen if you cancel the command while the tests are running and before it's had a chance to clean up? I remember that happening in the past, eg. #51735.

[SirLouen]

Replying to johnbillion:

What's the cause of a pre-existing image existing in the first place? Does that happen if you cancel the command while the tests are running and before it's had a chance to clean up? I remember that happening in the past, eg. #51735.

I'm not 100% sure. I was running some unit tests on media that actually used that image. At some point, it might have not wrapped the image and left it as a leftover (maybe because I cancelled the tests during execution or something like that). Then I started to freakout because I was completely unable to make the media tests work again, and I would rather not delete the wordpress-develop directory. So I decided to dig until I got to this issue.

[SergeyBiryukov]

Replying to SirLouen:

@SergeyBiryukov do you think that the failover I've already implemented is a good idea?

To clarify my previous comment (deleting the pre-existing file), I was thinking of something like this:

if ( file_exists( $file_path ) ) {
	unlink( $file_path );
}

$upload = wp_upload_bits( $file_basename, null, $contents );

This is a refresh of ​#8899

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

[sandeepdahiya]

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: ​PR 8927

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.28
• Server: nginx/1.27.5
• Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
• Browser: Firefox 139.0
• OS: Windows 10/11
• Theme: Twenty Twenty-Five 1.2
• MU Plugins: None activated
• Plugins:
  • Test Reports 1.2.0

Actual Results

1. ✅ OK, but incomplete, skipped, or risky tests!

Tests: 710, Assertions: 1702, Skipped: 3.

2. When tried without patch, 1 test failed: -

1) Tests_Media::test_wp_get_attachment_image_should_use_wp_get_attachment_metadata
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<img width="999" height="999" src="http://example.org/wp-content/uploads/2025/06/test-image-testsize-999x999.jpg" class="attachment-testsize size-testsize" alt="" decoding="async" loading="lazy" srcset="http://example.org/wp-content/uploads/2025/06/test-image-testsize-999x999.jpg 999w, http://example.org/wp-content/uploads/2025/06/test-image-large-150x150.jpg 150w" sizes="auto, (max-width: 999px) 100vw, 999px" />'
+'<img width="999" height="999" src="http://example.org/wp-content/uploads/2025/06/test-image-testsize-999x999.jpg" class="attachment-testsize size-testsize" alt="" decoding="async" loading="lazy" srcset="http://example.org/wp-content/uploads/2025/06/test-image-testsize-999x999.jpg 999w, http://example.org/wp-content/uploads/2025/06/test-image-large-1-150x150.jpg 150w" sizes="auto, (max-width: 999px) 100vw, 999px" />'

/var/www/tests/phpunit/tests/media.php:2765

Additional Notes

• npm run test:php -- --group media fails 1 test for file

  test-image-large.jpg
  

  but it doesn't fail for the file (without patch)

  test-image-large.jpeg
  

There was no failure when extension was .jpeg.

Supplemental Artifacts

Test result without patch:

[peterwilsoncc]

Replying to johnbillion:

What's the cause of a pre-existing image existing in the first place? Does that happen if you cancel the command while the tests are running and before it's had a chance to clean up? I remember that happening in the past, eg. #51735.

One cause can be that the Docker environment included in WordPress-Develop shares the uploads directory for both the installation and the test suite. If a developer uploads tests/phpunit/data/images/canola.jpg for some manual testing and then runs the test suite, the image will exist.

Replying to SergeyBiryukov:

I think a safer approach would perhaps be to delete the pre-existing file, otherwise there might be a collision if a test expects an attachment with the same name but different data.

In the case of a collision due to manual testing, this may cause problems for the developer using the environment as their test images will be deleted from the development environment.

---

For the test suite, could we filter the uploads path and URL to use wp-content/test-suite-uploads/ as a disposable location for attachment tests? That would allow the test suite to delete the directory each time it's started to avoid the issue?

[mindctrl]

the Docker environment included in WordPress-Develop shares the uploads directory for both the installation and the test suite.

...

For the test suite, could we filter the uploads path and URL to use wp-content/test-suite-uploads/ as a disposable location for attachment tests?

I like this idea. Thoughts @SirLouen?

[SirLouen]

Replying to mindctrl:

the Docker environment included in WordPress-Develop shares the uploads directory for both the installation and the test suite.

...

For the test suite, could we filter the uploads path and URL to use wp-content/test-suite-uploads/ as a disposable location for attachment tests?

I like this idea. Thoughts @SirLouen?

It makes sense to me to avoid unnecessary collisions.

[jorbin]

I agree with the idea to have the test suite use its own uploads directory. I think we can use the UPLOADS constant in tests/phpunit/includes/bootstrap.php (with a check to make sure it's not already defined) for this.

[wildworks]

The RC1 release is approaching, and a new patch is needed, so I'd like to punt this ticket to 7.0.