-
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
Add method coverage support #987
base: main
Are you sure you want to change the base?
Add method coverage support #987
Conversation
9851ba0
to
5987457
Compare
@PragTob hope you will find some time to review the changes. |
5987457
to
9a8b324
Compare
9ee16e9
to
0501b73
Compare
@@ -221,9 +242,9 @@ def ensure_remove_undefs(file_lines) | |||
end | |||
|
|||
def build_lines | |||
coverage_exceeding_source_warn if coverage_data["lines"].size > src.size | |||
coverage_exceeding_source_warn if lines_data.size > src.size |
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.
EDIT: I think this PR will solve a problem I'm having.
In 0.21.2
I'm encountering an error on this line when executing via rubycritic. Turns out coverage_data
is an Array (as documented), and was previously an Integer, so did this line ever work? Related to that, how does coverage_data.fetch(:branches, {})
(below) work?
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.
coverage_data
is a Hash in this PR. So it's strange that you have problems. Maybe you have coverage data in some old format that is breaking the code?
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.
Update: I see that rubycritic is doing a lot of stuff with simplecov internals. So I guess they will have to update the code if this PR is merged. They already do something similar here for example.
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.
@tycooon just to be clear, I'm not having any problem with this PR. I think this PR will solve my problem 🙂
Also for clarity: the error I encountered was no implicit conversion of String into Integer (TypeError)
. From my reading of the traceback it looks like this error is internal to simplecov
. Yes, it's possible that it's due to some sort of mishandling by rubycritic
(which invokes simplecov
through find_coverage_percentage
). But it seems smart to focus on simplecov
first because the existing code doesn't seem to make sense (which is what I was describing above). I'm looking forward to this PR helping to clarify SourceFile
, and hopefully also fixing the bug.
Traceback
running simple_cov Traceback (most recent call last): 19: from {PATH}bin/rubycritic:23:in `' 18: from {PATH}bin/rubycritic:23:in `load' 17: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/bin/rubycritic:10:in `' 16: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/cli/application.rb:21:in `execute' 15: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/commands/default.rb:19:in `execute' 14: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/commands/default.rb:24:in `critique' 13: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/analysers_runner.rb:29:in `run' 12: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/analysers_runner.rb:29:in `each' 11: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/analysers_runner.rb:32:in `block in run' 10: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/analysers/coverage.rb:20:in `run' 9: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/core/analysed_modules_collection.rb:32:in `each' 8: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/core/analysed_modules_collection.rb:32:in `each' 7: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/analysers/coverage.rb:21:in `block in run' 6: from {PATH}lib/ruby/gems/2.7.0/gems/rubycritic-4.6.1/lib/rubycritic/analysers/coverage.rb:38:in `find_coverage_percentage' 5: from {PATH}lib/ruby/gems/2.7.0/gems/simplecov-0.21.2/lib/simplecov/source_file.rb:81:in `covered_percent' 4: from {PATH}lib/ruby/gems/2.7.0/gems/simplecov-0.21.2/lib/simplecov/source_file.rb:35:in `coverage_statistics' 3: from {PATH}lib/ruby/gems/2.7.0/gems/simplecov-0.21.2/lib/simplecov/source_file.rb:333:in `line_coverage_statistics' 2: from {PATH}lib/ruby/gems/2.7.0/gems/simplecov-0.21.2/lib/simplecov/source_file.rb:241:in `lines_strength' 1: from {PATH}lib/ruby/gems/2.7.0/gems/simplecov-0.21.2/lib/simplecov/source_file.rb:43:in `lines' {PATH}lib/ruby/gems/2.7.0/gems/simplecov-0.21.2/lib/simplecov/source_file.rb:224:in `build_lines': no implicit conversion of String into Integer (TypeError)
Hey @PragTob, maybe you can take a look now? |
@PragTob ping! |
1 similar comment
@PragTob ping! |
2ce910b
to
54684e8
Compare
54684e8
to
8ede952
Compare
33cbb8e
to
574b45c
Compare
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.
Thanks for your work on this PR, it would make a welcome inclusion 🙏
I am curious - would method coverage improve coverage support for ruby 3's endless methods?
I have found that endless methods currently show as covered if they are never executed by tests.
Even if you have branching coverage enabled they report as fully covered.
Example class, spec and coverage report follow.
Note that uncovered_endless_method
shows as covered despite not being executed by the test.
Whereas endless_branching_method
shows branching coverage failing since it was executed.
I would have expected uncovered_endless_method
to show as partially covered.
class CoveragePoc
# this method is fully covered
def self.simple_method
:simple
end
# the :b branch for these methods is not covered
def self.endless_branching_method(arg) = arg ? :a : :b
def self.branching_method(arg)
arg ? :a : :b
end
# these methods are not covered at all
def self.uncovered_endless_method = :uncovered_endless
def self.uncovered_method
:uncovered
end
end
RSpec.describe CoveragePoc do
it 'works' do
expect(described_class.simple_method).to eq(:simple)
expect(described_class.branching_method(true)).to eq(:a)
expect(described_class.endless_branching_method(true)).to eq(:a)
end
end
See #782.
Also addresses #801 and #916.
Support for HTML formatter can be found here: simplecov-ruby/simplecov-html#110.
Some notes:
ResultSerialization
module).ResultMerger
optimizations :( Now it's operatingResult
objects, however it still parses and merges files one be one.lines: []
instead of"lines" => []
), just the same way you get it when calling Ruby'sCoverage.result
. Because of that, a lot of specs were updated.0x0000000000000000
.adapt_pre_simplecov_0_18_result
logic was moved toResultSerialization.deserialize
. Also old-style branch info is supported (eval
is still used for that).All this stuff has been battle-tested on a couple of big projects, some of which merge results of multiple CI jobs.