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

Force empty lines non executable #805

Conversation

dvdoug
Copy link
Contributor

@dvdoug dvdoug commented Aug 28, 2020

To hopefully fix #799

This isn't quite the same concept as an "ignored line" so I've deliberately kept this implementation distinct from the static analysis code. It's done as a single pass at the end of execution for speed, although it also simplifies the code.

@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #805 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #805      +/-   ##
============================================
+ Coverage     82.96%   83.01%   +0.04%     
- Complexity     1111     1118       +7     
============================================
  Files            60       60              
  Lines          3411     3421      +10     
============================================
+ Hits           2830     2840      +10     
  Misses          581      581              
Impacted Files Coverage Δ Complexity Δ
src/CodeCoverage.php 44.91% <100.00%> (+0.23%) 96.00 <0.00> (ø)
src/ProcessedCodeCoverageData.php 100.00% <100.00%> (ø) 58.00 <7.00> (+7.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 961fee7...1000da7. Read the comment docs.

$sourceLines = explode("\n", file_get_contents($filename));

foreach ($coverage as $line => $lineCoverage) {
if (is_array($lineCoverage) && trim($sourceLines[$line - 1]) === '') {
Copy link
Contributor Author

@dvdoug dvdoug Aug 28, 2020

Choose a reason for hiding this comment

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

The is_array isn't technically necessary, without it empty lines that are already non-executable are simply marked non-executable again. However I found it helpful for debugging to ensure that this only triggered on the cases I was expecting it to

@sebastianbergmann sebastianbergmann merged commit a35f278 into sebastianbergmann:master Aug 28, 2020
@sebastianbergmann
Copy link
Owner

I've deliberately kept this implementation distinct from the static analysis code. It's done as a single pass at the end of execution for speed, although it also simplifies the code.

The downside of keeping this separate is that the result of this analysis does not get cached.

@dvdoug dvdoug deleted the force_empty_lines_non_executable branch August 29, 2020 11:24
@lcobucci
Copy link
Contributor

lcobucci commented Sep 2, 2020

@dvdoug @sebastianbergmann it looks like this didn't solve the issue (or perhaps I'm doing something wrong?): lcobucci/jwt#380

@lcobucci
Copy link
Contributor

lcobucci commented Sep 2, 2020

@sebastianbergmann in v9.3.4 this component was relying on the driver to detect "dead code" (9.1.4...master#diff-b2fbc27b0e544bbffcdfba89b3fb1dd4L640), can the removal of that bit be cause of the problem?

@dvdoug
Copy link
Contributor Author

dvdoug commented Sep 2, 2020

The issue of the executable last line was originally raised in the context of reporting being incorrect, hence the implemention decision that:

It's done as a single pass at the end of execution for speed, although it also simplifies the code.

Those CI runs would appear to suggest that strict covers annotations means that it's not OK to do the analysis and adjustment as a one-off after the suite completes but requires it to be performed after each individual test invocation instead? That's annoying and will definitely want some caching 😢

I am happy to look into this more, but please note I am not likely to have time before next week

@sebastianbergmann
Copy link
Owner

I am happy to look into this more, but please note I am not likely to have time before next week

No worries, take your time. I will look into reverting the recent changes and roll a new release later today.

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.

Uncovered new line at end of file
3 participants