-
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
Rewrite merge helpers to work with frozen objects #558
Changes from all 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,39 @@ | ||
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.map.with_index do |count1, index| | ||
count2 = file2[index] | ||
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,92 @@ | ||
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 | ||
|
||
it "merges frozen resultsets" 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, | ||
}.freeze | ||
|
||
merged_result = SimpleCov::RawCoverage.merge_results(resultset1, resultset2) | ||
expect(merged_result.keys).to eq(resultset1.keys) | ||
expect(merged_result.values.map(&:frozen?)).to eq([false, false]) | ||
expect(merged_result[source_fixture("sample.rb")]).to eq([1, 1, 2, 2, nil, nil, 2, 2, nil, nil]) | ||
expect(merged_result[source_fixture("app/models/user.rb")]).to eq([nil, 1, 1, 1, nil, nil, 1, 0, nil, nil]) | ||
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