PHP-Parser interferes with testing code that uses (back|for)ward compatibility layers for ext/tokenizer

[jrfnl]

Q	A
php-code-coverage version	whichever version comes with PHPUnit releases
PHP version	PHP 7.3 - 7.4 (confirmed, possibly more)
Driver	Xdebug
Xdebug version (if used)	2.9.6
Installation Method	BOTH: Composer / PHPUnit PHAR
Usage Method	PHPUnit
PHPUnit version (if used)	9.3.x

When running PHPUnit with code coverage enabled the tests for PHPCSUtils and PHPCompatibility have started to semi-randomly break due to PHPUnit's use of PHP-Parser.

PHP-Parser breaks our tests as it interferes with globally defined PHP native constants by backfilling them.

The first few times a defined() for a PHP native token constant is called, the response is correct and as expected, but the third time that same defined() is called, the response will be true, even if the token constant is not defined in our stack and all subsequent tests relying on the defined() logic working will subsequently fail.

Related code in PHP-Parser:

vendor/nikic/php-parser/lib/PhpParser/Lexer.php:        if (!defined('T_NULLSAFE_OBJECT_OPERATOR')) {
vendor/nikic/php-parser/lib/PhpParser/Lexer.php:            \define('T_NULLSAFE_OBJECT_OPERATOR', -8);

Could it be that PHP Parser is loaded too early to prevent interference ?

For an extensive description of the problem, please see: https://gist.github.com/jrfnl/c6fad49bb7580b7b4371af3c68b122f7

Note: while this isn't a problem yet for PHP_CodeSniffer itself at this time as they don't support PHPUnit 9 yet, it will be in the near future. PHPCS itself should also be able to rely on whether or not tokens are defined in PHP to determine how to adjust a tokenized file (if necessary). So, the PHP native tokens being defined by PHP Parser will break coverage builds for PHPCS as well.

With graceful thanks to @derickr for helping me debug this as I initially though it was related to the use of the latest Xdebug version.

[sebastianbergmann]

If you use PHPUnit from a PHAR then no autoloading is used and all sourceode files of PHPUnit as well as its dependencies are loading during startup. This is to prevent problems that otherwise may occur when PHPUnit is used from a PHAR in a project that also install PHPUnit via Composer. If you install PHPUnit using Composer then regular, Composer-based autoloading is used.

In theory, we could be loading PHP-Parser too early when the PHAR is used. However, since you also experience the problem when PHPUnit is installed using Composer, I do not believe this to be the case. Furthermore, the code in PHP-Parser that invokes \define('T_NULLSAFE_OBJECT_OPERATOR', -8); is not outside a function or class, meaning it is not automatically run when the sourcecode file that contains it is loaded. So we should be able to rule out "loaded too early" as a cause for this issue.

Maybe @nikic has an idea what might be going on here.

[sebastianbergmann]

The first few times a defined() for a PHP native token constant is called, the response is correct and as expected, but the third time that same defined() is called, the response will be true

To clarify: by "[t]he first few times a defined() for a PHP native token constant is called" you mean calls to defined() from your code and by "the third time that same defined() is called" you also mean calls to defined() from your code but after PhpParser\Lexer::defineCompatibilityTokens() has been called. Right? If that is so, then there is no surprise that after PhpParser\Lexer::defineCompatibilityTokens() has been called that a token such as T_NULLSAFE_OBJECT_OPERATOR would exist when it did not exist before. That is the purpose of the PhpParser\Lexer::defineCompatibilityTokens() method.

However, I do not understand why PhpParser\Lexer::defineCompatibilityTokens() is even called by PHP-Parser when emulative lexing is not used.

I will leave this issue open although I do not believe there to be a problem in php-code-coverage. This looks like an issue of competing backward/forward compatibility layers, one in PHP-Parser and one in the code you're testing, Juliette, for dealing with token constants not defined in the version of PHP used.

[jrfnl]

@sebastianbergmann Thank you for taking a look.

To clarify: by "[t]he first few times a defined() for a PHP native token constant is called" you mean calls to defined() from your code and by "the third time that same defined() is called" you also mean calls to defined() from your code but after PhpParser\Lexer::defineCompatibilityTokens() has been called. Right?

Correct, though I have no idea when or why PhpParser\Lexer::defineCompatibilityTokens() is called, other than that I presume that is the call interfering.

This looks like an issue of competing backward/forward compatibility layers, one in PHP-Parser and one in the code you're testing, Juliette, for dealing with token constants not defined in the version of PHP used.

That's the whole point. How can I still test the (code coverage of the) code relating to the correct functioning of those compatibility layers in my code if the code of the test framework I use interferes ?

I can understand that for visualizing the code coverage, the tokenization of a file is used to determine which lines/tokens are "covered", but I would expect that to be done after my tests have finished running, not while my tests are running, which is what is causing the interference and why I used the loaded too early in the title of the issue.

[sebastianbergmann]

That's the whole point. How can I still test the (code coverage of the) code relating to the correct functioning of those compatibility layers in my code if the code of the test framework I use interferes ?

To be honest: I do not know. I understand that this is frustrating, but your use case might be an edge case that we are not able to accomodate.

I think that PHP-Parser is wrong about declaring those constants in PhpParser\Lexer::defineCompatibilityTokens() even if non-emulative lexing is used. But I do not know enough about PHP-Parser's internals to say this with certainty. This is something that probably only @nikic can shine a light on.

I can understand that for visualizing the code coverage, the tokenization of a file is used to determine which lines/tokens are "covered", but I would expect that to be done after my tests have finished running, not while my tests are running

A sourcecode file is processed after it has been "seen" by php-code-coverage for the first time. This processing included parsing it with PHP-Parser. We need to do this to find @codeCoverageIgnore annotations, for instance. This and similar information is needed as early as possible to discard code coverage data that we are not interested in as early as possible to reduce the amount of data that needs to be kept in memory as well as the amount of data that needs to be updated after each test.

[jrfnl]

For the time being, I've restricted our builds to use PHPUnit 9.2.x for code coverage, that will at least allow us to have passing builds again, so PRs can get merged.

I'll keep my fingers crossed that @nikic will have additional insights once he's back from his vacation.

@nikic If you do read your mail while on holiday: please enjoy your time off, this can wait until you're back.

[sebastianbergmann]

@nikic If you do read your mail while on holiday: please enjoy your time off, this can wait until you're back.

Hear, hear.

[sebastianbergmann]

@jrfnl Here is an idea you could try: configure the cache for static analysis results (either using the --coverage-cache <directory> CLI option for the PHPUnit test runner or using <coverage cacheDirectory="directory"> in PHPUnit's XML configuration file) and warm the cache using the --warm-coverage-cache CLI option before you run your tests. This should prevent php-code-coverage from running any PHP-Parser code while you're running the tests.

[jrfnl]

@sebastianbergmann I'd love to give that a try. Just been trying to find out more information about the options you mention in the PHPUnit documentation, but I can't seem to find anything. Are these options new for a future version ? Or have they just not been added to the documentation yet ?

[sebastianbergmann]

Are these options new for a future version ? Or have they just not been added to the documentation yet?

I did not get a chance yet to document these options. They were introduced in PHPUnit 9.3.4: https://github.com/sebastianbergmann/phpunit/blob/master/ChangeLog-9.3.md#934---2020-08-10

[sebastianbergmann]

I did not get a chance yet to document these options.

I pushed (an initial version of) documentation for these options to the https://github.com/sebastianbergmann/phpunit-documentation-english. However, ReadTheDocs does not build the documentation right now for some reason.

[jrfnl]

@sebastianbergmann Thanks for doing that! ❤️ I need to finish some other things first, but I'm hoping to be able to sit down with this again towards the end of the week.

[sebastianbergmann]

@jrfnl Did you get a chance to try the proposed solution/workaround?

[jrfnl]

@sebastianbergmann Sorry, I haven't gotten round to it yet. For now, I'm using PHPUnit 9.2 with PHP 7.4 for the code coverage builds, but once PHP 8 comes out I will need to have it working with PHPUnit 9.3+, so it is definitely on my (overly full) to-do list to experiment with the caching options you suggested, just not top-of-the-list priority at this very moment.

[sebastianbergmann]

No feedback, closing.

[jrfnl]

@sebastianbergmann Sorry about this. It's still on my todo list, but hasn't reached the top yet. I'm going to try and reserve time to look into this more deeply in a week or two.

[jrfnl]

Okay, so it took a little longer still before I had time to properly work on this and run tests with various scenarios, but I finally got round to it.

I can confirm that using the --warm-coverage-cache option together with the --coverage-cache <directory> option, as suggested above by @sebastianbergmann, works.

In case anyone else comes across this issue and is looking for a code sample on how to implement this in a repo which needs to support multiple versions of PHPUnit, here is a code sample for a working GH Actions workflow:

coverage:
    runs-on: ubuntu-latest
    strategy:
      matrix:
        php: ['5.6', '7.2', '8.1']

    steps:
      - name: Checkout code
        uses: actions/checkout@v3

      - name: Install PHP
        uses: shivammathur/setup-php@v2
        with:
          php-version: ${{ matrix.php }}
          coverage: xdebug

      - name: Install Composer dependencies
        uses: "ramsey/composer-install@v2"

      - name: Grab PHPUnit version
        id: phpunit_version
        run: echo ::set-output name=VERSION::$(vendor/bin/phpunit --version | grep --only-matching --max-count=1 --extended-regexp '\b[0-9]+\.[0-9]+')

      - name: "Warm the PHPUnit cache (PHPUnit 9.3+)"
        if: ${{ steps.phpunit_version.outputs.VERSION >= '9.3' }}
        run: vendor/bin/phpunit --coverage-cache ./build/phpunit-cache --warm-coverage-cache

      - name: "Run the unit tests with code coverage (PHPUnit < 9.3)"
        if: ${{ steps.phpunit_version.outputs.VERSION < '9.3' }}
        run: vendor/bin/phpunit

      - name: "Run the unit tests with code coverage (PHPUnit 9.3+)"
        if: ${{ steps.phpunit_version.outputs.VERSION >= '9.3' }}
        run: vendor/bin/phpunit --coverage-cache ./build/phpunit-cache

Notes:

• The --coverage-cache and --warm-coverage-cache options are available since PHPUnit 9.3.4 and if these are used on older PHPUnit versions, PHPUnit will error out with an "unrecognized CLI argument" error.
• In other words: these options can only be used with PHPUnit 9.3+, which is why the PHPUnit version is checked and remembered and subsequently used in the conditions.
• Also: running PHPUnit with the --warm-coverage-cache option does not run the tests. It literally only warms the cache, which is why this is implemented as a separate step in the workflow.

[sebastianbergmann]

Thank you, @jrfnl.