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

Use local tooling to run PHPUnit tests instead of wp-dev-lib #1124

Merged
merged 28 commits into from
Jul 6, 2020

Conversation

kasparsd
Copy link
Contributor

@kasparsd kasparsd commented Jul 3, 2020

Follow-up to #1123.

  • Use johnpbloch/wordpress and wp-phpunit/wp-phpunit to run PHPunit tests instead of wp-dev-lib.

  • Introduce the wait-for-it.sh helper to wait for the MySQL server to come alive in the db_phpunit container.

  • Simplify helpers and scripts for running tasks in Docker containers and inside the Vagrant wrapper for the Docker.

  • Add support for running PHPunit tests with different versions of PHP in the wordpress container. Switch Travis to build those containers instead of using different versions of PHP on the host. Sample run https://travis-ci.com/github/xwp/stream/jobs/357370647

  • Fix coverage reporting on Coveralls https://coveralls.io/github/xwp/stream -- introduce a new helper PHP script to make all file paths in coverage report relative to the project directory instead of absolute since the coverage report is generated inside Docker container and those paths don't exist outside of Docker.

"repositories": [
{
"type": "git",
"url": "https://github.com/WordPress/wordpress-develop.git"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use https://github.com/johnpbloch/wordpress now which doesn't require any build steps and is much faster to install, and can be used in combination of wp-phpunit/wp-phpunit to run the unit tests with any version of WP core.

}
},
"extra": {
"wordpress-install-dir": "local/public"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we were using the WP core files provisioned by the wordpress docker container during the entrypoint setup. We now have full control of the source files used for development which is much more reliable and provisions the container much faster, too.

"scripts": {
"pre-install-cmd": [
"Composer\\Config::disableProcessTimeout"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with a 600 second timeout for all commands as a sane default.

Copy link
Contributor

@kidunot89 kidunot89 Jul 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kasparsd I don't know the original purpose of this timeout.
@kopepasah You implemented this originally. Are you okay with this change?

WORDPRESS_DB_USER: wordpress
WORDPRESS_DB_PASSWORD: password
WP_TESTS_DIR: /var/www/html/wp-content/plugins/stream-src/vendor/wp-phpunit/wp-phpunit
WP_PHPUNIT__TESTS_CONFIG: /var/www/html/wp-tests-config.php
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required for wp-phpunit/wp-phpunit to find the WP install.

@@ -5,9 +5,6 @@
* phpcs:disable WordPress.WP.GlobalVariablesOverride.Prohibited
*/

define( 'ABSPATH', dirname( dirname( __DIR__ ) ) . '/' );

define( 'WP_TESTS_MULTISITE', true );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the WP_MULTISITE environment variable to control this behaviour now.

- WP_VERSION=latest WP_MULTISITE=0
- WP_VERSION=latest WP_MULTISITE=1
- WP_VERSION=trunk WP_MULTISITE=0
- WP_VERSION=trunk WP_MULTISITE=1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have npm run phpunit-multisite for this.

@kasparsd kasparsd requested a review from kidunot89 July 3, 2020 14:44
@kasparsd kasparsd self-assigned this Jul 3, 2020
@kasparsd kasparsd added this to the Version 3.5.0 milestone Jul 3, 2020
@kasparsd
Copy link
Contributor Author

kasparsd commented Jul 3, 2020

@kidunot89 Could you please code review this? I've left inline comments to explain the changes. This simplifies and unifies the testing approach across environments and we should have much more reliable developer experience going forward.

Here are sample Travis runs https://travis-ci.com/github/xwp/stream/builds/174220409

@kasparsd kasparsd added the tooling Development and deployment tooling improvements label Jul 3, 2020
- WP_VERSION=latest WP_MULTISITE=1
- WP_VERSION=trunk WP_MULTISITE=0
- WP_VERSION=trunk WP_MULTISITE=1
php: "7.4"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis jobs now handle the different types of runs we need.

env: {
"DOCKER_COMPOSE_FILE" => "/vagrant/docker-compose.yml"
}
inline: "docker-compose -f /vagrant/docker-compose.yml exec -T --user 1000 wordpress xwp_wait mysql:3306 -t 60 -- wp core multisite-install --url=stream.local",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the bash script with an inline command that does the same thing.

@@ -43,7 +38,12 @@
"@lint-php"
],
"test": [
"phpunit --coverage-text"
"phpunit --coverage-text",
"php local/scripts/make-clover-relative.php ./tests/reports/clover.xml"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This converts all file paths inside the coverage reports to be relative to the project root instead of absolute paths inside the Docker container which was causing issues to Coveralls reporting.


FROM wordpress:php${PHP_VERSION}-apache

ARG XDEBUG_VERSION=2.9.6
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP 5.6 needs an earlier version so we provide an ARG to override it as needed on Travis.

@@ -43,14 +43,16 @@
"lint-js": "eslint .",
"lint-php": "composer lint",
"lint": "npm run lint-js && npm run lint-php",
"cli": "./local/vagrant/cli.sh",
"compose": "./local/vagrant/docker-compose.sh",
"stop-all": "docker stop $(docker ps -a -q)",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new helper to stop all running containers on a Docker host in case there are port conflicts.

"phpunit-report": "npm run cli -- composer test-report --working-dir=wp-content/plugins/stream-src",
"vcli": "vagrant ssh -- DOCKER_COMPOSE_FILE=/vagrant/docker-compose.yml /vagrant/local/vagrant/cli.sh",
"vcompose": "vagrant ssh -- DOCKER_COMPOSE_FILE=/vagrant/docker-compose.yml /vagrant/local/vagrant/docker-compose.sh",
"phpunit-multisite": "npm run cli -- composer test-multisite --working-dir=wp-content/plugins/stream-src",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new helper for running the same unit tests with a WP multisite.

Copy link
Contributor

@kidunot89 kidunot89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kasparsd I having issue running npm run vphpunit and npm run phpunit locally. I've destroyed the vagrant box using vagrant destroy and deleted the docker images, before run vagrant up then npm run vphpunit.

Here's a snap of the terminal output
image

"scripts": {
"pre-install-cmd": [
"Composer\\Config::disableProcessTimeout"
Copy link
Contributor

@kidunot89 kidunot89 Jul 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kasparsd I don't know the original purpose of this timeout.
@kopepasah You implemented this originally. Are you okay with this change?

@kasparsd
Copy link
Contributor Author

kasparsd commented Jul 3, 2020

Thanks for reviewing the code and testing the changes @kidunot89!

Could you please try running composer install after deleting the vendor directory and the local/public directory? I tested both Docker and Vagrant runs locally and they seem to work fine.

@kidunot89 kidunot89 self-requested a review July 4, 2020 00:06
@kidunot89
Copy link
Contributor

kidunot89 commented Jul 4, 2020

@kasparsd Sorry about the delayed response, deleting the vendor and local/public did it. 👍

@kasparsd kasparsd merged commit cb8e5c4 into develop Jul 6, 2020
@kasparsd kasparsd deleted the devops/ci-update branch July 6, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Development and deployment tooling improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants