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

Include driver code in code coverage to enable proper self-test #745

Merged

Conversation

dvdoug
Copy link
Contributor

@dvdoug dvdoug commented May 12, 2020

Hello @sebastianbergmann

Please see attached a PR to remove @codeCoverageIgnore declarations from the driver classes.

Rationale for the change is this:

  • In the Xdebug driver, there is code to cleanup bad output from Xdebug (https://github.com/sebastianbergmann/php-code-coverage/blob/master/src/Driver/Xdebug.php#L76)
  • When Xdebug is configured to return branch coverage data (Support for Path Coverage #380), the data that is returned is in a different format from that used by line coverage (see https://xdebug.org/docs/code_coverage)
  • This code would therefore need to be adjusted as part of work on Support for Path Coverage #380
  • Except that Xdebug returning such bogus data seems like a really weird thing for it to do, and upon investigation I cannot make it do that in my tests
  • git blame reveals the code has been in the code base since v2 (see 577fdf3 and 121d8c7), which combined with the fact I cannot reproduce the behaviour strongly suggests to me that it was to workaround an issue in an ancient version of Xdebug that has long since been fixed
  • as a final confirmation to prove the code is now obsolete before submitting a PR to remove it, I wanted to check the code coverage data from php-code-coverage's own test suite - if the code was not executed but the test suite was still passing then that would provide additional reassurance that it could be safely removed
  • except that the drivers are excluded from code coverage so it is not possible to use the coverage report to do this

The drivers have been excluded for a very long time, the commit that originally introduced the exclusion for the whole Xdebug driver didn't include any rationale, it simply says "Simplify" (see 9db2cc4). Since then, drivers for phpdbg and pcov have been added but I can't find any reason for the exclusions there other than cargo-culting/copy-pasting.

I can't think of a reason why they should not be included, are you able to remember that far back and why you might have initially excluded them? The phpdbg driver in particular does a lot of post-processing on the data before handing it off and it would be good to have that show up in reports as something that doesn't have tests.

@codecov
Copy link

codecov bot commented May 12, 2020

Codecov Report

Merging #745 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #745   +/-   ##
=========================================
  Coverage     81.96%   81.96%           
- Complexity      842      843    +1     
=========================================
  Files            36       36           
  Lines          2484     2484           
=========================================
  Hits           2036     2036           
  Misses          448      448           
Impacted Files Coverage Δ Complexity Δ
src/Driver/PCOV.php 30.00% <ø> (ø) 3.00 <0.00> (ø)
src/Driver/PHPDBG.php 0.00% <ø> (ø) 13.00 <0.00> (ø)
src/Driver/Xdebug.php 0.00% <ø> (ø) 15.00 <0.00> (ø)
src/Report/Text.php 75.20% <0.00%> (ø) 23.00% <0.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 d905bc4...ce0d84b. Read the comment docs.

@sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann merged commit 1894727 into sebastianbergmann:master May 13, 2020
@sebastianbergmann
Copy link
Owner

I do not remember the rationale for ignoring the drivers from code coverage.

@dvdoug dvdoug deleted the include_drivers_in_coverage branch May 13, 2020 08:36
@derickr
Copy link
Contributor

derickr commented May 13, 2020

I could never really reproduce Xdebug showing the wrong line numbers, except for a few cases where PHP would add (internally) an extra line at the end of a script. But that's a long time ago, so I think this can be removed.

@sebastianbergmann
Copy link
Owner

Thank you, @derickr.

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