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

Store captured branch/path function data #754

Conversation

dvdoug
Copy link
Contributor

@dvdoug dvdoug commented May 23, 2020

Hi @sebastianbergmann

This is the next piece for #380, just storing the data from Xdebug inside RawCodeCoverageData. Unfortunately I've had to add back a version of the format "guessing" that you asked to be removed before, as it turns out from my testing that where a covered file doesn't have any functions (e.g. a bootstrap or configuration file), then Xdebug outputs the line data in the flat format, rather than embedded inside a lines key. Sniffing on a per-file basis is therefore required.

@codecov
Copy link

codecov bot commented May 23, 2020

Codecov Report

Merging #754 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #754      +/-   ##
============================================
+ Coverage     83.94%   83.97%   +0.02%     
- Complexity      854      855       +1     
============================================
  Files            39       39              
  Lines          2529     2533       +4     
============================================
+ Hits           2123     2127       +4     
  Misses          406      406              
Impacted Files Coverage Δ Complexity Δ
src/RawCodeCoverageData.php 100.00% <100.00%> (ø) 10.00 <1.00> (+1.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 9952b35...8b0341b. Read the comment docs.

@sebastianbergmann
Copy link
Owner

@derickr Is that intentional? Would be great if Xdebug behaved consistently.

}

public function clear(): void
{
$this->lineCoverage = [];
$this->lineCoverage = $this->functionCoverage = [];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like this and will change it two separate statements on separate lines after merging.

@sebastianbergmann sebastianbergmann merged commit 1b4794c into sebastianbergmann:master May 24, 2020
@derickr
Copy link
Contributor

derickr commented May 24, 2020

No, this sounds like a bug. Please file one at https://bugs.xdebug.org

@sebastianbergmann
Copy link
Owner

@dvdoug Can you open a ticket for Xdebug, please? Thanks!

@dvdoug
Copy link
Contributor Author

dvdoug commented May 24, 2020

Will do

@dvdoug dvdoug deleted the store_captured_branch_path_data branch May 24, 2020 17:05
@sebastianbergmann
Copy link
Owner

Now that this bug in Xdebug was fixed, I think we should add a version check to the Xdebug driver that allows path coverage only with Xdebug >= 2.9.6. And then we can remove the "guessing" from our code.

@dvdoug
Copy link
Contributor Author

dvdoug commented May 28, 2020

My concern with that would be that there will be many users who install xdebug from apt rather than from pecl and thus won't see the fix for many years.

How would you feel about something like this, which limits the workaround?

        if ($this->collectsBranchAndPathCoverage() && $isFixedXDebugVersion) {
            return RawCodeCoverageData::fromXdebugWithPathCoverage($data);
        }

        if ($this->collectsBranchAndPathCoverage() && !$isFixedXDebugVersion) {
            return RawCodeCoverageData::fromXdebugWithMixedFormatCoverage($data);
        }

   /**
     * @deprecated to be removed once PHP7.4 is EOL
     */
    public static function fromXdebugWithMixedFormatCoverage(array $rawCoverage): self
    {
    ...
    }

@sebastianbergmann
Copy link
Owner

My concern with that would be that there will be many users who install xdebug from apt rather than from pecl and thus won't see the fix for many years.

Installing current versions of PHP, Xdebug, etc. from trusted sources on LTS OS distributions is possible (from Ondřej Surý for Debian and and Ubuntu, from Remi Collet for Fedora/RHEL/CentOS).

But, IMO, life is too short for old software. If you decide to use an OS distribution that does not update PHP, Xdebug, etc. in a timely manner then you are on your own. This should not be a burden for us. meaning we should not have to maintain different execution branches, one for an old, buggy version of Xdebug, and one for the current version.

That being said, I think I can live with your workaround. However, I would not use "to be removed once PHP7.4 is EOL". We should get rid of this workaround when Xdebug 2 reaches its end-of-life because this has nothing to do with PHP 7.4.

@derickr How long do you plan to support Xdebug 2 once Xdebug 3.0.0 is released?

@derickr
Copy link
Contributor

derickr commented May 29, 2020

I probably won't continue to support it at all, just like I don't support Xdebug 2.8 either any more. I might do crash fixes only for 2.9 for a while after 3.0 is released though.

@dvdoug
Copy link
Contributor Author

dvdoug commented May 29, 2020

Installing current versions of PHP, Xdebug, etc. from trusted sources on LTS OS distributions is possible (from Ondřej Surý for Debian and and Ubuntu, from Remi Collet for Fedora/RHEL/CentOS).

But, IMO, life is too short for old software. If you decide to use an OS distribution that does not update PHP, Xdebug, etc. in a timely manner then you are on your own. This should not be a burden for us. meaning we should not have to maintain different execution branches, one for an old, buggy version of Xdebug, and one for the current version.

I would personally place a slightly different emphasis on the factors involved on this one - this is not about adding new code to additionally support an older version of Xdebug or backporting to an older branch of php-code-coverage to allow PHP7.2 users to benefit. In those cases the question is about whether you want to extend the range of potential users who can use a feature.

Here, the code that's already in the codebase supports all versions of Xdebug 2.7+. Therefore a decision to require literally the very latest point release would be a decision to significantly reduce the number of potential users who can get the benefit of the feature. That has different tradeoffs IMHO 😄

That being said, I think I can live with your workaround. However, I would not use "to be removed once PHP7.4 is EOL". We should get rid of this workaround when Xdebug 2 reaches its end-of-life because this has nothing to do with PHP 7.4.

I see @derickr has released a new Xdebug, I'll run a version of php-code-coverage without the workaround against a few different codebases to confirm everything's OK and will then submit a PR.

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

Successfully merging this pull request may close these issues.

3 participants