-
Notifications
You must be signed in to change notification settings - Fork 564
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
Rewrite merge helpers to work with frozen objects #558
Changes from 6 commits
9bb1aa3
a7747c1
d08e569
c29bbcd
7142edf
6dafc25
794cecb
fbebb99
e864390
c7a9d82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
module SimpleCov | ||
module RawCoverage | ||
module_function | ||
|
||
# Merges multiple Coverage.result hashes | ||
def merge_results(*results) | ||
results.reduce({}) do |result, merged| | ||
merge_resultsets(result, merged) | ||
end | ||
end | ||
|
||
# Merges two Coverage.result hashes | ||
def merge_resultsets(result1, result2) | ||
(result1.keys | result2.keys).each_with_object({}) do |filename, merged| | ||
file1 = result1[filename] | ||
file2 = result2[filename] | ||
merged[filename] = merge_file_coverage(file1, file2) | ||
end | ||
end | ||
|
||
def merge_file_coverage(file1, file2) | ||
return (file1 || file2).dup unless file1 && file2 | ||
|
||
file1.zip(file2).map do |count1, count2| | ||
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 wonder about zip vs. map.with_index GC.disable
a = Array.new(100) { 1 }.freeze; b = Array.new(100) { 2 }.freeze
count = GC.stat[:total_allocated_objects]; a.zip(b).map {|l,r| l + r }.tap { puts GC.stat[:total_allocated_objects] - count }
# 104
# => [3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3]
count = GC.stat[:total_allocated_objects]; a.map.with_index {|i,index| i + b[index] }.tap { puts GC.stat[:total_allocated_objects] - count }
# 5
# => [3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3
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. looks like previous behavior was each_with_index - def merge_resultset(array)
- new_array = dup
- array.each_with_index do |element, i|
- pair = [element, new_array[i]]
- new_array[i] = if pair.any?(&:nil?) && pair.map(&:to_i).all?(&:zero?)
- nil
- else
- element.to_i + new_array[i].to_i
- end
- end
- new_array
- 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. Good digging into that, thanks! :) 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. Looks like we can have the best of both worlds by using bench.rbrequire "benchmark/ips"
a = Array.new(100) { 1 }.freeze
b = Array.new(100) { 2 }.freeze
Benchmark.ips do |x|
x.report('lazy.zip.map') { a.lazy.zip(b).map {|l,r| l + r } }
x.report('zip.map') { a.zip(b).map {|l,r| l + r } }
x.report('map.with_index') { a.map.with_index {|i,index| i + b[index] } }
x.report('each_with_index') do |times|
new_array = []
i = 0
while i < times
a.each_with_index {|i,index| new_array << i + b[index] }
i = i + 1
end
end
x.compare!
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. Whoops, that was so lazy that it didn't even create a result array. A correct implementation (with a
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've reworked this to use |
||
merge_line_coverage(count1, count2) | ||
end | ||
end | ||
|
||
def merge_line_coverage(count1, count2) | ||
sum = count1.to_i + count2.to_i | ||
if sum.zero? && (count1.nil? || count2.nil?) | ||
nil | ||
else | ||
sum | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,6 @@ class Result | |
# Initialize a new SimpleCov::Result from given Coverage.result (a Hash of filenames each containing an array of | ||
# coverage data) | ||
def initialize(original_result) | ||
original_result = original_result.dup.extend(SimpleCov::HashMergeHelper) unless original_result.is_a? SimpleCov::HashMergeHelper | ||
@original_result = original_result.freeze | ||
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. 👍 |
||
@files = SimpleCov::FileList.new(original_result.map do |filename, coverage| | ||
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. Looks good. In my own testing I found that the def src
# We intentionally read source code lazily to
# suppress reading unused source code.
@src ||= File.open(filename, "rb", &:readlines)
end require 'simplecov'
require 'json'
SimpleCov.coverage_dir 'coveragetest'
SimpleCov.formatters = [SimpleCov::Formatter::SimpleFormatter]
sourcefiles = Dir.glob('./resultset*')
# work around `if File.file?(filename)` when running on other filesystem
add_files = ->(sr) { files = sr.original_result.map {|filename, coverage| SimpleCov::SourceFile.new(filename, coverage) }.compact.sort_by(&:filename); sr.instance_variable_set(:@files, SimpleCov::FileList.new(files)); sr }
results = sourcefiles.map { |file| SimpleCov::Result.from_hash(JSON.parse(File.read(file))) }.each do |result|
add_files.(result)
end; nil
result = SimpleCov::ResultMerger.merge_results(*results)
add_files.(result)
result.format! 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. so erm, I don't fully understand. Is this good to merge now or is this a bigger problem? (first you said it's probably good for another PR but then you said it doesn't work with the html formatter, so I'm a bit confused) 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. @PragTob good to merge. The 'good for another pr', would be a way to merge files not represented on the current file system, since that's out of scope. This works fine when running on the same filesystem (or if you were to munge the paths). |
||
SimpleCov::SourceFile.new(filename, coverage) if File.file?(filename) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,20 +54,24 @@ def results | |
results | ||
end | ||
|
||
# Merge two or more SimpleCov::Results into a new one with merged | ||
# coverage data and the command_name for the result consisting of a join | ||
# on all source result's names | ||
def merge_results(*results) | ||
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. 👍 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. Since there's already a 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 think you're right that there could be some confusion. I'm a little wary of calling this Without any concern for backward compatibility, I'd be tempted to rename 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. @bf4 Let me know how you'd like to proceed here. ❤️ 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 don't think I'd like to break backwards compatibility. I mean I wouldn't really consider it a public API of this gem, but from what I've seen I think some people have fiddled with this to merge results from multiple sources themselves so I wouldn't wanna break it for them. 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. @PragTob I listed a few in #558 (review) 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. |
||
merged = SimpleCov::RawCoverage.merge_results(*results.map(&:original_result)) | ||
result = SimpleCov::Result.new(merged) | ||
# Specify the command name | ||
result.command_name = results.map(&:command_name).sort.join(", ") | ||
result | ||
end | ||
|
||
# | ||
# Gets all SimpleCov::Results from cache, merges them and produces a new | ||
# SimpleCov::Result with merged coverage data and the command_name | ||
# for the result consisting of a join on all source result's names | ||
# | ||
def merged_result | ||
merged = {} | ||
results.each do |result| | ||
merged = result.original_result.merge_resultset(merged) | ||
end | ||
result = SimpleCov::Result.new(merged) | ||
# Specify the command name | ||
result.command_name = results.map(&:command_name).sort.join(", ") | ||
result | ||
merge_results(*results) | ||
end | ||
|
||
# Saves the given SimpleCov::Result in the resultset cache | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
require "helper" | ||
|
||
if SimpleCov.usable? | ||
describe SimpleCov::RawCoverage do | ||
describe "with two faked coverage resultsets" do | ||
before do | ||
@resultset1 = { | ||
source_fixture("sample.rb") => [nil, 1, 1, 1, nil, nil, 1, 1, nil, nil], | ||
source_fixture("app/models/user.rb") => [nil, 1, 1, 1, nil, nil, 1, 0, nil, nil], | ||
source_fixture("app/controllers/sample_controller.rb") => [nil, 1, 1, 1, nil, nil, 1, 0, nil, nil], | ||
source_fixture("resultset1.rb") => [1, 1, 1, 1], | ||
source_fixture("parallel_tests.rb") => [nil, 0, nil, 0], | ||
source_fixture("conditionally_loaded_1.rb") => [nil, 0, 1], # loaded only in the first resultset | ||
source_fixture("three.rb") => [nil, 1, 1], | ||
} | ||
|
||
@resultset2 = { | ||
source_fixture("sample.rb") => [1, nil, 1, 1, nil, nil, 1, 1, nil, nil], | ||
source_fixture("app/models/user.rb") => [nil, 1, 5, 1, nil, nil, 1, 0, nil, nil], | ||
source_fixture("app/controllers/sample_controller.rb") => [nil, 3, 1, nil, nil, nil, 1, 0, nil, nil], | ||
source_fixture("resultset2.rb") => [nil, 1, 1, nil], | ||
source_fixture("parallel_tests.rb") => [nil, nil, 0, 0], | ||
source_fixture("conditionally_loaded_2.rb") => [nil, 0, 1], # loaded only in the second resultset | ||
source_fixture("three.rb") => [nil, 1, 4], | ||
} | ||
|
||
@resultset3 = { | ||
source_fixture("three.rb") => [nil, 1, 2], | ||
} | ||
end | ||
|
||
context "a merge" do | ||
subject do | ||
SimpleCov::RawCoverage.merge_results(@resultset1, @resultset2, @resultset3) | ||
end | ||
|
||
it "has proper results for sample.rb" do | ||
expect(subject[source_fixture("sample.rb")]).to eq([1, 1, 2, 2, nil, nil, 2, 2, nil, nil]) | ||
end | ||
|
||
it "has proper results for user.rb" do | ||
expect(subject[source_fixture("app/models/user.rb")]).to eq([nil, 2, 6, 2, nil, nil, 2, 0, nil, nil]) | ||
end | ||
|
||
it "has proper results for sample_controller.rb" do | ||
expect(subject[source_fixture("app/controllers/sample_controller.rb")]).to eq([nil, 4, 2, 1, nil, nil, 2, 0, nil, nil]) | ||
end | ||
|
||
it "has proper results for resultset1.rb" do | ||
expect(subject[source_fixture("resultset1.rb")]).to eq([1, 1, 1, 1]) | ||
end | ||
|
||
it "has proper results for resultset2.rb" do | ||
expect(subject[source_fixture("resultset2.rb")]).to eq([nil, 1, 1, nil]) | ||
end | ||
|
||
it "has proper results for parallel_tests.rb" do | ||
expect(subject[source_fixture("parallel_tests.rb")]).to eq([nil, nil, nil, 0]) | ||
end | ||
|
||
it "has proper results for conditionally_loaded_1.rb" do | ||
expect(subject[source_fixture("conditionally_loaded_1.rb")]).to eq([nil, 0, 1]) | ||
end | ||
|
||
it "has proper results for conditionally_loaded_2.rb" do | ||
expect(subject[source_fixture("conditionally_loaded_2.rb")]).to eq([nil, 0, 1]) | ||
end | ||
|
||
it "has proper results for three.rb" do | ||
expect(subject[source_fixture("three.rb")]).to eq([nil, 3, 7]) | ||
end | ||
end | ||
end | ||
|
||
describe "with frozen resultsets" do | ||
before do | ||
@resultset1 = { | ||
source_fixture("sample.rb").freeze => [nil, 1, 1, 1, nil, nil, 1, 1, nil, nil].freeze, | ||
source_fixture("app/models/user.rb").freeze => [nil, 1, 1, 1, nil, nil, 1, 0, nil, nil].freeze, | ||
}.freeze | ||
|
||
@resultset2 = { | ||
source_fixture("sample.rb").freeze => [1, nil, 1, 1, nil, nil, 1, 1, nil, nil].freeze, | ||
source_fixture("app/models/user.rb").freeze => [nil, 1, 5, 1, nil, nil, 1, 0, nil, nil].freeze, | ||
}.freeze | ||
end | ||
|
||
it "can merge without exceptions" do | ||
SimpleCov::RawCoverage.merge_results(@resultset1, @resultset2) | ||
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. 👍 To clarify intent we could do 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. Yeah, I would normally do something at the top like example do
expect {
SimpleCov::RawCoverage.merge_results(@resultset1, @resultset2)
}.not_to raise_error
end I like 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 added |
||
end | ||
end | ||
end | ||
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.
I like the usage of
|
here