Refactor getLinesToBeIgnored() so it can work with branch/path data

[dvdoug]

For #380

Hello @sebastianbergmann

Take the example code below from the test suite

/**
 * @codeCoverageIgnore
 */
class Foo
{
    public function bar()
    {
    }
} 

That is easy to understand when dealing with line based coverage (ignore lines 4-9), and also with branch/path coverage (ignore all functions in the class).

However, it is possible to specify that individual lines should be ignored, not just whole functions e.g.

class Foo
{
    public function bar()
    {
        try {
            //some logic
         }
         catch ($e) {
             // @codeCoverageIgnoreStart
            $this->sendAlertEmail();
            throw new ThisShouldNeverHappenException() 
            // @codeCoverageIgnoreEnd
        }
    }
}

Branch/path coverage in this situation is much less obvious. There are 2 possible solutions (1) disallow ignoring branch/path coverage for only part of a function (2) treat the annotation as referring to the branch it's found in, and ignore that.

Option 2 sounds much better to me and is what I'm implementing. However, in testing I've found that getLinesToBeIgnored() performs 2 different types of "ignore". The first is the type above, where the code being tested explicitly asks for it. The second is code that ignores any source lines that aren't executable, which AFAICT exists solely for the purpose of dealing with the output of addUncoveredFilesFromWhitelist() even though it's called on every iteration.

This has consequences when trying to implement option 2 because blank/empty lines are considered non-executable under line based coverage. Ignoring every branch that includes a blank line means that most become accidentally ignored.

For paths, a similar undesirable effect happens with the function declaration - under line based coverage this is considered non-executable, however under path coverage the entry into the function is considered to be the line with the declaration on it. Since every possible path starts at function entry the current version of getLinesToBeIgnored() has the side effect of removing literally every single one!

To handle this, I've split the getLinesToBeIgnored() logic that exists to handle addUncoveredFilesFromWhitelist() into a seperate function. This logic exists to operate on line based coverage only and can be segregated. The remaining logic handles code annotations only and can therefore be used on both line and branch/path coverage.

There is one minor change to coverage as a result of this split, and that is handling of the keyword "function". Previously any line featuring this was always ignored, now it is only ignored when adding uncovered files. This means that in covered files, lines declaring anonymous functions are now included in the line coverage data rather than excluded. This seems correct to me anyway.

[codecov]

Codecov Report

Merging #757 into master will decrease coverage by 0.12%.
The diff coverage is 96.36%.

@@             Coverage Diff              @@
##             master     #757      +/-   ##
============================================
- Coverage     84.57%   84.45%   -0.13%     
- Complexity      872      881       +9     
============================================
  Files            38       40       +2     
  Lines          2554     2566      +12     
============================================
+ Hits           2160     2167       +7     
- Misses          394      399       +5     

Impacted Files	Coverage Δ	Complexity Δ
src/CodeCoverage.php	65.51% <80.00%> (-4.16%)	133.00 <0.00> (-24.00)
src/RawCodeCoverageData.php	98.76% <98.00%> (-1.24%)	44.00 <33.00> (+33.00)	⬇️
src/Node/Iterator.php	70.58% <0.00%> (-4.42%)	10.00% <0.00%> (ø%)
src/Node/AbstractNode.php	78.72% <0.00%> (-0.85%)	37.00% <0.00%> (ø%)
src/Node/Directory.php	90.16% <0.00%> (-0.75%)	51.00% <0.00%> (ø%)
src/Exception/RuntimeException.php	0.00% <0.00%> (ø)	0.00% <0.00%> (?%)
src/Exception/InvalidArgumentException.php	0.00% <0.00%> (ø)	0.00% <0.00%> (?%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d5d9ce...3cc8003. Read the comment docs.

[sebastianbergmann]

This seems correct to me anyway.

It's not, though, at least not in my opinion. function is only relevant at compile-time, it is not something that is executed at runtime. But I think that I can live with that.

[dvdoug]

This seems correct to me anyway.

It's not, though, at least not in my opinion. function is only relevant at compile-time, it is not something that is executed at runtime. But I think that I can live with that.

As a bare keyword I agree, so let me rephrase to add some nuance: "...in covered files, lines declaring anonymous functions are now included in the line coverage data rather than excluded where the driver considers the line to be executable."

e.g. I agree that line 3 below should probably not count as executable.

        array_walk(
            $filter,
            function (&$val, $key) {
                $val = preg_replace('|[^0-9]|', '', $val);
            }
        );

but if was reformatted to a 1-liner, then the entire line should not be removed from code coverage just because part of it contains a function.

array_walk($filter,function (&$val, $key) {$val = preg_replace('|[^0-9]|', '', $val);});