Skip to content
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

Merged
merged 9 commits into from
May 15, 2020
Merged

Wp-env: Add phpunit support #20090

merged 9 commits into from
May 15, 2020

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Feb 7, 2020

This PR allows the phpunit script to work with @wordpress/env.

Changes:

  • Add phpunit service to wp-env docker
  • Add wp_phpunit composer dependency so we don't have to use wordpress-develop
  • switch package.json to use wp-env for phpunit tests.

To test, run:

# Get the new composer changes.
./packages/env/bin/wp-env run composer 'composer install'  

# Run phpunit via package.json:
npm run test-unit-php

# With multisite setup:
npm run test-unit-php-multisite

# Alternatively, directly run phpunit command (the package commands are aliases for this)
./packages/env/bin/wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist'   

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:

  1. Currently, you are required to know the absolute path to the phpunit config file inside Docker. That will be different for every source.
  2. You have to install the correct wp-phpunit dependency yourself.
  3. You have to know how to setup the bootstrap and config files correctly.

See #22365 for explorations on how to improve the external API.

To do:

  • Should we export functions from @wordpress/env to handle things like getDockerComposeDir? (or even for reading the config file if needed.)
  • Provide a phpunit service in the default docker config. (Similar to the WordPress one we used before.)
  • verify multisite tests also work
  • verify exit code is 1 if tests fail
  • use different tests port in travis to avoid conflicting with scripts/env
  • verify php version from LOCAL_PHP is used in phpunit environment
  • make sure wordpress is installed before continuing
  • make sure we have permissions to rename files in travis

@noahtallen noahtallen changed the title (WIP): Use wp-env docker config within test-php (WIP): Use wp-env docker config for test-php Feb 7, 2020
@noahtallen noahtallen self-assigned this Feb 7, 2020
@noahtallen noahtallen added [Package] Env /packages/env [Package] Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling labels Feb 7, 2020
Copy link
Contributor

@epiqueras epiqueras left a 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.

@noahtallen
Copy link
Member Author

noahtallen commented Feb 7, 2020

Thanks, @epiqueras! I've modified this to call wpEnv.run instead (which adds a dependency on @wordpress/env.)

Though this raises the question: is test-php/phpcs needed anymore?

In a project, I can add: "test:php": "wp-env run phpunit phpunit -c /var/www/src/wp-content/plugins/full-site-editing-plugin/phpunit.xml.dist", to my package.json file and it should work so long as we have a phpunit service in the default docker config (which I'll look at adding next)

@epiqueras
Copy link
Contributor

epiqueras commented Feb 7, 2020 via email

@noahtallen
Copy link
Member Author

so long as we have a phpunit service in the default docker config

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):

// Give access to tests_add_filter() function.
require_once $_tests_dir . '/includes/functions.php';

It's possible we'll have to add a requirement to use WordPress/wordpress-develop in order to run phpunit. Otherwise, we'll have to make sure those test files are downloaded to the Docker image somehow.

@epiqueras
Copy link
Contributor

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 wordpress-develop is that we force ourselves to build it and lose the excellent speed wp-env start has right now.

@epiqueras
Copy link
Contributor

Maybe we should require it only for that test script.

@gziolo
Copy link
Member

gziolo commented Feb 19, 2020

@noahtallen - nice to see progress on migrating PHPUnit tests to wp-env 💯

Is it necessary to update wp-scripts env to make it work? It feels like we are going to deprecate the whole env subset of scripts and advice to use wp-env instead. Once everything is working with wp-env on Travis, there is no reason to keep two competing implementations.

@epiqueras
Copy link
Contributor

Agreed, I'd rather see this PR deprecating wp-scripts env and replacing its usage with wp-env everywhere instead of doing it in a follow-up. It will help catch any possible bugs.

@noahtallen
Copy link
Member Author

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?)

replacing its usage with wp-env everywhere

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 :)

@epiqueras
Copy link
Contributor

Use the deprecate utility to add messages, add a change-log entry, and write a dev note.

@noisysocks
Copy link
Member

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 wordpress-develop is that we force ourselves to build it and lose the excellent speed wp-env start has right now.

I think it's probably worth biting the bullet and doing this. A WordPress plugin that wants to use WP_UnitTestCase shouldn't have to vendor all of wordpress-develop in order to do so.

@epiqueras
Copy link
Contributor

Agreed, who would be the best person to work on that?

@noisysocks
Copy link
Member

My DMs are open if somebody is interested in doing this and needs help navigating Trac or the codebase 😄

@noahtallen
Copy link
Member Author

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)

@talldan
Copy link
Contributor

talldan commented Mar 6, 2020

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?

@noahtallen
Copy link
Member Author

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

@noisysocks
Copy link
Member

I also can't find any discussion about this, but I spoke with @johnbillion who suggested that Gutenberg could use the third party wp-phpunit/wp-phpunit package, and to look at this PR for inspiration on what needs to happen: johnbillion/user-switching#38

@chrisvanpatten
Copy link
Contributor

I would LOVE this to happen. Setting up the core unit test bootstrap is such a pain — smoothing over that with wp-env would be incredible. Are there any action items that could help move this forward?

@ockham
Copy link
Contributor

ockham commented Apr 30, 2020

Rebased.

@noahtallen noahtallen force-pushed the try/phpunit-tests-with-wp-env branch 2 times, most recently from 069920e to b1d4741 Compare May 14, 2020 18:16
- 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
@noahtallen noahtallen force-pushed the try/phpunit-tests-with-wp-env branch from b1d4741 to 8c8777d Compare May 14, 2020 18:30
@noahtallen
Copy link
Member Author

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'   

@noahtallen noahtallen requested a review from epiqueras May 14, 2020 18:34
@noahtallen
Copy link
Member Author

noahtallen commented May 14, 2020

currently checking two things:

  1. multisite tests (✅)
  2. make sure that exit code is not 0 if a test fails (✅)

@noahtallen
Copy link
Member Author

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.

36.43s$ npm run test-php && npm run test-unit-php-multisite

> gutenberg@8.1.0 test-php /home/travis/build/WordPress/gutenberg

> npm run lint-php && npm run test-unit-php

> gutenberg@8.1.0 prelint-php /home/travis/build/WordPress/gutenberg

> npm run wp-env run composer install -- --no-interaction

> gutenberg@8.1.0 wp-env /home/travis/build/WordPress/gutenberg

> packages/env/bin/wp-env "run" "composer" "install" "--no-interaction"

- 

latest: Pulling from library/composer

Digest: sha256:7d49a2b42bf742f39e0f8e823612ab0def8a479379d9610275c28c39d746fde6

Status: Downloaded newer image for composer:latest

PHP CodeSniffer Config installed_paths set to ../../phpcompatibility/php-compatibility,../../sirbrillig/phpcs-variable-analysis,../../wp-coding-standards/wpcs

✔ Ran `install` in 'composer'. (in 11s 417ms)

> gutenberg@8.1.0 lint-php /home/travis/build/WordPress/gutenberg

> npm run wp-env run composer run-script lint

> gutenberg@8.1.0 wp-env /home/travis/build/WordPress/gutenberg

> packages/env/bin/wp-env "run" "composer" "run-script" "lint"

- 

............................................................  60 / 114 (53%)

......................................................       114 / 114 (100%)

Time: 2.89 secs; Memory: 30MB

✔ Ran `run-script lint` in 'composer'. (in 4s 695ms)

> gutenberg@8.1.0 test-unit-php /home/travis/build/WordPress/gutenberg

> wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist'

- 

✖ Error while running docker-compose command.

latest: Pulling from library/mariadb

Digest: sha256:d96a3364fc783c02d19c54aa81e0dcf81a339340f74a4b3a64db1e72a419f5ab

Status: Downloaded newer image for mariadb:latest

latest: Pulling from library/wordpress

Digest: sha256:19faa564fd56e817cb6057ebacd3fda84fc2b4805a477aaf9eaa495c712f9e29

Status: Downloaded newer image for wordpress:latest

Pulling mysql (mariadb:)...

Pulling tests-wordpress (wordpress:)...

Creating ce047cf4da04af20491927334fb977c6_mysql_1 ... 

ERROR: for ce047cf4da04af20491927334fb977c6_tests-wordpress_1  Cannot start service tests-wordpress: driver failed programming external connectivity on endpoint ce047cf4da04af20491927334fb977c6_tests-wordpress_1 (39fa3b493882f2ce74659df096654f2ac142838d07ba071e17d35008c329b8fa): Bind for 0.0.0.0:8889 failed: port is already allocated

ERROR: for tests-wordpress  Cannot start service tests-wordpress: driver failed programming external connectivity on endpoint ce047cf4da04af20491927334fb977c6_tests-wordpress_1 (39fa3b493882f2ce74659df096654f2ac142838d07ba071e17d35008c329b8fa): Bind for 0.0.0.0:8889 failed: port is already allocated

Encountered errors while bringing up the project.

@noahtallen
Copy link
Member Author

Ahh. That's probably because the port is already allocated to the scripts/env instance 😆

.travis.yml Show resolved Hide resolved
@noahtallen
Copy link
Member Author

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 🤔

> wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist'

- 

latest: Pulling from library/mariadb

Digest: sha256:d96a3364fc783c02d19c54aa81e0dcf81a339340f74a4b3a64db1e72a419f5ab

Status: Downloaded newer image for mariadb:latest

latest: Pulling from library/wordpress

Digest: sha256:19faa564fd56e817cb6057ebacd3fda84fc2b4805a477aaf9eaa495c712f9e29

Status: Downloaded newer image for wordpress:latest

latest: Pulling from wordpressdevelop/phpunit

Digest: sha256:df71f7489bf95c4aa19e6de1ff581929f17d15c8a6d8a23a98310803cced0820

Status: Downloaded newer image for wordpressdevelop/phpunit:latest

<!DOCTYPE html>

<html xmlns="http://www.w3.org/1999/xhtml" dir='ltr'>

<head>

	<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />

	<meta name="viewport" content="width=device-width">

			<title>Database Error</title>

	<style type="text/css">

		html {

			background: #f1f1f1;

		}

		body {

			background: #fff;

			color: #444;

			font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell, "Helvetica Neue", sans-serif;

			margin: 2em auto;

			padding: 1em 2em;

			max-width: 700px;

			-webkit-box-shadow: 0 1px 3px rgba(0, 0, 0, 0.13);

			box-shadow: 0 1px 3px rgba(0, 0, 0, 0.13);

		}

		h1 {

			border-bottom: 1px solid #dadada;

			clear: both;

			color: #666;

			font-size: 24px;

			margin: 30px 0 0 0;

			padding: 0;

			padding-bottom: 7px;

		}

		#error-page {

			margin-top: 50px;

		}

		#error-page p,

		#error-page .wp-die-message {

			font-size: 14px;

			line-height: 1.5;

			margin: 25px 0 20px;

		}

		#error-page code {

			font-family: Consolas, Monaco, monospace;

		}

		ul li {

			margin-bottom: 10px;

			font-size: 14px ;

		}

		a {

			color: #0073aa;

		}

		a:hover,

		a:active {

			color: #00a0d2;

		}

		a:focus {

			color: #124964;

			-webkit-box-shadow:

				0 0 0 1px #5b9dd9,

				0 0 2px 1px rgba(30, 140, 190, 0.8);

			box-shadow:

				0 0 0 1px #5b9dd9,

				0 0 2px 1px rgba(30, 140, 190, 0.8);

			outline: none;

		}

		.button {

			background: #f7f7f7;

			border: 1px solid #ccc;

			color: #555;

			display: inline-block;

			text-decoration: none;

			font-size: 13px;

			line-height: 2;

			height: 28px;

			margin: 0;

			padding: 0 10px 1px;

			cursor: pointer;

			-webkit-border-radius: 3px;

			-webkit-appearance: none;

			border-radius: 3px;

			white-space: nowrap;

			-webkit-box-sizing: border-box;

			-moz-box-sizing:    border-box;

			box-sizing:         border-box;

			-webkit-box-shadow: 0 1px 0 #ccc;

			box-shadow: 0 1px 0 #ccc;

			vertical-align: top;

		}

		.button.button-large {

			height: 30px;

			line-height: 2.15384615;

			padding: 0 12px 2px;

		}

		.button:hover,

		.button:focus {

			background: #fafafa;

			border-color: #999;

			color: #23282d;

		}

		.button:focus {

			border-color: #5b9dd9;

			-webkit-box-shadow: 0 0 3px rgba(0, 115, 170, 0.8);

			box-shadow: 0 0 3px rgba(0, 115, 170, 0.8);

			outline: none;

		}

		.button:active {

			background: #eee;

			border-color: #999;

			-webkit-box-shadow: inset 0 2px 5px -3px rgba(0, 0, 0, 0.5);

			box-shadow: inset 0 2px 5px -3px rgba(0, 0, 0, 0.5);

		}

			</style>

</head>

<body id="error-page">

	<div class="wp-die-message"><h1>Error establishing a database connection</h1></div></body>

</html>

	

✔ Ran `phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist` in 'phpunit'. (in 21s 312ms)

@noahtallen
Copy link
Member Author

Alright, I think we're down to the last error.

1) Extend_Styles_Test::test_without_built_styles

rename(/var/www/html/wp-content/plugins/gutenberg/build/editor/editor-styles.css,/var/www/html/wp-content/plugins/gutenberg/build/editor/editor-styles.css.bak): Permission denied

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

@noahtallen
Copy link
Member Author

phpunit tests look good in Travis now. just some intermittent e2e failures now, so rerunning those.

@noahtallen noahtallen merged commit 8044d4a into master May 15, 2020
@noahtallen noahtallen deleted the try/phpunit-tests-with-wp-env branch May 15, 2020 00:03
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 15, 2020
@TimothyBJacobs TimothyBJacobs mentioned this pull request May 20, 2020
6 tasks
@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
Comment on lines +134 to +135
WP_TESTS_DOMAIN: 'example.org',
WP_PHP_BINARY: 'php',
Copy link
Contributor

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. 🤔

Copy link
Member Author

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:

// 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Env /packages/env [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants