Fix skipped tests in Image Placeholders and Modern Image Formats#2523
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| * | ||
| * @return bool True if AVIF encoding is supported, false otherwise. | ||
| */ | ||
| public function check_avif_encoding_support(): bool { |
There was a problem hiding this comment.
This is still required due to following:
The tests are now passing for PHP 8.1+ and PHP 7.2+ versions due to the upstream fix, but PHP 8.1 and PHP 7.2 will not be fixed because support for this Docker image has been dropped (see docker-library/wordpress#992.
We have the option to skip these failing tests for this specific version, or we can remove PHP 8.1 from testing.
For now, I will skip these failing tests on these version.
|
Note sure why Codecov is failing to upload I think we need to update the pined Codecov action version. https://github.com/WordPress/performance/actions/runs/27075344831/job/79911662004?pr=2523 |
|
FWIW, I tried setting all of them to wordpress and it didnt make a difference. So, seems that you caught the two where this currently matters |
| "test-php:performance-lab": "wp-env --config=.wp-env.test.json run cli --env-cwd=/var/www/html/wp-content/plugins/performance composer test:performance-lab", | ||
| "test-php:auto-sizes": "wp-env --config=.wp-env.test.json run cli --env-cwd=/var/www/html/wp-content/plugins/performance composer test:auto-sizes", | ||
| "test-php:dominant-color-images": "wp-env --config=.wp-env.test.json run cli --env-cwd=/var/www/html/wp-content/plugins/performance composer test:dominant-color-images", | ||
| "test-php:dominant-color-images": "wp-env --config=.wp-env.test.json run wordpress --env-cwd=/var/www/html/wp-content/plugins/performance composer test:dominant-color-images", |
There was a problem hiding this comment.
Any reason to not use the wordpress instead of cli for all of the plugins?
There was a problem hiding this comment.
I had this question/thought as well, and even confirmed that the tests all pass after changing them all to wordpress.
I assume the thought was that WordPress is perhaps heavier?
There was a problem hiding this comment.
@westonruter, after some investigation, I found that the tests-cli was introduced in PR #544 before it was phpunit. However, I could not find any discussion explaining the decision to use tests-cli instead of tests-wordpress.
Also, @thelovekesh commented on the issue:
wp-env generates the Dockerfile, builds it, and runs it, so it's wp-env specific. Also, tests should run in the tests WordPress image and not the CLI image.
Reference: #2521 (comment)
I think we should move everything to the wordpress image now, as that would simplify a lot of the setup and maintenance.
updated in b509cb1.
| "test-e2e:auto-sizes": "wp-scripts test-playwright --config tools/e2e/playwright.config.ts --project=auto-sizes", | ||
| "lint-php": "composer lint:all", | ||
| "test-php": "wp-env --config=.wp-env.test.json run cli --env-cwd=/var/www/html/wp-content/plugins/performance composer test:plugins", | ||
| "test-php": "npm-run-all -s --continue-on-error test-php:performance-lab test-php:auto-sizes test-php:dominant-color-images test-php:embed-optimizer test-php:image-prioritizer test-php:optimization-detective test-php:speculation-rules test-php:view-transitions test-php:web-worker-offloading test-php:webp-uploads", |
There was a problem hiding this comment.
Can you elaborate on this change?
There was a problem hiding this comment.
Ah, right, it's because we have the same test command in both package.json and composer.json, but only here do we control where it is run.
There was a problem hiding this comment.
Alternatively, this could be just the following, if we move all execution from cli:
| "test-php": "npm-run-all -s --continue-on-error test-php:performance-lab test-php:auto-sizes test-php:dominant-color-images test-php:embed-optimizer test-php:image-prioritizer test-php:optimization-detective test-php:speculation-rules test-php:view-transitions test-php:web-worker-offloading test-php:webp-uploads", | |
| "test-php": "wp-env --config=.wp-env.test.json run wordpress --env-cwd=/var/www/html/wp-content/plugins/performance composer test:plugins", |
|
As per weston's review, I think using wordpress for all plugins would be cleaner/easier for people to follow than a mix of cli and wordpress. Unless there's a strong reason to minimize use of wordpress? |
Co-authored-by: Weston Ruter <weston@ruter.net>
The v6.0.1 action imported the GPG key from a Keybase account that Codecov deleted during a migration, causing "No public key" signature verification failures. v6.0.2 (a copy of v7.0.0) switches to the codecovsecops account with the original key. See codecov/codecov-action#1955. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ohh missed that yeah we need to update those as well thanks. working on it |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #2523 +/- ##
==========================================
- Coverage 69.29% 69.14% -0.16%
==========================================
Files 90 90
Lines 7723 7723
==========================================
- Hits 5352 5340 -12
- Misses 2371 2383 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: nickchomey <nickchomey@users.noreply.github.com>
There was a problem hiding this comment.
@b1ink0 I was looking into this as well and got Claude's opinion:
.github/workflows/plugin-check.ymlonly runswp plugin check/wp plugin install/wp plugin list— no unit tests, no image processing, so imagick is irrelevant. Thecliservice is actually the conventional wp-env container for running wp-cli commands. The only argument for changing it is the team's broader "move everything to thewordpressimage for consistency/maintenance" goal — and notably,wp-env startprovisions both images regardless, so there's no perf cost either way. It's purely a consistency choice, not a correctness fix.
So seems fine.
There was a problem hiding this comment.
Oh, but Plugin Check is now failing. It seems this needs to be reverted.
There was a problem hiding this comment.
Okay I will remove the change for plugin-check.yml as it actually requires cli for wp cli commands.
Summary
Fixes #2521
Relevant technical choices
This PR updates the test command to use the
wordpressDocker image instead of thecliDocker image.Originally posted by @b1ink0 in #2521
Use of AI Tools
Used AI tools to help identify the root cause of the issue and verify the fix.