-
Notifications
You must be signed in to change notification settings - Fork 553
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
Fix relevant lines for unloaded files #605
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
module SimpleCov | ||
# Classifies whether lines are relevant for code coverage analysis. | ||
# Comments & whitespace lines, and :nocov: token blocks, are considered not relevant. | ||
|
||
class LinesClassifier | ||
RELEVANT = 0 | ||
NOT_RELEVANT = nil | ||
|
@@ -18,7 +21,7 @@ def classify(lines) | |
if line =~ self.class.no_cov_line | ||
skipping = !skipping | ||
NOT_RELEVANT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 using constants! |
||
elsif line =~ WHITESPACE_OR_COMMENT_LINE || skipping | ||
elsif skipping || line =~ WHITESPACE_OR_COMMENT_LINE | ||
NOT_RELEVANT | ||
else | ||
RELEVANT | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,49 +3,88 @@ | |
|
||
describe SimpleCov::LinesClassifier do | ||
describe "#classify" do | ||
describe "only relevant lines" do | ||
it "classifies each line as relevant" do | ||
describe "relevant lines" do | ||
it "determines code as relevant" do | ||
classified_lines = subject.classify [ | ||
"def foo", | ||
"module Foo", | ||
" class Baz", | ||
" def Bar", | ||
" puts 'hi'", | ||
" end", | ||
" end", | ||
"end", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :) |
||
] | ||
|
||
expect(classified_lines.length).to eq 2 | ||
expect(classified_lines.length).to eq 7 | ||
expect(classified_lines).to all be_relevant | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this magic I've never seen before 😳 Cool stuff, thanks for teaching me! |
||
end | ||
end | ||
|
||
describe "not-relevant lines" do | ||
it "classifies whitespace as not-relevant" do | ||
it "determines whitespace is not-relevant" do | ||
classified_lines = subject.classify [ | ||
"", | ||
" ", | ||
" ", | ||
"\t\t", | ||
] | ||
|
||
expect(classified_lines.length).to eq 2 | ||
expect(classified_lines).to all be_not_relevant | ||
expect(classified_lines.length).to eq 3 | ||
expect(classified_lines).to all be_irrelevant | ||
end | ||
|
||
it "classifies comments as not-relevant" do | ||
classified_lines = subject.classify [ | ||
"#Comment", | ||
" # Leading space comment", | ||
] | ||
describe "comments" do | ||
it "determines comments are not-relevant" do | ||
classified_lines = subject.classify [ | ||
"#Comment", | ||
" # Leading space comment", | ||
"\t# Leading tab comment", | ||
] | ||
|
||
expect(classified_lines.length).to eq 3 | ||
expect(classified_lines).to all be_irrelevant | ||
end | ||
|
||
expect(classified_lines.length).to eq 2 | ||
expect(classified_lines).to all be_not_relevant | ||
it "doesn't mistake interpolation as a comment" do | ||
classified_lines = subject.classify [ | ||
'puts "#{var}"', | ||
] | ||
|
||
expect(classified_lines.length).to eq 1 | ||
expect(classified_lines).to all be_relevant | ||
end | ||
end | ||
|
||
it "classifies :nocov: blocks as not-relevant" do | ||
classified_lines = subject.classify [ | ||
"# :nocov:", | ||
"def hi", | ||
"end", | ||
"# :nocov:", | ||
] | ||
describe ":nocov: blocks" do | ||
it "determines :nocov: blocks are not-relevant" do | ||
classified_lines = subject.classify [ | ||
"# :nocov:", | ||
"def hi", | ||
"end", | ||
"# :nocov:", | ||
] | ||
|
||
expect(classified_lines.length).to eq 4 | ||
expect(classified_lines).to all be_irrelevant | ||
end | ||
|
||
it "determines all lines after a non-closing :nocov: as not-relevant" do | ||
classified_lines = subject.classify [ | ||
"# :nocov:", | ||
"puts 'Not relevant'", | ||
"# :nocov:", | ||
"puts 'Relevant again'", | ||
"puts 'Still relevant'", | ||
"# :nocov:", | ||
"puts 'Not relevant till the end'", | ||
"puts 'Ditto'", | ||
] | ||
|
||
expect(classified_lines.length).to eq 8 | ||
|
||
expect(classified_lines.length).to eq 4 | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
end | ||
end | ||
end | ||
end | ||
|
@@ -56,7 +95,7 @@ | |
end | ||
end | ||
|
||
RSpec::Matchers.define :be_not_relevant do | ||
RSpec::Matchers.define :be_irrelevant do | ||
match do |actual| | ||
actual == SimpleCov::LinesClassifier::NOT_RELEVANT | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)