Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#56807 closed defect (bug) (fixed)

Delete existing attachment subsizes after WP_Customize_Manager tests

Reported by: ironprogrammer's profile ironprogrammer Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version: 6.1
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)

After running the PHPUnit test for test_import_theme_starter_content() (in tests/customize/manager.php), the following files are generated and left in place, resulting in a dirty working copy of the project:

tests/phpunit/data/images/canola-150x150.jpg
tests/phpunit/data/images/canola-300x225.jpg

Background

In the test, canola.jpg is mocked as an existing attachment. When the test invokes WP_Customize_Manager::import_theme_starter_content(), existing attachments are passed directly to wp_generate_attachment_metadata(), which does not make a copy of the file, and wp_create_image_subsizes() creates the above referenced files in the original image's location.

By contrast, waffles.jpg, also used in the test, is not set as an existing attachment, and in import_theme_starter_content() is passed to media_handle_sideload(), which makes a copy of the file in wp-content/uploads/, and then calls wp_create_image_subsizes() against the new file. The resulting subsizes are generated in that location, and are cleaned up during tear_down().

The test's tear_down() uses remove_added_uploads(), but the function only clears items created in wp-content/uploads/ (i.e. waffles*.jpg), and not files in the DIR_TESTDATA directory (canola-*.jpg).

Testing Instructions

Steps to Reproduce

  1. Run phpunit --filter test_import_theme_starter_content.
  2. 🐞 Observe the newly created canola-*.jpg files in tests/phpunit/data/images/.

Expected Results

  • ❌ There should not be any added files to the DIR_TESTDATA directory after running the test.

Related: [54424].

Test Report Icons:
🐞 <= Indicates where issue ("bug") occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.

Change History (13)

#1 @ironprogrammer
8 months ago

The "Related" mention in the ticket description should be changeset [54424], not another ticket 😔

Last edited 8 months ago by SergeyBiryukov (previous) (diff)

#2 @SergeyBiryukov
8 months ago

  • Description modified (diff)

#3 in reply to: ↑ description @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 6.1
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

Thanks for the ticket!

Replying to ironprogrammer:

In the test, canola.jpg is mocked as an existing attachment. When the test invokes WP_Customize_Manager::import_theme_starter_content(), existing attachments are passed directly to wp_generate_attachment_metadata(), which does not make a copy of the file, and wp_create_image_subsizes() creates the above referenced files in the original image's location.

It looks like my assessment in [54424] was incorrect, and the file does in fact need to be copied. I could not reproduce the leftover files when running the tests on Windows, but can under WSL.

This ticket was mentioned in PR #3445 on WordPress/wordpress-develop by ironprogrammer.


8 months ago
#4

  • Keywords has-patch has-unit-tests added

Addresses files left over from import_theme_starter_content() PHPUnit test.

  • Existing attachments will most likely be located in uploads (not arbitrary path like unit test folder).
  • Uploads folder is cleared during tear down, which addresses the dirty copy issue.

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

#5 @ironprogrammer
8 months ago

PR 3445 addresses the leftover file by making a copy into uploads, but only within the test in question.

This should play well with the other optimization in [54424] 👍🏻

@SergeyBiryukov, thanks for cleaning up the "Related" flub 😂

This ticket was mentioned in Slack in #core by chaion07. View the logs.


8 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


8 months ago

#8 @audrasjb
8 months ago

As per today's bug scrub: the logic of the unit test changes provided in PR3445 looks good.

Let's keep it in the milestone as it has a patch/unit test and an owner :)

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


8 months ago

#10 follow-up: @SergeyBiryukov
8 months ago

I was curious why I didn't see the leftover files when running the tests on Windows.

It looks like this has to do with one of the test failures mentioned in #50664:

1) Tests_WP_Customize_Manager::test_import_theme_starter_content
Expected reuse of non-auto-draft attachment.
Failed asserting that an array contains 949.

S:\home\wordpress.test\develop\tests\phpunit\tests\customize\manager.php:691

Upon some investigation, the file is saved with an incorrect path on Windows:

S:\home\wordpress.test\develop-dynamic-properties/src/wp-content/uploads/S:homewordpress.testdevelop-dynamic-propertiestestsphpunitincludes/../data/images/canola.jpg

so it's not detected as an existing attachment. The path probably needs to be normalized at some point. This should be further explored in #50664.

I was able to reproduce the leftover files when running the tests under WSL. The PR works as expected.

#11 in reply to: ↑ 10 @SergeyBiryukov
8 months ago

Replying to SergeyBiryukov:

I was curious why I didn't see the leftover files when running the tests on Windows.

It looks like this has to do with one of the test failures mentioned in #50664:

1) Tests_WP_Customize_Manager::test_import_theme_starter_content
Expected reuse of non-auto-draft attachment.
Failed asserting that an array contains 949.

S:\home\wordpress.test\develop\tests\phpunit\tests\customize\manager.php:691

It appears that the PR actually resolves this test failure too. If the existing attachment is under wp-content/uploads/ (as it should be), the file path is correct, and the attachment is detected and reused as expected.

#12 @SergeyBiryukov
8 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 54519:

Tests: Delete leftover image sub-sizes after WP_Customize_Manager tests.

After running phpunit --filter test_import_theme_starter_content, the following files were generated and left in place in the version-controlled DIR_TESTDATA directory, resulting in a dirty working copy of the project:

tests/phpunit/data/images/canola-150x150.jpg
tests/phpunit/data/images/canola-300x225.jpg

In the test, canola.jpg is mocked as an existing attachment. When the test invokes WP_Customize_Manager::import_theme_starter_content(), existing attachments are passed directly to wp_generate_attachment_metadata(), which does not make a copy of the file, and wp_create_image_subsizes() creates the above referenced files in the original image's location.

By contrast, waffles.jpg, also used in the test, is not set as an existing attachment, and in import_theme_starter_content() is passed to media_handle_sideload(), which makes a copy of the file in the wp-content/uploads/ directory, and then calls wp_create_image_subsizes() against the new file. The resulting sub-sizes are generated in that location, and are cleaned up during tear_down().

The test's tear_down() uses remove_added_uploads(), which only clears items created in wp-content/uploads/ (i.e. waffles*.jpg), but not the files in the DIR_TESTDATA directory (canola-*.jpg).

This commit addresses the issue by creating a copy of the file in uploads. This better matches the intention of the test, as existing attachments will most likely be located in uploads and not in an arbitrary path like the test data directory.

Follow-up to [39276], [39346], [39411], [40142], [54424], [54425].

Props ironprogrammer, audrasjb, boniu91, dariak, SergeyBiryukov.
Fixes #56807.

@SergeyBiryukov commented on PR #3445:


8 months ago
#13

Thanks for the PR! Merged in r54519.

Note: See TracTickets for help on using tickets.