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

Add WP PHPUnit #366

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,6 @@ web/.htaccess
# Composer
vendor/*
!vendor/.gitkeep

# PHPUnit
phpunit.xml
11 changes: 11 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,23 @@ cache:
- $HOME/.composer/cache
- vendor

env:
global:
- DB_USER=wp
- DB_PASSWORD=password
- DB_NAME=wp_tests

before_install:
- phpenv config-rm xdebug.ini || true
- composer self-update

install:
- composer install -o --prefer-dist --no-interaction

before_script:
- mysql -u root -e "GRANT ALL PRIVILEGES ON ${DB_NAME}.* TO ${DB_USER} IDENTIFIED BY '${DB_PASSWORD}';"
- mysql -u root -e "CREATE DATABASE ${DB_NAME};"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking maybe travis db testing integration should be a blog post or something separate. There are many ways to skin this cat (docker, external dbs, etc) so best to leave up to the user's discretion. We shouldn't even need a db for unit tests, that is for e2e tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The database is required by WordPress' test library. I agree that it isn't setup for pure unit testing, but I also think that this kind of higher level testing is more valuable at the application level than pure isolated unit tests.


script:
- composer lint
- composer test
19 changes: 14 additions & 5 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@
"forum": "https://discourse.roots.io/category/bedrock"
},
"config": {
"preferred-install": "dist"
"preferred-install": "dist",
"platform": {
"php": "5.6.32"
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be php 7 or omitted. Omit this if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. This has to do with PHPUnit more than anything. The reason this is here is so that all dependencies installed will be compatible with PHP 5.6 when it runs on Travis. This is because, even though PHPUnit 5.x is required, because my local version of PHP is 7.2, some of PHPUnit's dependencies will be installed with versions which are not compatible with 5.6.

The alternatives to doing this are:

  • Requiring in the offending dependencies and locking them to a version which is compatible with PHP 5.6. Not a good option as those dependencies could always change and this would be a pain to manage.
  • Running composer update on Travis instead of composer install. I don't like this as much either as its less predictable
  • Always run composer update with PHP 5.6.

This is a very useful configuration would be something that could be added to the docs, as most users probably aren't using Bedrock in a multi-version environment. Users can (and probably should be) set this to their own value based on their target environment.

With PHP 5.6 and 7.0 reaching end of life at the end of the year perhaps this won't be necessary anymore.

}
},
"repositories": [
{
Expand All @@ -41,7 +44,9 @@
"roots/wp-password-bcrypt": "1.0.0"
},
"require-dev": {
"squizlabs/php_codesniffer": "^3.0.2"
"phpunit/phpunit": "^5.0",
"squizlabs/php_codesniffer": "^3.0.2",
"wp-phpunit/wp-phpunit": "4.9.7"
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly don't really want to be married to wp-phpunit. We should leave this open ended and just give people raw phpunit and let people choose whatever approach they want. eg https://github.com/10up/wp_mock type approach or full on unit + e2e tests and such.

@johnpbloch what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair statement, and I'm probably biased as the package creator, but wp-phpunit is more practical for the average user as it will be what most people are familiar with IMO. I don't think it would be a problem for anyone who is familiar with other testing frameworks to replace and use whatever they want.

I think reducing the setup to bare PHPUnit would be less valuable for more people, and wouldn't really make a difference to those who are want to use something else. It's just a boilerplate. No one is married to anything.

Of course bare PHPUnit is better than nothing, I think we agree on that.

},
"extra": {
"installer-paths": {
Expand All @@ -55,8 +60,12 @@
"post-root-package-install": [
"php -r \"copy('.env.example', '.env');\""
],
"test": [
"phpcs"
]
"test": "phpunit",
"lint": "phpcs"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a good change

},
"autoload-dev": {
"psr-4": {
"Tests\\": "tests/src"
}
}
}
Loading