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

Code coverage not recorded for setting of static variables #913

Closed
jrfnl opened this issue Apr 11, 2022 · 8 comments
Closed

Code coverage not recorded for setting of static variables #913

jrfnl opened this issue Apr 11, 2022 · 8 comments
Labels

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Apr 11, 2022

Q A
php-code-coverage version 9.2.15
PHP version 8.1.4
Driver Xdebug
Xdebug version (if used) via GH Actions, I'm presuming the latest version, but can check if needed
Installation Method Composer
Usage Method PHPUnit
PHPUnit version (if used) 9.5.20

Given the following code

class Foo
{
    public static function bar()
    {
        static $baz; // Shows as covered.
        
        if (isset($baz) === false) { // Shows as covered.
            // All code within this condition will always show as
            // "executable, but not executed", even though the code
            // will absolutely be run during the tests as otherwise
            // the tests would fail.
            $baz = (strpos('This is just an example', 'example') !== false);
        }
        
        return $baz; // Shows as covered.
    }
}

With a test along the lines of:

class FooTest extends PHPUnit\Framework\TestCase
{
    public function testBar() {
        $this->assertTrue(Foo::bar());
    }
}

I'd expect

That the lines within the condition, which set the static variable would show as covered...

But instead this happened

They don't show as covered.

Additional information

Some examples of coverage reports where this can be seen:

I have a feeling this may be the same issue as #23 which was closed due too little information having been provided to be actionable.

Happy to set up a clonable reproduction case is so desired or alternatively, the PHPCSUtils repo can be used to reproduce the issue.

Please let me know if I can provide any additional details to help debug this.

@sebastianbergmann
Copy link
Owner

CC @derickr

@sebastianbergmann
Copy link
Owner

There can be many reasons why a specific line is not covered. Each of them is likely a different issue. In order to be able to reproduce anything and not get bogged down with details, you need to provide the following in ONE new issue per case.

That means:

  • The exact versions of (this is what the template requires too, but more):
    • PHP
    • PHP Code Coverage
    • Xdebug and/or PCOV
    • PHPUnit
  • php -v output
  • Either a .tar.gz archive or a GitHub repository with a reproduce case. With one file that shows the problem and one test case file (with the exception that sometimes, you might need two source files to show a problem.

This information should be filed as one specific case per issue.

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 11, 2022

Working on setting up a reproduction example now (as you surmised the code above was just a code pattern, not actual code with which the issue could be reproduced).

@derickr
Copy link
Contributor

derickr commented Apr 11, 2022

Please also add which Xdebug version you're using, as sometimes I do fix these problems. For example https://bugs.xdebug.org/view.php?id=2075 that was fixed in 3.1.4.

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 11, 2022

Hmm.. this is weird... both of the above classes use the exact same code pattern as outlined above, but when using a minimal code sample, I can't reproduce it.

Will keep trying to figure out a minimal reproduction scenario, but may be (a lot) harder than I thought.

As for the php -v output - this is what GH Actions showed me:

Run php -v
PHP 8.1.4 (cli) (built: Apr  4 2022 13:30:17) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.4, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.4, Copyright (c), by Zend Technologies
    with Xdebug v3.1.2, Copyright (c) 2002-2021, by Derick Rethans

So, yes, looks like the Xdebug version is out of date, so maybe I should chase that a little and only continue work on the reproduction sample once I have confirmed that the issue still exists with Xdebug 3.1.4.

@derickr
Copy link
Contributor

derickr commented Apr 11, 2022

FWIW, I don't think this is an Xdebug issue. It's very likely something with this code being run once before the tests are run. Because the static persists between multiple calls to the function, it's very possible that this just "cashed". What happens if you use process isolation? (https://phpunit.readthedocs.io/en/9.5/configuration.html#the-processisolation-attribute)

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 11, 2022

I managed to get a test run done with Xdebug 3.1.4, which didn't change anything.

      - name: Install PHP
        uses: shivammathur/setup-php@v2
        with:
          php-version: ${{ matrix.php }}
          ini-values: ${{ steps.set_ini.outputs.PHP_INI }}
          extensions: xdebug-3.1.4
          coverage: xdebug

FWIW, I don't think this is an Xdebug issue. It's very likely something with this code being run once before the tests are run. Because the static persists between multiple calls to the function, it's very possible that this just "cashed". What happens if you use process isolation? (https://phpunit.readthedocs.io/en/9.5/configuration.html#the-processisolation-attribute)

@derickr That's a very good point. As depending on the order in which the tests are run, this could be the case for these functions.
Bit of a 🤦🏻‍♀️ moment when I saw your comment ;-)

I've just done a review of the use of function local static variables across the whole codebase now.
I've removed a few which were more than anything micro-optimization. For the few remaining ones, I've added the process isolation test annotations and am running a new build now.

Will let you know the result. 🤞🏻

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 11, 2022

Yeah! @derickr was right! Thank you so much for pointing this out to me. You know how you can stare yourself blind on something ? Yes, that. 🤗

I will close this issue, but before I do, let me recap the way I ended up fixing it in case other people run into the same.

Using the annotation(s) turned out to be a no-go.

In most cases/for most people this will probably work fine, but for this particular codebase it did not.

I started by adding @runTestsInSeparateProcesses + @preserveGlobalState disabled at the class level.
Also tried with @runInSeparateProcess + @preserveGlobalState disabled at the test method level in another iteration and in both cases, I ran into an Use of undefined constant STDIN - assumed 'STDIN' error for these tests.

The particular build which failed was on PHP 5.4 with PHPUnit 4.8.36 in combination with PHP_CodeSniffer 2.6.0 (runtime dependency for the codebase with PHPCS 2.6.0 being the lowest supported version).

While I would truly and dearly hope nobody would still use such combinations, I unfortunately know from the PHPCS stats that's not yet the case, which means that this codebase still has to support it and that includes safeguarding that support via the tests.

Using separate test suites works

Not sure where I got this from, but I remembered that I'd run into issues with @runInSeparateProcess before elsewhere (can't even remember where) and that, in that case, I managed to solve it by splitting the test suite up into multiple test suites.

In my original phpunit.xml.dist file, I had the test suites defined like so:

    <testsuites>
        <testsuite name="PHPCSUtils">
            <directory suffix="Test.php">./Tests/</directory>
        </testsuite>
    </testsuites>

I've now changed it to the below, which in effect means the three test classes which test code which declare function local static variables are now run first, before the bulk of the tests:

    <testsuites>
        <testsuite name="Isolation-EmptyTokens">
            <file>./Tests/BackCompat/BCTokens/EmptyTokensTest.php</file>
        </testsuite>
        <testsuite name="Isolation-NamespaceType">
            <file>./Tests/Utils/Namespaces/NamespaceTypeTest.php</file>
        </testsuite>
        <testsuite name="Isolation-GetCompleteNumber">
            <file>./Tests/Utils/Numbers/GetCompleteNumberTest.php</file>
        </testsuite>
        <testsuite name="PHPCSUtils">
            <directory suffix="Test.php">./Tests/</directory>
            <exclude>Tests/BackCompat/BCTokens/EmptyTokensTest.php</exclude>
            <exclude>Tests/Utils/Namespaces/NamespaceTypeTest.php</exclude>
            <exclude>Tests/Utils/Numbers/GetCompleteNumberTest.php</exclude>
        </testsuite>
    </testsuites>

I couldn't find the file or exclude options for testsuite in the documentation, but the phpunit.xsd file confirmed that I can use those elements within a testsuite and the tests run fine with this configuration on all PHP/PHPUnit/PHPCS versions I need to support and code coverage looks to record correctly for the code I previously identified as problematic.

I'll tidy up my WIP branch tomorrow and merge the changes. Very happy to have been able to figure this one out with your help. Thank you!

@jrfnl jrfnl closed this as completed Apr 11, 2022
jrfnl added a commit to PHPCSStandards/PHPCSUtils that referenced this issue Apr 12, 2022
The functions being tested in these three test classes all use function local `static` variables for optimization.

Having the function local `static` variable will often prevent code coverage from being recorded correctly for the code as the code for setting the static will only be run when the function is first called, which may not be in the dedicated test for the function, so it makes code coverage recording highly dependant on the order in which tests are run.

In these three cases, I do believe the function local `static` variable is justified (for the time being, benchmarking should be able to determine for sure at some point in the future).

To still prevent missed lines in the code coverage reports, I'm moving the tests for these three functions into a separate test suite, which will run first.
This should ensure the code which sets the static will be run first when the dedicated tests for these methods are run and should allow the correct registration of code coverage for this code.

Mind: if at some point the `executionOrder` configuration would be added to the config/enabled, it needs to be verified how that behaves in combination with multiple test suites. But that's for later.

Ref: sebastianbergmann/php-code-coverage#913
jrfnl added a commit to PHPCSStandards/PHPCSUtils that referenced this issue Apr 12, 2022
The functions being tested in these three test classes all use function local `static` variables for optimization.

Having the function local `static` variable will often prevent code coverage from being recorded correctly for the code as the code for setting the static will only be run when the function is first called, which may not be in the dedicated test for the function, so it makes code coverage recording highly dependant on the order in which tests are run.

In these three cases, I do believe the function local `static` variable is justified (for the time being, benchmarking should be able to determine for sure at some point in the future).

To still prevent missed lines in the code coverage reports, I'm moving the tests for these three functions into a separate test suite, which will run first.
This should ensure the code which sets the static will be run first when the dedicated tests for these methods are run and should allow the correct registration of code coverage for this code.

Mind: if at some point the `executionOrder` configuration would be added to the config/enabled, it needs to be verified how that behaves in combination with multiple test suites. But that's for later.

Ref: sebastianbergmann/php-code-coverage#913
jrfnl added a commit to PHPCSStandards/PHPCSUtils that referenced this issue Jun 29, 2022
The functions being tested in these three test classes all use function local `static` variables for optimization.

Having the function local `static` variable will often prevent code coverage from being recorded correctly for the code as the code for setting the static will only be run when the function is first called, which may not be in the dedicated test for the function, so it makes code coverage recording highly dependant on the order in which tests are run.

In these three cases, I do believe the function local `static` variable is justified (for the time being, benchmarking should be able to determine for sure at some point in the future).

To still prevent missed lines in the code coverage reports, I'm moving the tests for these three functions into a separate test suite, which will run first.
This should ensure the code which sets the static will be run first when the dedicated tests for these methods are run and should allow the correct registration of code coverage for this code.

Mind: if at some point the `executionOrder` configuration would be added to the config/enabled, it needs to be verified how that behaves in combination with multiple test suites. But that's for later.

Ref: sebastianbergmann/php-code-coverage#913
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants