Incorrect marking of code as 'not executed' following merging of cov files

[iainfogg]

Hi, I'm running using Robo (http://robo.li/) to run a series of Codeception unit tests which are then having the .cov files merged (using phpcov) and an HTML file produced to show code coverage - I mention all the steps in case that's relevant.

However, I'm finding that quite a few lines are being marked as 'not executed' when, depending on your perspective, they either were executed or are dead code (unless I'm misunderstanding how this should work). For example, the first brace here - also, I don't know if this is the correct marking for the closing brace.

Also, in this one, how can line 52 be not executed?

Or in this one, how can line 54 be not executed code?

My composer.lock file shows php-code-coverage at version 2.2.4, and phpcov at 2.0.2. phpUnit is at 4.8.19 and codeception is at 2.1.4.

Any ideas on what the issue is, or anything I can do to help get to the bottom of it?

Thanks, Iain

[iainfogg]

OK, I have some insight into what's gone on here. The issue is related to the merging of cov files prior to producing the report.

For files which were not executed, but were required to appear in the HTML report, the list of lines is generated, but by a relatively inaccurate (compared to executed code) assessment of which lines are executable and should therefore be monitored. Essentially, this includes most of the lines from the opening bracket in a method up to and including the closing bracket of each method.

For files which are actually executed, as I understand it they are analysed by Xdebug to get a definitive list of executable lines in that file. This does not include the opening bracket of each method, and doesn't include certain other lines.

The issue is coming because the two approaches are generating a different list of lines - the non-executed code has more lines in it than the executed code.

When a single run is being done, this is never an issue; however, when two cov files are being merged, and the two runs include a class which was executed in one run / cov file, but not in the other run / cov file, a bunch of lines (the ones which only appear in the non-executed code) get added into the merged file but can never be marked as having been executed, thereby throwing the stats for that particular class.

I have a solution which seems to be working for me, although there may be a better approach that someone can suggest? However, the idea is that if the number of lines differ between two cov files for a particular file, always accept by default the one with the fewest lines (as that must be the executed one, which will have the most accurate list of lines that can be run). Doing so avoids tagging lines incorrectly as not executed in the final report.

Does this seem a sensible approach? If so, I can do a PR for it.

I've added this code in the merge method, just before going through and doing the line by line merging of results, to avoid that if there are lines and they don't match in number:

            // Check if there are some lines already present in the two
            // versions being merged, and also that the number of lines
            // in each is different. This indicates that one is based on
            // pulling lines in from glancing through the code in unexecuted
            // code (the one with more lines in), and one is based on using Xdebug
            // to properly analyse the code (the one with less lines in).
            // In this scenario, ditch the lines from the unexecuted analysis,
            // as it will contain lines which are not classed as executable by
            // Xdebug, and those lines will throw the code analysis by appearing
            // as if they should be run but weren't.
            if ((count($lines) > 0)
                && (count($this->data[$file]) > 0)
                && (count($lines) != count($this->data[$file]))
            ) {
                if (count($lines) > count($this->data[$file])) {
                    // More lines in the one being added in, so just ignore them
                    // and move on to the next file.
                    continue;
                } else {
                    // More lines in the one we currently have, so ditch what we
                    // currently have and take what's being merged in exactly as
                    // it is.
                    $this->data[$file] = $lines;

                    // Move on to the next file.
                    continue;
                }
            }

Thanks, Iain

[dunglas]

Hello,

I'm having the exact same issue. If you agree to include @iainfogg's fix, I can open the PR.

Thanks.

[KintradimCrux]

Hello,

I'm having the exact same issue - running only one test class shows 100% of executable lines are covered, while when running all tests the same class shows random "not executed" lines that either contain comments or definitely were executed. I just ran the tests with @iainfogg's fix and it now shows the correct coverage.

[sebastianbergmann]

@iainfogg Please send a pull request, thanks.

[iainfogg]

Apologies to @dunglas - I never saw his comment from August 2016.

I have added the PR as requested #542

However, that pull request is based on the work I did 18 months ago which was in branch 2.2. Obviously that's pretty old now; maybe someone can either advise how to get this into later branches too if needed, or consider whether that fix is likely to be compatible with later branches, as I have not looked at anything later.

It's my first PR, so be gentle with me :)

Thanks @sebastianbergmann

[KintradimCrux]

@iainfogg I inserted your code sample inside the outer foreach loop of the method merge() just before the inner foreach loop is run, it seems to work with both phpunit 6.2.3 and 5.7.21, respectively the php-code-coverage version they have as dependency.

[iainfogg]

So should I be looking at adding this to every branch (2.2, 3.0, 3.1, 3.2, 3.3, 4.0, 5.0 and master)? And is it a separate PR for each one? Or should I just add it to master? @KintradimCrux

[sebastianbergmann]

As you can see on at https://phpunit.de/build-status.html, php-code-coverage 4.0 is the oldest, still supported branch. So please send a pull request to the 4.0 branch and I'll take it from there. Thanks!

[iainfogg]

@sebastianbergmann I will be doing this and coming up with a new pull request on 4.0. The code's easy to add, but the data in the tests looks like it's going to take a reasonable amount of going through so it'll pass tests and properly test the new code. I'll let you know once it's done.

[KintradimCrux]

@sebastianbergmann I only tested this with version 4.0.8 and 5.1.2 of the php-code-coverage package, i cannot tell if this works with older versions

[pixelcat]

Gentle bump on this one. Looks like the build on this failed for an unrelated reason.

[iainfogg]

Sorry @pixelcat I am struggling to get the time to fix the failed build, as I don't understand enough of what's making that other test fail. If anyone else can pick this up so this can get implemented, that would be great.