Allow for checking test code coverage #560

jrfnl · 2020-07-27T21:24:53Z

As part of the sniff reviews, I'll be checking that the sniffs are properly covered by unit tests.

Adding a code coverage configuration and @covers tags makes that simpler.

Note: this doesn't yet add code coverage checking to the Travis build/PR build checks. If so desired, that can be added separately.

The current code coverage results as is:

Commit details

Tests: add code coverage configuration

PHPUnit config: add code coverage configuration.
By default, when there is a code coverage configuration and Xdebug is enabled, code coverage will be generated.
Note: generating code coverage is slow.
Git ignore the build directory as created by PHPUnit to store the log files.
Adjust the "normal" test script to not generate code coverage information.
Add a bin/unit-tests-coverage to run the unit tests with code coverage enabled.
Add a Composer script to call the unit-tests-coverage script.

Code coverage recording: add @Covers tags

... to all unit test files, as well as enable strict coverage recording.

GaryJones · 2020-07-27T22:19:24Z

bin/unit-tests-coverage.txt

+#   ./bin/unit-test-coverage --coverage-html ./build/logs/
+#
+
+"$(pwd)/vendor/bin/phpunit" --filter WordPressVIPMinimum "$(pwd)/vendor/squizlabs/php_codesniffer/tests/AllTests.php" %*


What does the %* do here?

It will take any additional arguments you pass on the command line and append them.

Without it, the second example of how to run the command (line 13) wouldn't (shouldn't) work as --coverage-html ./build/logs/ would be thrown away instead of added to the command.

Hmm.. trying to remember now whether this is a Windows syntax or cross-platform. Would you mind testing both the commands ? IIRC you are on Mac or not ?

GaryJones

Approved, with one question.

jrfnl · 2020-07-27T23:29:18Z

I probably should have mentioned in the issue above that generating code coverage only works with Xdebug turned on. If it's turned off, the tests will still run, but no coverage reports are generated.

GaryJones

Running:

$ composer coverage
> bin/unit-test-coverage
sh: bin/unit-test-coverage: No such file or directory
Script bin/unit-test-coverage handling the coverage event returned with error code 127

I:

updated composer.json line to add the missing plural (bin/unit-test-coverage to bin/unit-tests-coverage)
renamed the file from unit-tests-coverage.txt to unit-tests-coverage (no extension)
Ran chmod +x unit-tests-coverage (to make the file executable).

Then tried the composer coverage command again:

> bin/unit-tests-coverage
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

Cannot open file "%*".

Trying the ./bin/unit-tests-coverage directly naturally gave me the same result.

Removing the %* from bin/unit-tests-coverage, and it ran successfully:

> bin/unit-tests-coverage
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

..........................................                        42 / 42 (100%)

43 sniff test files generated 117 unique error codes; 0 were fixable (0%)

Time: 6.16 seconds, Memory: 18.00 MB

OK (42 tests, 0 assertions)

Generating code coverage report in Clover XML format ... done

...and I now have a populated build/logs/clover.xml file.

jrfnl · 2020-07-28T00:14:40Z

@GaryJones And you've just demonstrated why I find it important that people actually test my PRs ;-) Thanks a lot for this!

txt extension removed (shouldn't have been there in the first place).
%* removed. Windows compatibility for the script is irrelevant as these type of scripts don't run on Windows anyway, so I got my own .bat files set up.
Typo in composer.json fixed.

Remaining:

Making the file executable (chmod). That's not something I can do on Windows, so would be great if someone else could add/adjust a commit to make the file executable.
Testing the command for taking additional arguments as per the second example ./bin/unit-test-coverage --coverage-html ./build/logs/.
When run like that, the code coverage report is generated in HTML as per the screenshot above, which is, of course, a lot more user friendly than a Clover XML report.
If this doesn't work out of the box, the command may need to be adjusted still - I have a niggling feeling $* might be the equivalent of the previous %* for *nix based OS-es.
If the ability to pass additional arguments is not needed, then the second example in the script file should be removed.

GaryJones · 2020-07-29T01:12:30Z

Making the file executable (chmod). That's not something I can do on Windows, so would be great if someone else could add/adjust a commit to make the file executable.

Done. Squashed into the commit that added the file in the first place (first commit).

Testing the command for taking additional arguments as per the second example ./bin/unit-test-coverage --coverage-html ./build/logs/.

As noted previously, the %* gave me an error. $* and $@ both worked, though I suspect they wouldn't work for Windows users. But...

If the ability to pass additional arguments is not needed, then the second example in the script file should be removed.

Since generation is slow, then this is likely to be done as a manual (not CI) task, so if a clover.xml is going to be generated, then we may as well generate the HTML output as well. I swapped out the %* for --coverage-html ./build/logs/, so now calling composer coverage generates the HTML.
(Squashed into the same first commit, and I also removed the extra example and fixed the plural-typo with the first example.)

jrfnl · 2020-07-29T09:59:55Z

@GaryJones Thank you very much for making those changes.

if a clover.xml is going to be generated, then we may as well generate the HTML output as well. I swapped out the %* for --coverage-html ./build/logs/, so now calling composer coverage generates the HTML.

For now, the clover file would then not be needed and the config to generate the HTML output could be added to the PHPUnit config file instead (and removed from the script).

Would you like me to change that ?

Other than that - the ability to pass extra command line arguments to the script can still be useful, both for the test as well as the coverage script.

As previously said, it's not relevant for me as I run these via my own bat scripts anyway, but as an example of typical other command line args I use:

bin/unit-tests --filter AlwaysReturnInFilter just runs the tests for that one sniff instead of all tests which can be useful while working on one particular sniff.

Since generation is slow, then this is likely to be done as a manual (not CI) task

I imagine it can be added to CI at some point, but yes, that's not high priority at the moment.

For reference - both PHPCompatibility and PHPCSUtils check code coverage during CI and as part of the required status checks for each PR.

The way this is set up:

Register with an external service which tracks code coverage across builds, The above both use Coveralls, but there are other services which can do this too.
For Coveralls, a config file is needed.
Then in Travis CI, we have slightly different set up:
- For non-PR/merge builds we don't run the tests against all versions, but do a quick test against just the high/low PHP and high/low PHPCS versions (like in WPCS)
- For PR/merge builds, we run the tests against all other PHP/PHPCS version combi's and run just the high/low combis from the quicktest stage with code coverage.

As the high/low PHPCS versions for VIPCS are so close, I can even imagine that for VIPCS a coverage stage with only two builds, PHP 7.4 + PHPCS master and PHP 5.4 + PHPCS "low" would be sufficient.

Requested changes have been made

GaryJones · 2020-07-29T10:29:22Z

For now, the clover file would then not be needed and the config to generate the HTML output could be added to the PHPUnit config file instead (and removed from the script).

Would you like me to change that ?

Yes please.

As previously said, it's not relevant for me as I run these via my own bat scripts anyway, but as an example of typical other command line args I use:

bin/unit-tests --filter AlwaysReturnInFilter just runs the tests for that one sniff instead of all tests which can be useful while working on one particular sniff.

In which case, please go ahead and restore $* (although one page I was reading yesterday suggested that $@ was the safer option), and include your example of being able to run tests for a single sniff in CONTRIBUTING.md / the shell script(s), as that would be useful to have for the future.

jrfnl · 2020-07-29T11:06:18Z

Done - I've made the base config + script change in the first commit and added a third commit adding the ability to pass additional arguments to both scripts.

jrfnl · 2020-07-29T11:09:27Z

Re: Contributing.md - I've not made any changes there (yet) as it looks like that whole section should probably get a rewrite. Currently looks to be taken largely from WPCS and doesn't account for the scripts used in this repo yet,

Might be more efficient to do that rewrite when PHPCSUtils is added, as that section would need to change then anyway.

GaryJones

When I run unit-tests, it works fine.

When I run unit-tests-coverage (having just deleted my local branch and checked it out again), I now get:

./bin/unit-tests-coverage
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

PHP Fatal error:  Cannot declare class WordPressVIPMinimum\Sniffs\Compatibility\ZoninatorSniff, because the name is already in use in /Users/gary/code/vipcs/WordPressVIPMinimum/Sniffs/Compatibility/ZoninatorSniff.php on line 17
PHP Stack trace:
PHP   1. {main}() /Users/gary/code/vipcs/vendor/phpunit/phpunit/phpunit:0
PHP   2. PHPUnit\TextUI\Command::main() /Users/gary/code/vipcs/vendor/phpunit/phpunit/phpunit:61
PHP   3. PHPUnit\TextUI\Command->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/TextUI/Command.php:162
PHP   4. PHPUnit\TextUI\TestRunner->doRun() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/TextUI/Command.php:206
PHP   5. PHP_CodeSniffer\Tests\TestSuite->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:652
PHP   6. PHP_CodeSniffer\Tests\TestSuite->run() /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/tests/TestSuite7.php:28
PHP   7. PHPUnit\Framework\TestSuite->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
PHP   8. PHPUnit\Framework\TestSuite->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
PHP   9. WordPressVIPMinimum\Tests\Compatibility\ZoninatorUnitTest->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
PHP  10. PHPUnit\Framework\TestResult->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestCase.php:796
PHP  11. WordPressVIPMinimum\Tests\Compatibility\ZoninatorUnitTest->runBare() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestResult.php:693
PHP  12. WordPressVIPMinimum\Tests\Compatibility\ZoninatorUnitTest->runTest() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestCase.php:842
PHP  13. WordPressVIPMinimum\Tests\Compatibility\ZoninatorUnitTest->testSniff() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestCase.php:1154
PHP  14. PHP_CodeSniffer\Ruleset->__construct() /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/tests/Standards/AbstractSniffUnitTest.php:149
PHP  15. PHP_CodeSniffer\Ruleset->registerSniffs() /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/src/Ruleset.php:217
PHP  16. PHP_CodeSniffer\Autoload::loadFile() /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/src/Ruleset.php:1159
PHP  17. include() /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/autoload.php:167

Fatal error: Cannot declare class WordPressVIPMinimum\Sniffs\Compatibility\ZoninatorSniff, because the name is already in use in /Users/gary/code/vipcs/WordPressVIPMinimum/Sniffs/Compatibility/ZoninatorSniff.php on line 17

Call Stack:
    0.0004     401816   1. {main}() /Users/gary/code/vipcs/vendor/phpunit/phpunit/phpunit:0
    0.0034     784680   2. PHPUnit\TextUI\Command::main() /Users/gary/code/vipcs/vendor/phpunit/phpunit/phpunit:61
    0.0034     784792   3. PHPUnit\TextUI\Command->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/TextUI/Command.php:162
    0.0918    7188136   4. PHPUnit\TextUI\TestRunner->doRun() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/TextUI/Command.php:206
    0.1093    7655264   5. PHP_CodeSniffer\Tests\TestSuite->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:652
    0.1093    7655264   6. PHP_CodeSniffer\Tests\TestSuite->run() /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/tests/TestSuite7.php:28
    0.1266    7656144   7. PHPUnit\Framework\TestSuite->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
    0.1287    7657024   8. PHPUnit\Framework\TestSuite->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
    0.1297    7659144   9. WordPressVIPMinimum\Tests\Compatibility\ZoninatorUnitTest->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
    0.1297    7659144  10. PHPUnit\Framework\TestResult->run() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestCase.php:796
    0.4381   11920376  11. WordPressVIPMinimum\Tests\Compatibility\ZoninatorUnitTest->runBare() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestResult.php:693
    0.4386   11936920  12. WordPressVIPMinimum\Tests\Compatibility\ZoninatorUnitTest->runTest() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestCase.php:842
    0.4386   11936920  13. WordPressVIPMinimum\Tests\Compatibility\ZoninatorUnitTest->testSniff() /Users/gary/code/vipcs/vendor/phpunit/phpunit/src/Framework/TestCase.php:1154
    0.4514   12115752  14. PHP_CodeSniffer\Ruleset->__construct() /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/tests/Standards/AbstractSniffUnitTest.php:149
    0.4526   12117928  15. PHP_CodeSniffer\Ruleset->registerSniffs() /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/src/Ruleset.php:217
    0.4526   12117928  16. PHP_CodeSniffer\Autoload::loadFile() /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/src/Ruleset.php:1159
    0.4528   12167568  17. include('/Users/gary/code/vipcs/WordPressVIPMinimum/Sniffs/Compatibility/ZoninatorSniff.php') /Users/gary/code/vipcs/vendor/squizlabs/php_codesniffer/autoload.php:167

jrfnl · 2020-07-30T15:07:45Z

@GaryJones Argh... ok, did some digging and the processUncoveredFilesFromWhitelist="true" was the cause as it loads the classes prior to running the tests which interferes with the PHPCS native autoloader.

I've removed that now from the first commit and 🤞 all should work fine again now, including generating the code coverage.

GaryJones · 2020-07-30T21:29:14Z

✅

./bin/unit-tests-coverage --filter AlwaysReturnInFilter
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

1 sniff test files generated 2 unique error codes; 0 were fixable (0%)

Time: 945 ms, Memory: 14.00 MB

❌

composer coverage
> bin/unit-tests-coverage
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

...........................................                       43 / 43 (100%)

44 sniff test files generated 120 unique error codes; 0 were fixable (0%)

Time: 6.48 seconds, Memory: 18.00 MB

OK (43 tests, 0 assertions)

Generating code coverage report in HTML format ... failed
Directory "/build/logs/" does not exist.

Not sure what's changed here, as I didn't have the /build/logs/ before, and they were created just fine previously.

jrfnl · 2020-07-30T21:38:28Z

Not sure what's changed here, as I didn't have the /build/logs/ before, and they were created just fine previously.

PHPUnit creates that fine for me. Could it be that there is still a lock on the name on your system as it previously existed ?
Have you tried manually creating the directory and running the command with the (empty) directory in place ?

rebeccahum · 2020-07-31T01:54:29Z

I get the same error as well even with manually running the command with the empty directory in place:

> bin/unit-tests-coverage
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

...........................................                       43 / 43 (100%)

44 sniff test files generated 120 unique error codes; 0 were fixable (0%)

Time: 6.85 seconds, Memory: 18.00 MB

OK (43 tests, 0 assertions)

Generating code coverage report in HTML format ... failed
Directory "/build/logs/" does not exist.

jrfnl · 2020-07-31T02:20:00Z

Hmm... just to see if it makes a difference, could you try changing the <log type="coverage-html" target="/build/logs/"/> to <log type="coverage-html" target="/build/logs"/> ? i.e. without the trailing slash in the path.

Oh, and another thing to try could be <log type="coverage-html" target="./build/logs"/>, i.e. adding a "." at the start.

rebeccahum · 2020-07-31T03:56:45Z

<log type="coverage-html" target="./build/logs/"/> worked:

> bin/unit-tests-coverage
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

...........................................                       43 / 43 (100%)

44 sniff test files generated 120 unique error codes; 0 were fixable (0%)

Time: 7.18 seconds, Memory: 20.00 MB

OK (43 tests, 0 assertions)

Generating code coverage report in HTML format ... done

👏

GaryJones · 2020-07-31T09:24:02Z

./build/logs and ./build/logs/ both worked for me.

./bin/unit-tests-coverage --filter AlwaysReturnInFilter also worked, though naturally all of the categories that weren't being tested get a zero.

* PHPUnit config: add code coverage configuration. By default, when there is a code coverage configuration and Xdebug is enabled, code coverage will be generated. For now, code coverage is set up for local use with an HTML code coverage report being generated in a `build/logs/` directory. Note: generating code coverage is slow. * Git ignore the `build` directory as created by PHPUnit to store the log files. * Adjust the "normal" test script to not generate code coverage information. * Add a `bin/unit-tests-coverage` to run the unit tests with code coverage enabled. * Add a Composer script to call the `unit-tests-coverage` script.

... to all unit test files, as well as enable strict coverage recording.

jrfnl · 2020-07-31T10:10:30Z

@rebeccahum @GaryJones Thank you both for testing. Seems that even PHPUnit has some cross-platform quirks.

I've added the . now to the config file.

GaryJones

LGTM, thanks!

rebeccahum

Thank you!

jrfnl added the Type: Maintenance label Jul 27, 2020

jrfnl added this to the 2.2.0 milestone Jul 27, 2020

jrfnl requested a review from a team as a code owner July 27, 2020 21:24

GaryJones reviewed Jul 27, 2020

View reviewed changes

GaryJones approved these changes Jul 27, 2020

View reviewed changes

GaryJones previously requested changes Jul 27, 2020

View reviewed changes

jrfnl force-pushed the add/code-coverage-checking branch from f43d947 to f640e6a Compare July 28, 2020 00:01

GaryJones force-pushed the add/code-coverage-checking branch from f640e6a to 45ea7cd Compare July 29, 2020 01:08

jrfnl force-pushed the add/code-coverage-checking branch from 45ea7cd to 29c834c Compare July 29, 2020 11:05

GaryJones reviewed Jul 30, 2020

View reviewed changes

jrfnl force-pushed the add/code-coverage-checking branch from 29c834c to 010cc39 Compare July 30, 2020 15:07

jrfnl force-pushed the add/code-coverage-checking branch from 010cc39 to 2e141c1 Compare July 31, 2020 10:08

jrfnl added 3 commits July 31, 2020 12:09

Code coverage recording: add @Covers tags

03697e0

... to all unit test files, as well as enable strict coverage recording.

Test scripts: add ability to pass additional command line arguments

2e141c1

GaryJones approved these changes Jul 31, 2020

View reviewed changes

rebeccahum approved these changes Jul 31, 2020

View reviewed changes

GaryJones merged commit 271708f into develop Aug 1, 2020

GaryJones deleted the add/code-coverage-checking branch August 1, 2020 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for checking test code coverage #560

Allow for checking test code coverage #560

jrfnl commented Jul 27, 2020

GaryJones Jul 27, 2020

jrfnl Jul 27, 2020

GaryJones left a comment

jrfnl commented Jul 27, 2020

GaryJones left a comment •

edited

Loading

jrfnl commented Jul 28, 2020

GaryJones commented Jul 29, 2020 •

edited

Loading

jrfnl commented Jul 29, 2020

GaryJones commented Jul 29, 2020

jrfnl commented Jul 29, 2020

jrfnl commented Jul 29, 2020 •

edited

Loading

GaryJones left a comment

jrfnl commented Jul 30, 2020

GaryJones commented Jul 30, 2020

jrfnl commented Jul 30, 2020

rebeccahum commented Jul 31, 2020

jrfnl commented Jul 31, 2020 •

edited

Loading

rebeccahum commented Jul 31, 2020

GaryJones commented Jul 31, 2020 •

edited

Loading

jrfnl commented Jul 31, 2020

GaryJones left a comment

rebeccahum left a comment

Allow for checking test code coverage #560

Allow for checking test code coverage #560

Conversation

jrfnl commented Jul 27, 2020

Commit details

Tests: add code coverage configuration

Code coverage recording: add @Covers tags

GaryJones Jul 27, 2020

Choose a reason for hiding this comment

jrfnl Jul 27, 2020

Choose a reason for hiding this comment

GaryJones left a comment

Choose a reason for hiding this comment

jrfnl commented Jul 27, 2020

GaryJones left a comment • edited Loading

Choose a reason for hiding this comment

jrfnl commented Jul 28, 2020

GaryJones commented Jul 29, 2020 • edited Loading

jrfnl commented Jul 29, 2020

GaryJones commented Jul 29, 2020

jrfnl commented Jul 29, 2020

jrfnl commented Jul 29, 2020 • edited Loading

GaryJones left a comment

Choose a reason for hiding this comment

jrfnl commented Jul 30, 2020

GaryJones commented Jul 30, 2020

jrfnl commented Jul 30, 2020

rebeccahum commented Jul 31, 2020

jrfnl commented Jul 31, 2020 • edited Loading

rebeccahum commented Jul 31, 2020

GaryJones commented Jul 31, 2020 • edited Loading

jrfnl commented Jul 31, 2020

GaryJones left a comment

Choose a reason for hiding this comment

rebeccahum left a comment

Choose a reason for hiding this comment

GaryJones left a comment •

edited

Loading

GaryJones commented Jul 29, 2020 •

edited

Loading

jrfnl commented Jul 29, 2020 •

edited

Loading

jrfnl commented Jul 31, 2020 •

edited

Loading

GaryJones commented Jul 31, 2020 •

edited

Loading