Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Method with "return with multiline constant expr only" is not generating any coverage lines #946

Closed
mvorisek opened this issue Oct 16, 2022 · 6 comments

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Oct 16, 2022

Q A
php-code-coverage version 9.2.17 / latest
PHP version 8.0.24
Driver Xdebug
Xdebug version 3.2.0RC1
Installation Method Composer
Usage Method PHPUnit
PHPUnit version 9.5.25

code:

https://github.com/atk4/data/blob/0c6bce5c2ac53589934b02bc0832a4753c283f15/src/Persistence/Sql/Oracle/PlatformTrait.php#L104-L109

is not generating any coverage line:

...
    <file name="/__w/data/data/src/Persistence/Sql/Oracle/PlatformTrait.php">
      <class name="Atk4\Data\Persistence\Sql\Oracle\PlatformTrait" namespace="global">
        <metrics complexity="7" methods="5" coveredmethods="5" conditionals="0" coveredconditionals="0" statements="24" coveredstatements="24" elements="29" coveredelements="29"/>
      </class>
      <line num="12" type="method" name="getVarcharTypeDeclarationSQL" visibility="public" complexity="1" crap="1" count="209"/>
      <line num="14" type="stmt" count="209"/>
      ...
      <line num="74" type="stmt" count="231"/>
      <line num="94" type="stmt" count="231"/>
      <line num="95" type="stmt" count="231"/>
      <line num="96" type="stmt" count="231"/>
      <line num="99" type="stmt" count="231"/>
      <line num="101" type="stmt" count="231"/>
      <metrics loc="121" ncloc="101" classes="1" methods="5" coveredmethods="5" conditionals="0" coveredconditionals="0" statements="24" coveredstatements="24" elements="29" coveredelements="29"/>
    </file>
...

image

I would expect at least one coverage line for the getListDatabasesSQL method to be able to tell if called or not

@mvorisek
Copy link
Contributor Author

Hi @Slamdunk, do you think this can be fixed?

@Slamdunk
Copy link
Contributor

It definitively should

@mvorisek
Copy link
Contributor Author

mvorisek commented Oct 20, 2022

here is minimalistic code to reproduce:

class Cl
{
    public function getSql(): string
    {
        return 'select 1 from' . ' from dual';
    }
}

generates a coverage line, but

class Cl
{
    public function getSql(): string
    {
        return 'select 1 from'
            . ' from dual';
    }
}

does not, notice same AST, for some reasons, no coverage is generated if the the string is concat across multiple lines

the problem is xdebug generates coverage on a line where the last string is defined (l5 for first example, l6 for the 2nd), but executable line from static analysis is always on l5

I have looked into the source - https://github.com/sebastianbergmann/php-code-coverage/blob/main/src/StaticAnalysis/ExecutableLinesFindingVisitor.php - but I have not enough knowledge where executable lines should be generated.

Help wanted with the fix.

Also I propose to run the static analyser against some large project like Symfony (and maybe php-src tests) with random NL between AST tokens to verify the start line/end line rules are correctly used.

@mvorisek mvorisek changed the title Method with "return string" expr only is not generating any coverage lines Method with "return with multiline constant expr only" is not generating any coverage lines Oct 20, 2022
@sebastianbergmann
Copy link
Owner

I was able to reproduce this.

src/C.php

<?php declare(strict_types=1);
final class C
{
    public function one(): string
    {
        return 'foo' . 'bar';
    }

    public function two(): string
    {
        return 'foo'
            . 'bar';
    }
}

tests/CTest.php

<?php declare(strict_types=1);
use PHPUnit\Framework\TestCase;

/**
 * @covers C
 */
final class CTest extends TestCase
{
    public function testOne(): void
    {
        $this->assertSame('foobar', (new C)->one());
    }

    public function testTwo(): void
    {
        $this->assertSame('foobar', (new C)->two());
    }
}

Code Coverage Report

grafik

@Slamdunk
Copy link
Contributor

I'm going to look at this as soon as I can

@sebastianbergmann
Copy link
Owner

Once again: thank you so much, @Slamdunk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants