#56807 closed defect (bug) (fixed)
Delete existing attachment subsizes after WP_Customize_Manager tests
Reported by: |
|
Owned by: |
|
---|---|---|---|
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 )
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
- Run
phpunit --filter test_import_theme_starter_content
. - 🐞 Observe the newly created
canola-*.jpg
files intests/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)
#3
in reply to:
↑ description
@
2 years 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 invokesWP_Customize_Manager::import_theme_starter_content()
, existing attachments are passed directly towp_generate_attachment_metadata()
, which does not make a copy of the file, andwp_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.
2 years 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
This ticket was mentioned in Slack in #core by chaion07. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
#8
@
2 years 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.
2 years ago
#10
follow-up:
↓ 11
@
2 years 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
@
2 years 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.
@SergeyBiryukov commented on PR #3445:
2 years ago
#13
Thanks for the PR! Merged in r54519.
The "Related" mention in the ticket description should be changeset [54424], not another ticket 😔