-
Notifications
You must be signed in to change notification settings - Fork 812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docker: Make WP unit test versions match installed WP version #41934
Docker: Make WP unit test versions match installed WP version #41934
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
@@ -79,14 +79,15 @@ fi | |||
if [ "$COMPOSE_PROJECT_NAME" == "jetpack_dev" ] ; then | |||
# If we don't have the wordpress test helpers, download them | |||
if [ ! -d /tmp/wordpress-develop/tests ]; then | |||
CUR_WP_VERSION=$(wp --allow-root core version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated a few lines in this file, but I'm not sure when this runs or how to test it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file runs as the default command whenever the container is started.
This particular code path will run when /tmp/wordpress-develop/tests
(which is mounted from the host's tools/docker/wordpress-develop
) doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that run.sh
didn't seem to run when the container started; I could only get it to run when the docker image was built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does for me. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what needs to be done to test the initial state/updating the Docker image, etc.
To update the image, you can either jetpack docker build-image
to build it locally or wait for CI to finish and change the docker-compose.yml to use ghcr.io/automattic/jetpack-wordpress-dev:pr-41934
.
jetpack/tools/docker/docker-compose.yml
Line 35 in 877c10e
image: automattic/jetpack-wordpress-dev:latest |
To test the startup, you may have to remove tools/docker/wordpress-develop/tests
on your host, or maybe even start from a fresh clone.
@@ -79,14 +79,15 @@ fi | |||
if [ "$COMPOSE_PROJECT_NAME" == "jetpack_dev" ] ; then | |||
# If we don't have the wordpress test helpers, download them | |||
if [ ! -d /tmp/wordpress-develop/tests ]; then | |||
CUR_WP_VERSION=$(wp --allow-root core version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file runs as the default command whenever the container is started.
This particular code path will run when /tmp/wordpress-develop/tests
(which is mounted from the host's tools/docker/wordpress-develop
) doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One suggestion inline.
* Remove problematic call to `_upgrade_422_remove_genericons()` from `wp-admin/includes/update-core.php`. That call | ||
* recursively searches all directories for genericons files, which results in a circular reference loop for some | ||
* of our projects. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention that there's unfortunately no direct filter for the upgrade stuff, but it happens to call wp_opcache_invalidate()
before requiring the file which gives us a chance to hack it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a fresh
jetpack docker
instance, running PHPUnit tests were failing because the version of core unit tests (trunk latest) was trying to load a file that didn't exist in the version of WordPress (6.7.2).With this PR they should be in sync on init. In particular:
jetpack docker update-core
command was added. This runs a script that updates both WP core and WP unit tests to the same version.jetpack docker update-core-unit-tests
command was removed.See also: p1739803957236829-slack-C05Q5HSS013
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
jetpack docker clean
jetpack docker build-image
jetpack docker up -d
jetpack docker install
jetpack docker phpunit
In
trunk
, the last command will fail. With this branch, the last command will run PHPUnit correctly.As for the other stuff, try some commands to upgrade and downgrade:
jetpack docker update-core
<-- updates to latestjetpack docker update-core 6.6.0
jetpack docker update-core latest
This should successfully update both WordPress and core unit tests to the same version.