xwp / wp-dev-lib

DEPRECATED. Common code used during development of WordPress plugins and themes
MIT License
279 stars 65 forks source link

Always install a valid version of phpunit #306

Closed kasparsd closed 5 years ago

kasparsd commented 5 years ago

Fixes #305.

westonruter commented 5 years ago

Doesn't this mean that phpunit will always be downloaded even if it isn't needed? Should this check if phpunit is already a dependency in Composer before doing this?

kasparsd commented 5 years ago

@westonruter See the description of #305.

Doesn't this mean that phpunit will always be downloaded even if it isn't needed?

We're still leaving in the check_should_execute 'phpunit' check. I don't think we've ever had any other checks for this. There is always an option to disable the phpunit checks with DEV_LIB_SKIP.

Should this check if phpunit is already a dependency in Composer before doing this?

Hard to do that reliably, I think. The order of binary directories in $PATH ensures that we use the Composer version if it exists.

westonruter commented 5 years ago

The can_generate_coverage_clover function does have some logic to sniff the composer.json for something being installed:

https://github.com/xwp/wp-dev-lib/blob/4b3a130ebcc0d8b7af6bad10b68c6215a2d944a7/scripts/check-diff.sh#L395-L404

kasparsd commented 5 years ago

@westonruter Yes. And we've had issues related to that before when php-coveralls renamed their packages.

We currently install the following tools by default -- phpcs, grunt, jshint, jscs, eslint and phpunit. Some of them are only installed when a relevant config file is found.

We could check if $PHPUNIT_CONFIG is not empty but that would be a breaking change. I suggest we keep installing it for now.

westonruter commented 5 years ago

This fixed the AMP plugin build: https://github.com/ampproject/amp-wp/pull/2750 👍