Set up code coverage for unit tests

[swissspidy]

So much for "let me just whip up a quick PR" 😅

Beware, it's quite messy, but it works.

Updating wp-env

This was almost working, but the wp-env version was way too old. There was no xdebug on the phpunit container. PHPUnit would complain with No code coverage driver available. In newer versions that container got removed and merged into tests-cli.

Composer dependencies, platform, autoloader, and more

The original version at 53fc7f1 / 355860b was rather straightforward and worked like this: keep dependencies as-is, but on CI update PHPUnit if needed.
This was done because the platform config in composer.json will cause PHPUnit 6.5x to be installed, but on newer PHP versions we need newer PHPunit

After initial code review this was changed to be a bit more complicated:

• Move PHPunit dependency to /build-cs/composer.json
• Remove /build-cs/composer.lock and the platform config in build-cs/composer.json, rely on composer update for installing the most suitable PHPUnit version
• Automatically run composer update in build-cs when running tests to install PHPUnit

The problem there:

• wp-cli/wp-cli-tests in the main /composer.json also requires PHPUnit, so there will be another version of PHPUnit installed, causing conflicts because of the autoloader
• The autoloader from /composer.json is required for unit tests to work
• PHPStan in build-cs/composer.json requires PHP 7.2, so we would need to ignore platform requirements when installing dependencies...
• However ignoring platform requirements would then install a PHPUnit version that is too new and does not support older PHP version
• --prefer-lowest would also not work because then you get like PHPUnit 4.8

Long story short, the solution for all of that is:

• Move composer.json with just PHPUnit into its own /build-phpunit directory.
• For unit tests, install /composer.json dependencies but add the dev autoloader on top, so there's only one version of PHPUnit being loaded ever
• Install PHPunit with --ignore-platform-reqs
• Do not install any build-cs dependencies so PHPStan won't cause issues.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (db28ffe) 39.49% compared to head (87ee235) 79.43%.

❗ Current head 87ee235 differs from pull request most recent head 61e8b11. Consider uploading reports for the commit 61e8b11 to get more accurate results

Files	Patch %	Lines
.../Checker/Checks/Abstract_PHP_CodeSniffer_Check.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##            trunk     #321       +/-   ##
===========================================
+ Coverage   39.49%   79.43%   +39.94%     
===========================================
  Files          43       43               
  Lines        2028     2028               
===========================================
+ Hits          801     1611      +810     
+ Misses       1227      417      -810     

Flag	Coverage Δ
feature	39.49% <0.00%> (ø)
unit	71.66% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[felixarntz]

@swissspidy Thanks, this approach looks excellent to me!

It's messy but it's also the most elegant solution I can think of. The separate build-phpunit directory with its own composer.json makes total sense to me.

Two minor things below, most importantly shouldn't the yoast/phpunit-polyfills package no longer be installed in the main vendor directory?

Other than that, there's a failure in https://github.com/WordPress/plugin-check/pull/321/checks?check_run_id=18672695525, can we address that? It seems extremely nit-picky, so we should probably put a threshold on when codecov fails (maybe 5% less coverage? or at least a very basic 2%?)

[felixarntz]

Thanks @swissspidy, looks great!

[joemcgill]

Yikes. This is indeed a complicated set of hoops to jump through. Thanks for the writeup, @swissspidy and for coming up with this workaround. This looks like it should work and I've been able to confirm that running tests locally still work as expected after these changes when using the npm script defined in the package.json file.

All of this kind of makes me curious what benefit we're getting out of keeping a platform config setting in our composer.json in the first place? From what I can see, we're not actually installing any dependencies that are bundled into the plugin package. Even the PHPCS rulesets could be moved to dev-dependencies. For tools like code coverage or the WP CLI Tests suite, if they're only meant to work in certain workflows and not in local development environments, then it may be worth removing them also and only installing manually in the workflows where they're being used in order to simplify the setup a bit. I think looking at alternate composer setups can be handled apart from unblocking this effort though—just thinking out loud here.

[joemcgill]

I assume we do not need to autoload the Filesystem classes in this directory since they get loaded through the With_Mock_Filesystem trait, otherwise other classes in that folder would not be automatically loaded. I think that's fine for now 👍🏻

[swissspidy]

All of this kind of makes me curious what benefit we're getting out of keeping a platform config setting in our composer.json in the first place? From what I can see, we're not actually installing any dependencies that are bundled into the plugin package.

Yes we are. Plugin Check requires PHPCS as a prod dependency for many checks.

[swissspidy]

For tools like code coverage or the WP CLI Tests suite, if they're only meant to work in certain workflows and not in local development environments

I do like to run tests locally though :-)

[joemcgill]

Yes we are. Plugin Check requires PHPCS as a prod dependency for many checks.

🤦🏻 of course. Afternoon brain got me again. In that case, I wonder if defining a PHP version via the platform could actually cause issues whenever the plugin is run in a more recent version of PHP? Granted, our test matrix should be able to pick up those issues so it's more of a theoretical problem currently, but something to keep in mind.