Uncovered new line at end of file

[davidgrayston]

Q	A
php-code-coverage version	9.1.4
PHP version	7.4
Driver	Xdebug
Xdebug version	2.9.6
Installation Method	Composer
Usage Method	PHPUnit
PHPUnit version	9.3.7

Since upgrading from PHPUnit 9.2, new lines at the end of files are being reported as uncovered in the XML report, e.g.

<line num="32" type="stmt" count="0"/>

It seems to be caused by the removal of this workaround (as reinstating it fixes the clover report): d9c9b7b

The report is being consumed by SonarCloud - should the last line of files be ignored regardless of the uploaded XML report? If that's the case, I can raise an issue with SonarCloud.

[sebastianbergmann]

Thank you for your report.

Please provide a minimal, self-contained, reproducing test case that shows the problem you are reporting.

Without such a minimal, self-contained, reproducing test case I will not be able to investigate this issue.

[davidgrayston]

I've created a small demo here: https://github.com/davidgrayston/phpunit-demo/tree/master/php-code-coverage-799

You can run cd php-code-coverage-799 and composer demo to see a report diff

[sebastianbergmann]

CC @dvdoug @derickr

[derickr]

PHP reports a line "14" for the implicit return at the end of a file. This is not a bug in php-code-coverage or Xdebug:

derick@singlemalt:/tmp$ php -dvld.active=1 Demo.php 
Finding entry points
Branch analysis from position: 0
1 jumps found. (Code = 62) Position 1 = -2
filename:       /tmp/Demo.php
function name:  (null)
number of ops:  3
compiled vars:  none
line     #* E I O op                           fetch          ext  return  operands
-------------------------------------------------------------------------------------
    3     0  E >   NOP                                                      
   14     1        EXT_STMT                                                 
          2      > RETURN                                                   1

branch: #  0; line:     3-   14; sop:     0; eop:     2; out0:  -2
path #1: 0, 
Class DG\Demo:
Function hello:
Finding entry points
Branch analysis from position: 0
1 jumps found. (Code = 62) Position 1 = -2
filename:       /tmp/Demo.php
function name:  hello
number of ops:  6
compiled vars:  none
line     #* E I O op                           fetch          ext  return  operands
-------------------------------------------------------------------------------------
    9     0  E >   EXT_NOP                                                  
   11     1        EXT_STMT                                                 
          2      > RETURN                                                   'hello'
   12     3*       EXT_STMT                                                 
          4*       VERIFY_RETURN_TYPE                                       
          5*     > RETURN                                                   null

branch: #  0; line:     9-   12; sop:     0; eop:     5
path #1: 0, 
End of function hello

End of class DG\Demo.

[davidgrayston]

The introduction of this line in the report has affected coverage reporting on SonarCloud - should I follow this up with SonarCloud linking to this issue?

[davidgrayston]

Link for reference https://community.sonarsource.com/t/php-coverage-uncovered-new-line-at-end-of-file/29888

[davidgrayston]

I understand the issue is now closed (so I apologise for replying), but I thought it's worth mentioning that setting processUncoveredFiles="false" results in the line being covered:

<line num="14" type="stmt" count="1"/>

I don't fully understand why this is happening - perhaps the file is loaded when gathering uncovered files (so executes the line at the end of the file), but not loaded again by the autoloader when running the tests.

[dvdoug]

Hi @sebastianbergmann

Notwithstanding the analysis by @derickr (thanks!), I think this should be reopened. I feel this is one of those cases where being technically correct is not the best kind of correct. There are few overlapping reasons:

• The fact there that the PHP interpreter thinks there is an implicit return on line 14 is completely unintuitive. It's an empty line! Every other empty line in the file is considered non-executable.
• The fact that the coverage results for the same file are different depending on what would seem to be a completely unrelated configuration setting processUncoveredFiles tips the balance in favour of adding code to ensure consistency for me. This isn't simply a case of Xdebug and PHPDBG disagreeing on what a covered line is, this is a line showing as uncovered that developers are not able to exercise in their tests at all
• The fact that the HTML report explicitly removes the empty line when producing its line by line output (https://github.com/sebastianbergmann/php-code-coverage/blob/master/src/Report/Html/Renderer/File.php#L1008) means there is inconsistency in results between report formats
• The fact that despite line 14 being shown as uncovered in the clover report, 100% line coverage is still being claimed by all of the other reports. There's an inconsistency in the maths here. The clover report itself even claims there are 13 lines, but then outputs data for line 14. That...doesn't seem right.

<?xml version="1.0" encoding="UTF-8"?>
<coverage generated="1598109988">
  <project timestamp="1598109988">
    <file name="C:\tmp\phpunit-demo\php-code-coverage-799\src\Demo.php">
      <class name="DG\Demo" namespace="global">
        <metrics complexity="1" methods="1" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="1" coveredstatements="1" elements="2" coveredelements="2"/>
      </class>
      <line num="9" type="method" name="hello" visibility="public" complexity="1" crap="1" count="1"/>
      <line num="11" type="stmt" count="1"/>
      <line num="14" type="stmt" count="0"/>
      <metrics loc="13" ncloc="13" classes="1" methods="1" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="1" coveredstatements="1" elements="2" coveredelements="2"/>
    </file>
    <metrics files="1" loc="13" ncloc="13" classes="1" methods="1" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="1" coveredstatements="1" elements="2" coveredelements="2"/>
  </project>
</coverage>

Since every other part of the codebase (and developer expectation) seems to think that example file only has 13 lines, I think the clover report should be adjusted to match and strip off the last line of a file if it's empty to match.

[sebastianbergmann]

I think the clover report should be adjusted to match and strip off the last line of a file if it's empty to match.

I would like to avoid handling this in the report(s) and rather clean up the data.

[derickr]

It should be fixed in PHP.

[derickr]

this is a line showing as uncovered that developers are not able to exercise in their tests at all

That's not true though, the line should show as covered if the file is included.

[dvdoug]

That's not true though, the line should show as covered if the file is included.

When processUncoveredFiles is false, the first test that happens to trigger the autoloader for that class will execute the line as PHP parses the file for the first time. The line will thus be (unintentionally?) covered by that first test (not sure how that interacts with strict @Covers annotations)

However, when processUncoveredFiles is true, all of the source code files are loaded before the test suite is run[1]. Thus although the line may be executed by the PHP engine, it is never done so in the context of an individual test and from the POV of a PHPUnit user it doesn't matter how many tests they write, they won't be able to get that line marked as covered.

[1] Loading of source files appears to be done before the test suite rather than after as a part of a Xdebug performance hack for dead code detection, see #433 and #384

[sebastianbergmann]

I wonder whether we can get rid of the processUncoveredFiles option and loading all of the source code files before the tests are run now that we have AST-based static analysis in play. I think the ExecutableLinesFindingVisitor is good enough at figuring out whether a line of code is executable or not. This would (almost certainly) mean that we would no longer be able to show whether or not a line of code is dead code in the reports. But this is functionality I would be willing to sacrifice as a lot of users are confused by it anyways. And if we no longer highlight dead code as such in reports then we no longer need to pass the XDEBUG_CC_DEAD_CODE flag to Xdebug.

[dvdoug]

processUncoveredFiles isn't just doing the additional dead code detection speedup hack anymore, it also generates branch and path data for those files. I think "write a test if you want to see the numbers" wouldn't be completely unreasonable, but since using path data generates a more accurate CRAP score then there's an edge case where the project risks dashboard might show a higher score after that first test is written and the file is processed through Xdebug than when calculating using the AST line count. I think that would need to be flagged up in documentation as it could be surprising since writing a test is usually expected to reduce the score.

It's also completely possible that the preloading code isn't necessary anymore...looking at the original issue that added it, the justification was that a lot of time was spent doing analysis on code in vendor etc that wouldn't ever be needed. However we now use Xdebug's filter to exclude those files up front so it might be possible now to load the uncovered files afterwards without regressing performance.

Time for some benchmarking...

[dvdoug]

The PR in #800 will ensure that line 14 from this example has consistent raw data to work at least with regardless of internal settings

@sebastianbergmann how do you want to resolve the wider issues? I see two questions:

1. Should a newline at the end of the file be considered a source code line or not? Does the file have 13 or 14 lines?
2. If 14, should we expose the engine's implicit return to userland and mark the line executable, or is it an implementation detail that should be hidden away?

[sebastianbergmann]

Should a newline at the end of the file be considered a source code line or not? Does the file have 13 or 14 lines?

How about considering empty lines to be not executable in general?

[remorhaz]

Seems that this bug is reintroduced in 9.1.7 - I look into --coverage-clover report and see uncovered EOF strings again (same picture occured before 9.1.6).

[sebastianbergmann]

Of course, the whole point of 9.1.7 was to revert this (so that it can be properly fixed later). See #805 (comment) for details.