-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add WP PHPUnit #366
Conversation
phpunit 5 for compatibility with php 5.6
this file is for local customizations, phpunit.xml.dist is shipped
this constant cannot be defined in the testing environment as a matter of timing.
using php 5.6 for compatibility with all versions tested on travis CI
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.
TLDR bedrock does an excellent job of being a neutral party in that pretty much everything that it does is 100% the uncontroversial/standard approach. This PR as-is makes some statements bedrock should not be making.
These are two statements that I am okay with bedrock supporting and that most people will stand by:
- PHPUnit is widely accepted as the defacto PHP unit test framework
- Composer is generally how people manage their PHP dependencies
The following is something bedrock should not suggest:
PHPUnit + wp-phpunit is the best way to test your wordpress installation.
Bedrock should not endorse any particular testing paradigm whether it be the full wordpress.org testing suite, online db unit/e2e tests with https://github.com/wp-phpunit/wp-phpunit, offline unit tests with https://github.com/10up/wp_mock or any other solution out there.
CC @johnpbloch any thoughts here?
"preferred-install": "dist" | ||
"preferred-install": "dist", | ||
"platform": { | ||
"php": "5.6.32" |
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.
this should be php 7 or omitted. Omit this if possible.
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.
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 ofcomposer 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.
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};" |
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.
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.
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.
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.
"phpcs" | ||
] | ||
"test": "phpunit", | ||
"lint": "phpcs" |
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.
this is a good change
@@ -72,7 +72,6 @@ | |||
* Custom Settings | |||
*/ | |||
define('AUTOMATIC_UPDATER_DISABLED', true); | |||
define('DISABLE_WP_CRON', env('DISABLE_WP_CRON') ?: false); |
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.
what is going on here?
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.
This is not completely removed, but the define is moved into the environment configuration files. This is because this constant cannot be defined in the testing environment, as it will define it itself without checking if it has already been defined or not.
The alternative to moving it to the configuration files (and not to testing.php
) is to wrap this with a conditional to only define it if WP_ENV !== 'testing'
which is a little less clean IMO.
"squizlabs/php_codesniffer": "^3.0.2" | ||
"phpunit/phpunit": "^5.0", | ||
"squizlabs/php_codesniffer": "^3.0.2", | ||
"wp-phpunit/wp-phpunit": "4.9.7" |
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.
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?
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 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.
Also wow sorry meant to include this in initial review but: thank you so much for putting this together! This is a great start 👍. |
I haven't taken a look at the code, just jumping in to follow up to @austinpray's comment. I think insofar as the PR improves the compatibility between WP PHPUnit and Bedrock, this is a great initiative. I do tend to agree with the idea of keeping Bedrock testing framework-agnostic. Maybe it would be worth putting some of the more popular testing frameworks (WP Mock, WP PHPUnit, BrainMonkey, etc.) in the suggest block of |
I mostly agree with @austinpray. I kind of forgot that the WP test unit library is mostly designed for testing WP core itself (for core developers). It's pretty heavy-handed for normal application/project testing. So having the boilerplate required to make that easier with Bedrock is still useful 👍 |
Thank you everyone for your feedback! It seems like everyone is on board with the idea that adding some boilerplate for testing is a step in the right direction but maybe it shouldn't be opinionated in choosing a framework to include boilerplate for.
I don't particularly agree with the idea that including a library is making such a statement, nor would I make that claim myself. The library is really just the WordPress core library that supports running PHPUnit in a WordPress environment. It's just been made easily installable via Composer. I would say that this library is the one that most people are going to be familiar with in a WordPress context (WP CLI scaffolds it, and most articles/resources on testing in WordPress are based on it). Of course others are available, but I think if Bedrock were to include a more complete testing boilerplate (as proposed) that it offers more value than if it was more barebones. By no means does it lock anyone in to using it or impose anything that isn't easily changeable which I think is the important aspect of the principal of remaining framework agnostic. With that said, I fully respect the teams decision to keep Bedrock framework agnostic if that's the consensus. In my opinion, including the "official" core library for testing is still relatively framework agnostic because it's part of WordPress core. Either way, I'm still interested in helping with this initiative in whatever way I can. |
|
@aaemnnosttv maybe you could make an example project of this instead ? |
Thanks to the work done in roots/bedrock#366 .
This is a first draft of a working implementation of what it would look like to integrate WP PHPUnit into Bedrock.
Documentation is not included yet and still needs to be added, but I wanted to get a first round of feedback before writing it.
There are a few things that are somewhat of stylistic preferences, like the exceptions made to the PHPCS rules for tests and the formatting of the example test. This is somewhat bending the rules of the existing ruleset, but I believe it's a common test-specific style.
Apart from that, very little has really changed, as almost everything here are additions.
I look forward to hearing anyone's feedback on the proposed changes.
Resolves #365