-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Env: Download WordPress PHPUnit Into Container #41780
Env: Download WordPress PHPUnit Into Container #41780
Conversation
One potential optimization for the git checkout could be to add a remote to an empty repository instead of cloning. This would mean nothing is downloaded that we don't want. I think we could fetch the specific tag or branch and then do the sparse checkout. I'm actually not completely sure if the sparse checkout saves us anything as-is, but it felt nice to empty the directory haha. With that said, as far as I can tell, performance isn't really an issue. It's just a thought though. I think the depth of 1 already aggressively optimizes what we're cloning in the first place, but, I can take another pass at the download next week if we want to try and bleed whatever performance out that we can. |
I went ahead with the performance optimization anyway. It should now only be downloading the latest files for the test library. It's a bit faster and the fact that the tests and development files are downloaded in parallel helps too! |
As I was working on this I noticed that the test config doesn't have a different table prefix. I wasn't sure if it mattered at the time, but now, it feels like it should. Is there a good reason we shouldn't change the prefix so that unit tests don't truncate all of the tables? |
This commit adds support for downloading the WordPress PHPUnit source and mounting it at /phpunit in the container. It uses the version of WordPress installed in the container to decide what version to download.
This commit minimizes the amount of data that needs to be downloaded when we pull in the test framework. It does this by not cloning the entire repository and only checking out once we've set up the sparse checkout.
This commit moves the test library from `/phpunit' to `/wordpress-phpunit`. This change is to avoid potential confusion. `/phpunit` sounds like a tool while `/wordpress-phpunit` is not so ambiguous.
I've opened a PR making the appropriate changes for this to be used in Gutenberg. That pull request includes the commits from this one, so, it should also be a demonstration of the functionality here. |
@ObliviousHarmony and @noahtallen the linked issue and this PR came up in today's Editor Bug Scrub. Just checking in to see what the status of this PR is and what needs to be done to get it across the line. |
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.
Nice, overall I think this is good to go! I think we can also remove the line which creates the old wp-phpunit-config.php file, as the new file does the same thing.
On this branch, after removing wp-phpunit from composer, npm run test-unit-php
passes. As a result, it's using these changes successfully. This also works after removing the file I mentioned above.
So this change should be backwards compatible, which is a nice benefit :)
expect( devVolumes ).toEqual( expect.arrayContaining( localSources ) ); | ||
|
||
localSources = [ |
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.
Nit: I'd just define this as a separate variable.
), | ||
readWordPressVersion( config.env.tests.coreSource, spinner, debug ), | ||
] ); | ||
await downloadWPPHPUnit( |
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.
Do we need to run this for every wp-env
install? I wonder if we should instead have a top-level config option for including phpunit. That said, if this isn't causing any meaningful performance impact, we can probably leave it as is.
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 considered this initially but instead decided to heavily optimize the develop
checkout. It only downloads the phpunit
files. I've also taken care to parallelize the download, so as far as performance goes, I've found this to be incredibly fast. It only downloads something like 21MB. For what it's worth, I'm pretty sure this is lighter than the phpunit
container we're currently spinning up.
For me, this was a tradeoff between standardization and performance. Everyone who is using wp-env
should be writing unit tests. All of these tests will likely require the PHPUnit files, so, I didn't see any good reasons not to include them by default.
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.
That's a good reason. The easier we can make unit tests to use, the better.
The entire point of including the test files is that they are better to use than a package like this. We should have an opinionated default and allow them to override it if they really want to.
Since we've made it so that the default is no longer to support `wp-phpunit/wp-phpunit`, we need to explicitly override the default `wp-env` behavior. This will be cleaned up in a future pull request that switches over entirely to the test files in the environment.
@noahtallen, is this good to merge? |
No @Mamaduka, sorry, I need to check on something first. The PHP unit tests in this PR have a different output than |
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.
Left some notes about the changelog and readme. I also want to test the upgrade path for other plugins, so I'll run this branch on a different plugin locally now!
Thanks for the feedback @noahtallen, I've gone ahead and made some changes to the changelog and documentation as requested. |
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.
Alright, I think this is good to go!
What?
This pull request downloads the WordPress PHPUnit test files into the
wordpress
andtests-wordpress
container. This closes #23171.Why?
Currently, the best option we have for installing the WordPress PHPUnit test library is using
wp-phpunit/wp-phpunit
. This is not a bad solution, however, it comes with a few problems:wp-phpunit/wp-phpunit
your tests will fail.Since we know what version of WordPress the environment contains, a better solution would be to just download it and mount it in the container.
How?
This pull request works by scanning
~/.wp-env/{hash}/{coreSource}/wp-includes/version.php
for the current WordPress version. An alternative that was considered was usingwp core version
, but, I wanted to find a solution that could give us the version to include the library in the docker-compose file.We can then use this information to figure out what version of the WordPress PHPUnit library to install. We store it in the
~/.wp-env/{hash}/
directory and mount it at/phpunit
in the container. To get around the need for anything other thangit
, we download from WordPress/wordpress-develop. It mirrors from SVN so we can rely onmaster
being the bleeding edge and the tags containing WordPress versions. We're also using a sparse checkout to prevent having the entireWordPress/wordpress-develop
repository checked out.Lastly, in order to use it in unit tests, we copy the
wp-config.php
file and set the required constants. Once we create the containers with theWP_PHPUNIT__DIR
we can now run unit tests withoutwp-phpunit/wp-phpunit
Note: I looked at removing
wp-phpunit/wp-phpunit
but it felt out of scope for this PR. It requires changes to the test suite to properly useYoast/PHPUnit-Polyfills
.Testing Instructions
This is tricky to test, unfortunately. We're going to have to remove
wp-phpunit/wp-phpunit
. The tests won't pass unless you do the legwork, but, just removing it can let you see that the test suite boots up. That is really all you need to know this in particular works.composer remove wp-phpunit/wp-phpunit --dev
to get rid ofwp-phpunit/wp-phpunit
. In theory, the PHPUnit tests should now not even be able to boot.npm run wp-env run tests-wordpress "/var/www/html/wp-content/plugins/gutenberg/vendor/bin/phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose"
wp-phpunit/wp-phpunit
failures.~/.wp-env/{hash}
and check out theWordPress-PHPUnit
andtests-WordPress-PHPUnit
directories. They should contain all of the test files with a sparse checkout.WP_ENV_CORE
environment variable to try out a few different options for WordPress installs. A local one, a zip, a git repository, etc. Verify that the correct version of the PHPUnit library is installed in each case.I have prepared #41852 with all of the commits from this pull request. It makes the appropriate changes for the Gutenberg PHPUnit tests to be ran in
wp-env
. The PHPUnit test workflow in that pull request uses the provided test files. That pull request passing is a demonstration of this working in CI.