-
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
Wp-env: Add phpunit support #20090
Wp-env: Add phpunit support #20090
Conversation
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 export functions from @wordpress/env to handle things like getDockerComposeDir? (or even for reading the config file if needed.)
The wp-env run
command removes the need for any of that.
Provide a phpunit service in the default docker config. (Similar to the WordPress one we used before.)
Sure
In order to remove the wp-env package.json field, what if we added a primaryPluginDir field to the .wp-env.json file? If not, we'll need to pass it through args and/or default to cwd.
It should be cwd
like with other scripts.
Thanks, @epiqueras! I've modified this to call Though this raises the question: is test-php/phpcs needed anymore? In a project, I can add: |
I guess we need them for backwards compatibility.
I don’t think importing wp-env is a good idea. Why not make it a peer
dependency and invoke the bin file like the other scripts do.
|
I've added the basic configuration for the phpunit service but ran into an issue. Previously, we always used the wordpress-develop repository, which includes the WordPress test files all the required bootstrapping. Since we are now using the production image, those directories don't exist. This means that WordPress phpunit bootstrap scripts will fail (like this): gutenberg/phpunit/bootstrap.php Lines 28 to 31 in 4030f73
It's possible we'll have to add a requirement to use |
Ideally, the test bootstrap code would be a separate composer package that we load in isolation. I've heard that was planned for Core, but it's not a priority. The issue with adding the requirement to use |
Maybe we should require it only for that test script. |
@noahtallen - nice to see progress on migrating PHPUnit tests to Is it necessary to update |
Agreed, I'd rather see this PR deprecating |
Definitely agree! How would we go about deprecating those scripts? Is there a previous example I could look at for how we typically handle these things? (Or do we just delete those files and the references to them and move on?)
I think we'll only need to update phpcs and phpunit within the scripts package, which is convenient, but as we can see here it might not work out of the box :) |
Use the |
I think it's probably worth biting the bullet and doing this. A WordPress plugin that wants to use |
Agreed, who would be the best person to work on that? |
My DMs are open if somebody is interested in doing this and needs help navigating Trac or the codebase 😄 |
Nice :) I'm afraid my cycles got taken up by a different project at the moment, so I probably won't be able to revisit this PR for a while. (Maybe days, more likely weeks) |
I haven't been able to find any reference in trac to making the unit test bootstrap a separate composer package. Would you be able to share that discussion if you can find it @epiqueras, @noisysocks? |
Also, I'd be interested in poking around at that, but I'm not sure how much work it would take, or even what steps need to be taken to make it happen |
I also can't find any discussion about this, but I spoke with @johnbillion who suggested that Gutenberg could use the third party |
I would LOVE this to happen. Setting up the core unit test bootstrap is such a pain — smoothing over that with |
60ea870
to
6fb0e92
Compare
Rebased. |
069920e
to
b1d4741
Compare
- install wp-phpunit via composer - include composer autoload and config in bootstrap - add missing config options - allow service to be run without starting wp-env first
b1d4741
to
8c8777d
Compare
this PR now only exposes the phpunit and composer changes itself. it doesn't try to expose it in a nice way for other consumers. see #22365 for that. To test, run: # Get the new composer changes.
./packages/env/bin/wp-env run composer 'composer install'
# Run phpunit command pointing at Gutenberg config file.
./packages/env/bin/wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist' |
currently checking two things:
|
currently troubleshooting this error in CI. it looks like running the phpunit command fails because it doesn't like that tests-wordpress is already running, or something like that.
|
Ahh. That's probably because the port is already allocated to the scripts/env instance 😆 |
Hm. Now getting this error. The weird thing is that it does not cause phpunit to fail. I wonder if this could be caused by not having run the wordpress install yet 🤔
|
Alright, I think we're down to the last error.
There's a php test that renames some CSS files and we don't have permission to do that in travis with wp-env apparently |
phpunit tests look good in Travis now. just some intermittent e2e failures now, so rerunning those. |
WP_TESTS_DOMAIN: 'example.org', | ||
WP_PHP_BINARY: 'php', |
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 this is working 🤔 -- at least not for me 🙁
This is the readConfig
function, which is used by init-config.js
to write the docker-compose.yml
file (via build-docker-compose-config.js
). But AFAICS, none of those files evaluate the contents of the config
object. And if I check my ~/wp-env/12345678/docker-compose.yml
, I don't see any of those config constants either. 🤔
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.
Right, the constants are not added to the docker compose config, but set with wp-cli:
gutenberg/packages/env/lib/wordpress.js
Lines 95 to 112 in 066d839
// Set wp-config.php values. | |
for ( let [ key, value ] of Object.entries( config.config ) ) { | |
// Ensure correct port setting from config when configure WP urls. | |
if ( key === 'WP_SITEURL' || key === 'WP_HOME' ) { | |
const url = new URL( value ); | |
url.port = port; | |
value = url.toString(); | |
} | |
const command = [ 'wp', 'config', 'set', key, value ]; | |
if ( typeof value !== 'string' ) { | |
command.push( '--raw' ); | |
} | |
await dockerCompose.run( | |
environment === 'development' ? 'cli' : 'tests-cli', | |
command, | |
options | |
); | |
} |
This function is called during wp-env start
This PR allows the phpunit script to work with @wordpress/env.
Changes:
To test, run:
External API
This works pretty well for us internally, but I think it will be difficult to use from a 3rd party perspective for a few reasons:
See #22365 for explorations on how to improve the external API.
To do:
getDockerComposeDir
? (or even for reading the config file if needed.)LOCAL_PHP
is used in phpunit environment