Fix relevant lines for unloaded files #605
Conversation
Lines with whitespace, comments, or blocks with :nocov: should not be relevant (nil). Other lines from unloaded files should be relevant with zero cover (0). Fixes #604
There was a problem hiding this comment.
Thank you for your excellent PR! 👍 💪 🎉 🚀
Comments are mostly about some more edge casey testing that I'd like to have, if you're lacking the time (as I lacked the team for a review earlier, sorry) I'm happy to take this over and get it to master :)
Ghost and Nala say thank you:
|
|
||
| When I open the coverage report generated with `bundle exec rspec spec` | ||
| Then I follow "lib/not_loaded.rb" | ||
| Then I should see "3 relevant lines" within ".highlighted" |
There was a problem hiding this comment.
Thank you for adding an integration test! 👍
lib/simplecov/lines_classifier.rb
Outdated
| @@ -0,0 +1,29 @@ | |||
| module SimpleCov | |||
| class LinesClassifier | |||
There was a problem hiding this comment.
First I was like damn why do we need this, that is something Coverage should expose, but it doesn't and it's implemented in C.
A comment on the class might help though :)
| lines.map do |line| | ||
| if line =~ self.class.no_cov_line | ||
| skipping = !skipping | ||
| NOT_RELEVANT |
lib/simplecov/lines_classifier.rb
Outdated
| if line =~ self.class.no_cov_line | ||
| skipping = !skipping | ||
| NOT_RELEVANT | ||
| elsif line =~ WHITESPACE_OR_COMMENT_LINE || skipping |
There was a problem hiding this comment.
very minor thing, but I think skipping should be evaluated first (regexes are potentially slower) - I know probably premature optimization as I hope people don't track to many not loaded files, but better safe than sorry :)
spec/lines_classifier_spec.rb
Outdated
| ] | ||
|
|
||
| expect(classified_lines.length).to eq 2 | ||
| expect(classified_lines).to all be_relevant |
There was a problem hiding this comment.
What is this magic I've never seen before 😳 Cool stuff, thanks for teaching me!
spec/lines_classifier_spec.rb
Outdated
| "# :nocov:", | ||
| "def hi", | ||
| "end", | ||
| "# :nocov:", |
There was a problem hiding this comment.
I'd like to see some tests/behaviour with non closing #:nocov:
something like:
# :nocov:
foo
# :nocov:
bar
# :nocov:
lol:)
spec/lines_classifier_spec.rb
Outdated
| it "classifies whitespace as not-relevant" do | ||
| classified_lines = subject.classify [ | ||
| "", | ||
| " ", |
There was a problem hiding this comment.
tab(s) inside the string? :)
spec/lines_classifier_spec.rb
Outdated
| it "classifies comments as not-relevant" do | ||
| classified_lines = subject.classify [ | ||
| "#Comment", | ||
| " # Leading space comment", |
There was a problem hiding this comment.
I think a test that goes and checks that puts "#{var}" is not considered a comment might be worth it :)
spec/lines_classifier_spec.rb
Outdated
| it "classifies each line as relevant" do | ||
| classified_lines = subject.classify [ | ||
| "def foo", | ||
| "end", |
There was a problem hiding this comment.
I might be over testing stuff, but a test with a class and a method definiton inside might be cool to make sure that all that works correctly :)
|
Thanks for the review @PragTob - excellent points. I think I'll have time this weekend / Monday to make further edits addressing your feedback 😃 |
|
That'd be great! I missed out on doing a release the last couple of weeks, but now that it's here I'd wait for this to land as it's a good bug fix :) |
Adds examples for hard tabs, string interpolation and non-closing :nocov: tokens. Also adds a descriptive comment to SimpleCov::LinesClassifier.
|
@PragTob I've updated the PR based on your feedback, thanks. |
spec/lines_classifier_spec.rb
Outdated
| expect(classified_lines).to all be_not_relevant | ||
| expect(classified_lines[0..2]).to all be_irrelevant | ||
| expect(classified_lines[3..4]).to all be_relevant | ||
| expect(classified_lines[5..7]).to all be_irrelevant |
* [ci skip] * reordering in bug fixes shows my subjective perception of how many people this might affect/relevance :)
0.15.0 (2017-08-14) ([changes](simplecov-ruby/simplecov@v0.14.1...v0.15.0)) ======= ## Enhancements * Ability to use regex filters for removing files from the output. See [#589](simplecov-ruby/simplecov#589) (thanks @jsteel) ## Bugfixes * Fix merging race condition when running tests in parallel and merging them. See [#570](simplecov-ruby/simplecov#570) (thanks @jenseng) * Fix relevant lines for unloaded files - comments, skipped code etc. are correctly classigied as irrelevant. See [#605](simplecov-ruby/simplecov#605) (thanks @odlp) * Allow using simplecov with frozen-string-literals enabled. See [#590](simplecov-ruby/simplecov#590) (thanks @pat) * Make sure Array Filter can use all other filter types. See [#589](simplecov-ruby/simplecov#589) (thanks @jsteel) * Make sure file names use `Simplecov.root` as base avoiding using full absolute project paths. See [#589](simplecov-ruby/simplecov#589) (thanks @jsteel)
As discussed in #604, unloaded files appear to have whitespace, comments & nocov block lines counted as relevant. This leads to the overall coverage being understated.
Before:
After: