Skip to content

Line coverage stubs: be consistent with actual xDebug/PCOV output #891

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

Merged
merged 1 commit into from
Feb 8, 2022
Merged

Line coverage stubs: be consistent with actual xDebug/PCOV output #891

merged 1 commit into from
Feb 8, 2022

Conversation

Slamdunk
Copy link
Contributor

@Slamdunk Slamdunk commented Feb 8, 2022

Found this inconsistency while investigating #889

I manually checked both Xdebug3Driver and PcovDriver outputs and they differ a bit from what's coded in TestCase.

Most of the differences are negligible and have no impact on the purpose of the involved tests, they all still pass, but the diff addressed in this PR can sneak bugs in the future if not merged, especially while merging multiple CC reports.

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #891 (d37f1c4) into 9.2 (caeda52) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##                9.2     #891   +/-   ##
=========================================
  Coverage     83.19%   83.19%           
  Complexity     1112     1112           
=========================================
  Files            62       62           
  Lines          3648     3648           
=========================================
  Hits           3035     3035           
  Misses          613      613           

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 caeda52...d37f1c4. Read the comment docs.

Comment on lines +55 to +56
14 => -1,
15 => -1,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reference:

protected function setBalance($balance)
{
if ($balance >= 0) {
$this->balance = $balance;
} else {
throw new RuntimeException;
}
}

Line 14

Line 14 is always reported by each driver. Not having it here can introduce a bug on CC merging in the future

Line 15

This line is not reported by any driver, but I kept it here because removing it would fail pretty much every test in this library.
I've checked each of them, and removing this reported line to be consistent with drivers output adds no value to the test suite.

@Slamdunk
Copy link
Contributor Author

Slamdunk commented Feb 8, 2022

To be more clear on how I got this PR committed:

  1. I've run each of the 4 BankAccountTest tests
  2. Gathered xdebug_get_code_coverage() output
  3. Copy-pasted it into \SebastianBergmann\CodeCoverage\TestCase::getLineCoverageXdebugDataForBankAccount (and partially into getPathCoverageXdebugDataForBankAccount)
  4. Manually re-added line 15 => -1, to the arrays to get the test pass again

@sebastianbergmann sebastianbergmann merged commit be9a612 into sebastianbergmann:9.2 Feb 8, 2022
@Slamdunk Slamdunk deleted the cc_stub_consistency branch February 8, 2022 13:42
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.

2 participants