Rework how code coverage settings are propagated to the driver

[dvdoug]

Hello @sebastianbergmann

As collecting branch and path data makes Xdebug very slow, there will need to be a way to enable/disable this. Additionally, as a new feature, it also makes sense to turn it off by default out of caution.

Currently, the signature of the call to start collecting code coverage looks like this:

    /**
     * Start collection of code coverage information.
     */
    public function start(bool $determineUnusedAndDead = true): void;

I would prefer not to add a second parameter to that call for several reasons:

1. The start() call acts as a wrapper around the driver's specific implementation, but not all drivers configure their settings when collection is started. Some (e.g. phpdbg) have their options configured when retrieving the data instead. This interface assumes an Xdebug-like approach.
2. Both the existing $determineUnusedAndDead and the new branch/path setting are for the Xdebug driver only. The other drivers simply ignore them. The current interface makes no acknowledgement that this is permitted behaviour.
3. It should be possible to easily configure each setting independently. Where multiple arguments exist, this is not possible without making each setting nullable. Users should be able to configure only the setting they want, whilst allowing others to be left in their default state.
4. ->start($determineUnusedAndDead = false, $collectPathCoverage = true) would not actually be a permissible setting as Xdebug requires all 3 of \XDEBUG_CC_UNUSED | \XDEBUG_CC_DEAD_CODE | \XDEBUG_CC_BRANCH_CHECK for it to be enabled.
5. It would be nice to be able to add any additional settings in the future without needing a BC break on the interface.

I would therefore like to suggest that the configuration be changed to accept "hints" instead, which are explicitly documented to operate on a best-effort basis. The attached PR implements this approach.

What do you think?

[codecov]

Codecov Report

Merging #752 into master will decrease coverage by 0.45%.
The diff coverage is 41.86%.

@@             Coverage Diff              @@
##             master     #752      +/-   ##
============================================
- Coverage     84.03%   83.57%   -0.46%     
- Complexity      846      866      +20     
============================================
  Files            37       38       +1     
  Lines          2480     2515      +35     
============================================
+ Hits           2084     2102      +18     
- Misses          396      413      +17     

Impacted Files	Coverage Δ	Complexity Δ
src/Driver/PHPDBG.php	0.00% <0.00%> (ø)	15.00 <3.00> (+2.00)
src/Driver/Xdebug.php	0.00% <0.00%> (ø)	11.00 <6.00> (+5.00)
src/CodeCoverage.php	68.82% <14.28%> (-1.03%)	160.00 <1.00> (+3.00)	⬇️
src/Driver/Driver.php	100.00% <100.00%> (ø)	8.00 <8.00> (?)
src/Driver/PCOV.php	69.23% <100.00%> (+35.89%)	5.00 <3.00> (+2.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 f0d58f7...ed411cb. Read the comment docs.

[sebastianbergmann]

I agree that the setting(s) for what (level of detail of) code coverage data is collected should not be part of the start() method's signature.

I think it makes sense to not configure what (level of detail of) code coverage data is collected through constructor arguments but instead throug individual methods, one for each aspect that influences data collection. Exactly what you propose.

I do not agree, however, with the "hints" system you propose. As far as I understand it, and please correct me if I'm wrong, then the "hints" are used for two things: asking a driver what level of detail it supports and telling it at which level of detail to collect data. This feels wrong.

I would prefer something like this:

<?php declare(strict_types=1);
abstract class Driver
{
    private $detectDeadCode = true;

    private $collectPathCoverage = false;

    public function canDetectDeadCode(): bool
    {
        return false;
    }

    public function detectDeadCode(bool $flag): void
    {
        if ($flag && !$this->canDetectDeadCode()) {
            throw new DeadCodeDetectionNotSupportedException;
        }

        $this->detectDeadCode = $flag;
    }

    public function detectsDeadCode(): bool
    {
        return $this->detectDeadCode;
    }

    public function canCollectPathCoverage(): bool
    {
        return false;
    }

    public function collectPathCoverage(bool $flag): void
    {
        if ($flag && !$this->canCollectPathCoverage()) {
            throw new PathCoverageNotSupportedException;
        }

        $this->collectPathCoverage = $flag;
    }

    public function collectsPathCoverage(): bool
    {
        return $this->collectPathCoverage;
    }

    abstract public function start(): void;

    abstract public function stop(): RawCodeCoverageData;
}

<?php declare(strict_types=1);
final class Xdebug extends Driver
{

    public function canDetectDeadCode(): bool
    {
        return true;
    }

    public function canCollectPathCoverage(): bool
    {
        return true;
    }

    public function start(): void
    {
        $flags = \XDEBUG_CC_UNUSED;

        if ($this->detectsDeadCode()) {
            $flags |= \XDEBUG_CC_DEAD_CODE;
        }

        if ($this->collectsPathCoverage()) {
            $flags |= \XDEBUG_CC_BRANCH_CHECK;
        }
        
        \xdebug_start_code_coverage($flags);
    }

    public function stop(): RawCodeCoverageData
    {
        // ...
    }
}

[dvdoug]

I do not agree, however, with the "hints" system you propose. As far as I understand it, and please correct me if I'm wrong, then the "hints" are used for two things: asking a driver what level of detail it supports and telling it at which level of detail to collect data. This feels wrong.

The getHint() method exists for retrieving the current state of any hints set, which I added really just for the purpose of being able to write at least some tests around the setHint() calls. Interrogating the driver for it's capabilities was certainly not the use case I had in mind (for branch/path collection I intend to determine whether to include branch/path data in reports based purely on whether the driver returned data, not on any hint setting)

[sebastianbergmann]

My main problem with getHint() is that the name is generic (implicit semantic) whereas something like detectsDeadCode() is specific (explicit semantic).

[dvdoug]

OK, I'll have a go implementing something along the lines you've suggested - swapping out the interface for an abstract class

[dvdoug]

It's not a situation that exists today, but in the spirit of trying to be future proof there is a conceptual difference between "supports doing something" and "supports turning it on and off" so this isn't feeling quite right to me.

I'm also thinking of situations where e.g. path coverage is configured for collection, but then someone runs the test suite against pcov instead of Xdebug. I'm not sure that should throw an exception, it's feels like something to warn about rather than halt execution for.

How about public function collectPathCoverage(bool $flag): bool? It could return true if supported and false if not?

[dvdoug]

i.e. configuring something the driver doesn't support wouldn't be a noisy failure, but the caller could check the return value if it wanted to know whether the call had succeeded? In PHPUnit that could be used emit a warning when trying to enable path coverage, but in php-code-coverage the call to turn off dead code detection wouldn't bother checking the return value as that particular setting is just being (ab)used as an optimisation?

[sebastianbergmann]

How about public function collectPathCoverage(bool $flag): bool? It could return true if supported and false if not?

That would violate command/query separation.

If we really need to check whether path coverage collection can be controlled (in addition to checking whether path coverage collection is possible) then we need

• canCollectPathCoverage(): bool
• pathCoverageCollectionCanBeControlled(): bool
• collectsPathCoverage(): bool
• collectPathCoverage(bool $flag): void

When collectPathCoverage(true) is called and canCollectPathCoverage() returns false or pathCoverageCollectionCanBeControlled() returns false then we raise an exception.

I'm not sure that should throw an exception, it's feels like something to warn about rather than halt execution for.

Within the context of this library that must be an exception. How a consumer of this library, PHPUnit for instance, handles that exception is up to them. PHPUnit would most certainly print a warning and then run the tests without collecting and processing code coverage.

[dvdoug]

That would violate command/query separation.

That's a good principle to have, but I don't think it necessarily applies in this particular case, since the question would be "did this command succeed". Checking the return value of a call that may or may not work is a very common pattern in PHP.

But it's your codebase 😄

[dvdoug]

This optimisation method relies on dead code detection being enabled so I've made enabling this explicit rather than just relying on the default value of the driver

[dvdoug]

Note that this now necessarily defaults to false rather than true because otherwise detectingDeadCode() would return true for PHPDBG and PCOV even though canDetectDeadCode would return false

[sebastianbergmann]

I manually merged this and made some further refactorings / cleanups.

[dvdoug]

👍