Fix code coverage for main framework repo

[swissspidy]

See #241

To test:

1. Check out the wp-cli/wp-cli repo
2. Run composer install
3. Copy the changes in this file to vendor/wp-cli/wp-cli-tests/utils/generate-coverage.php
4. Run WP_CLI_TEST_COVERAGE=1 composer behat features/aliases.feature:3

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/Context/FeatureContext.php	0.00%	20 Missing ⚠️

📢 Thoughts on this report? Let us know!

[mrsdizzie]

I did some testing and at least in the wp-cli-dev project / envorionment this line causes an infinite (or too long to wait for) hang, probably because of the complicated way that project uses symlinks and wp-cli-tests requiring wp-cli and vice versa. Adding some debugging into the eventual iterator from coverage code shows stuff like:

Scanning file: /Users/isla/source/wp-cli-dev/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/wp-cli-tests/vendor/wp-cli/media-command/vendor/wp-cli/checksum-command/checksum-command.php -> Resolved: /Users/isla/source/wp-cli-dev/checksum-command/checksum-command.php

Was able to reproduce this by just having $HOME/source/wp-cli-dev/wp-cli/bin
in my PATH and running:
wp --require=/Users/isla/source/wp-cli-dev/wp-cli-tests/utils/generate-coverage.php core is-installed

I noticed this because running the tests were hanging as well

[swissspidy]

Hmm interesting, haven't checked it in that combo yet. Any ideas off the top of your head to fix it?

[swissspidy]

Perhaps the scan should be rather based on the current working directory?

[mrsdizzie]

I'm not very familiar with the source but when I looked in the filter code includeDirectory is listed as deprecated so maybe we shouldn't use that anyway? Off the top of my head I think we can do our own search for -command.php files that knows to avoid links/vendor etc... and then use includeFiles (which is not marked as deprecated).

[swissspidy]

Yeah we can use RecursiveDirectoryIterator directly to make this more future proof.

[mrsdizzie]

I wouldn't be totally surprised if this is the source of the random hangs / long runs on some projects too

[swissspidy]

I just pushed a change using iterators. Did some slight testing already locally, but have to say the wp-cli-dev symlinking makes this a bit more complicated.

[mrsdizzie]

Is the only goal for the -command.php part to find php files that define a wp cli command which aren't inside of $project/php and $project/src that we've already looked in? And does this need to find those in vendor folders as well? or only project level files?

I think we can come up with something but I don't understand the original goal of what we're trying to do here.

[swissspidy]

Is the only goal for the -command.php part to find php files that define a wp cli command which aren't inside of $project/php and $project/src that we've already looked in?

Yes.

And does this need to find those in vendor folders as well? or only project level files?

Only project level files.

Let's say we're running WP_CLI_TEST_COVERAGE=1 composer behat within https://github.com/wp-cli/entity-command/

We want to get coverage for /entity-command.php and everything in src/, and ignore everything else.

[mrsdizzie]

OK Here are some changes I think might be helpful based on some testing:

First I think using SRC_DIR and trying to chain dirname calls is not going to work with the wp-cli-dev project because as it is now, nothing records what directory the test was run in so it would never be able to know what folder to look for files inside of. If we rely on _DIR_, it can't work because in wp-cli-dev the vendor folder which has this code is symlinked to a folder outside of the actual command we are running tests for.

For example, I have wp-cli-dev checked out as /Users/isla/source/wp-cli-dev and right now if I run composer behat inside of /Users/isla/source/wp-cli-dev/wp-cli its going to set PROJECT_DIR to /Users/isla because vendor/wp-cli is actually wp-cli -> ../../vendor/wp-cli so _DIR_ is always relative to wp-cli-tests outside of the project folder .

I suggest adding a new variable that records the directory that composer behat is run from:

private static $behat_run_dir;

// inside of prepare function ()
self::$behat_run_dir = getcwd();

// Then set internal variable

$variables = [
  'FRAMEWORK_ROOT' => realpath( $framework_root ),
  'SRC_DIR'        => realpath( dirname( dirname( __DIR__ ) ) ),
  'PROJECT_DIR'    => realpath( dirname( dirname( dirname( dirname( dirname( __DIR__ ) ) ) ) ) ),
  'TEST_RUN_DIR'   =>  self::$behat_run_dir,
];

// and put it in env too
$env            = [
  'PATH'         => $bin_path . $path_separator . getenv( 'PATH' ), 
  'BEHAT_RUN'    => 1,
  'HOME'         => sys_get_temp_dir() . '/wp-cli-home',
  'TEST_RUN_DIR' => self::$behat_run_dir,
];

Since the tests can only be run from the root of the project (I think), that is what we would set $project_dir from. Then it will always be correct no matter what context its run from. In my testing, this makes --require={TEST_RUN_DIR}/vendor/wp-cli/wp-cli-tests/utils/generate-coverage.php" correct in both cases (wp-cli-dev and normal checkout) and if I do this:

$project_dir =  (string) getenv( 'TEST_RUN_DIR' );

It is always correct -- and files for coverage seem populated correctly as well with the new iterator code. Then we don't need any of the current project / package folder detection. Might possibly need a single exception for wp-cli-tests itself.

If you think that sounds reasonable to explore, I can push those changes to this PR if you want to test (or create another PR to test)

[swissspidy]

Thanks, that all sounds clear to me and would nicely simplify the current logic. Feel free to push changes directly to this PR :)

We can now also use WP_CLI_EARLY_REQUIRE instead of --require. Just need to preserve any existing WP_CLI_EARLY_REQUIRE value and add a comma.

[mrsdizzie]

hmm well switching to WP_CLI_EARLY_REQUIRE brings all new kinds of problems now because it runs earlier and isn't enough to do:
require_once "{$project_dir}/vendor/autoload.php"; (which runs when SebastianBergmann\CodeCoverage\Filter is not already loaded. You start seeing errors like:

PHP Fatal error:  Uncaught Error: Call to undefined function WP_CLI\Dispatcher\get_path() in php/WP_CLI/Runner.php:94

Presumably because wp itself needs some kind of custom bootstrap process ... This is getting into some composer / autoload weeds that I don't really know enough about. This doesn't happen with --require presumably because it isn't run until later on and wp has already bootstrapped itself properly. Any ideas for that?

[swissspidy]

Agh, would have been too easy 🙃 Yes WP_CLI_EARLY_REQUIRE runs before any WP-CLI bootstrap logic... Sounds like another rabbit hole.

Then let's go with your proposed approach with the regex, since you've already written it out above. Sorry for the back and forth!

[mrsdizzie]

@swissspidy ok put up some changes to try

[swissspidy]

@mrsdizzie Just to clarify, does this now work with aliases?

[mrsdizzie]

yes -- what fixes the previous alias problem is putting the --require at the end of the wp command instead of the beginning (since an alias must be the first thing after wp). All this extra code is needed because you can't just stick --require at the end of the full command because it might look like wp eval xyz | something else so you need to split on pipes (if present) and make sure its only put on the end of a wp command

[swissspidy]

Ah I see it now. Tested with features/aliases.feature and it works as expected there.

Path resolution in wp-cli-tests itself works too, even within wp-cli-dev.

So I think this is good to go. You would need to approve it though :)

[mrsdizzie]

I'm happy to do a follow up PR with tests if you have any idea how to actually add tests for this to satisfy codecov. I wrote a unit test I thought would handle at least the coverage part but since it doesn't actually write coverage files until inside an on_shutdown function I wasn't able to figure out how to actually verify that easily...