Skip to content
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

Merged
merged 10 commits into from
Mar 5, 2017
2 changes: 1 addition & 1 deletion lib/simplecov.rb
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ def usable?
require "simplecov/filter"
require "simplecov/formatter"
require "simplecov/last_run"
require "simplecov/merge_helpers"
require "simplecov/raw_coverage"
require "simplecov/result_merger"
require "simplecov/command_guesser"
require "simplecov/version"
Expand Down
37 changes: 0 additions & 37 deletions lib/simplecov/merge_helpers.rb

This file was deleted.

39 changes: 39 additions & 0 deletions lib/simplecov/raw_coverage.rb
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|
Copy link
Collaborator

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

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
1 change: 0 additions & 1 deletion lib/simplecov/result.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@files = SimpleCov::FileList.new(original_result.map do |filename, coverage|
Copy link
Collaborator

@bf4 bf4 Feb 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. In my own testing I found that the files list wasn't set correctly if I was trying to load and merge the results from another machine. Probably good for a future pr... but doesn't work with html formatter because it tries to read each file to overlay coverage in report source_file.rb

 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!

Copy link
Collaborator

Choose a reason for hiding this comment

The 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)

Copy link
Collaborator

Choose a reason for hiding this comment

The 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)
Expand Down
20 changes: 12 additions & 8 deletions lib/simplecov/result_merger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's already a results method and it is passed to this method as the argument, perhaps we can avoid possible which-results-are-we-using by changing the argument name? *resultsets? maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 resultsets because this is an array of SimpleCov::Results, not an array of .resultset.json-type data. That actually seems like a problem with RawCoverage.merge_resultsets too. But I'm not super familiar with SimpleCov's terminology, so maybe resultsets is an OK name here?

Without any concern for backward compatibility, I'd be tempted to rename ResultMerger.results to ResultMerger.stored_results, and leave the argument here as *results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bf4 Let me know how you'd like to proceed here. ❤️

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PragTob I listed a few in #558 (review)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PragTob @aroben how about

def merge_results(*results = results())

this is something that I know has been done in Rails at Matz's recommendation when the default value for a param is a method by the same name

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
Expand Down
92 changes: 92 additions & 0 deletions spec/raw_coverage_spec.rb
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
42 changes: 2 additions & 40 deletions spec/merge_helpers_spec.rb → spec/result_merger_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require "helper"

if SimpleCov.usable?
describe "merge helpers" do
describe SimpleCov::ResultMerger do
describe "with two faked coverage resultsets" do
before do
SimpleCov.use_merging true
Expand All @@ -12,7 +12,7 @@
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
}.extend(SimpleCov::HashMergeHelper)
}

@resultset2 = {
source_fixture("sample.rb") => [1, nil, 1, 1, nil, nil, 1, 1, nil, nil],
Expand All @@ -24,44 +24,6 @@
}
end

context "a merge" do
subject do
@resultset1.merge_resultset(@resultset2)
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
end

# See Github issue #6
it "returns an empty hash when the resultset cache file is empty" do
File.open(SimpleCov::ResultMerger.resultset_path, "w+") { |f| f.puts "" }
Expand Down