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.

Sign up for GitHub

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

Jump to bottom

Rewrite merge helpers to work with frozen objects #558

Merged
merged 10 commits into from
Mar 5, 2017
Merged

Rewrite merge helpers to work with frozen objects #558

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9bb1aa3
Rewrite merge helpers to work with frozen objects
aroben Feb 14, 2017
a7747c1
MergeHelper.merge_resultsets -> RawCoverage.merge_results
aroben Feb 14, 2017
d08e569
Split ResultMerger and RawCoverage specs into separate files
aroben Feb 14, 2017
c29bbcd
Add support for merging more than two Coverage.result hashes
aroben Feb 14, 2017
7142edf
Add ResultMerger.merge_results
aroben Feb 14, 2017
6dafc25
Fix RuboCop offenses
aroben Feb 14, 2017
794cecb
Use map.with_index instead of zip for performance
aroben Feb 17, 2017
fbebb99
Make frozen result merging spec more clear
aroben Feb 17, 2017
e864390
Test merging frozen resultsets more thoroughly
aroben Feb 21, 2017
c7a9d82
Use double quotes to appease RuboCop
aroben Feb 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/simplecov.rb
View file
Open in desktop
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/merge_helper"
require "simplecov/result_merger"
require "simplecov/command_guesser"
require "simplecov/version"
Expand Down
31 changes: 31 additions & 0 deletions lib/simplecov/merge_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
module SimpleCov
module MergeHelper
class << self
# Merges two Coverage.result hashes
def merge_resultsets(a, b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a long needed feature. Any reason it shouldn't just be a function on the ResultMerger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting in on ResultMerger seems sensible. I considered it, then thought maybe it'd be nicer to keep this code self-contained. I.e., ResultMerger manages .resultset.json, and this module manages merging results in-memory. But it was a close call, and if you prefer moving it to ResultMerger I'll do that.

(a.keys | b.keys).each_with_object({}) do |filename, merged|
result1 = a[filename]
result2 = b[filename]
merged[filename] = if result1 && result2
merge_arrays(result1, result2)
else
(result1 || result2).dup
end
end
end

private

def merge_arrays(a, b)
a.zip(b).map do |count1, count2|
sum = count1.to_i + count2.to_i
if sum.zero? && (count1.nil? || count2.nil?)
nil
else
sum
end
end
end
end
end
end
37 changes: 0 additions & 37 deletions lib/simplecov/merge_helpers.rb
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion lib/simplecov/result.rb
View file
Open in desktop
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 •
edited
Loading

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
2 changes: 1 addition & 1 deletion lib/simplecov/result_merger.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def results
def merged_result
merged = {}
results.each do |result|
merged = result.original_result.merge_resultset(merged)
merged = SimpleCov::MergeHelper.merge_resultsets(result.original_result, merged)
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't it be nice if this were merged = SimpleCov::RawCoverage.merge_results(results.map(&:original_result))

module SimpleCov::RawCoverage
  module_function

  def merge_results(*results)
    Array(results).reduce({}) do |result, merged|
      merge_resultset(result, merged)
    end
  end

  def merge_resultset(resultset1, resultset2)
    (resultset1.keys | resultset2.keys).each_with_object({}) do |filename, merged_resultset|
      coverage1 = resultset1[filename]
      coverage2 = resultset2[filename]
      merged_resultset[filename] = merge_file_coverage(coverage1, coverage2)
    end
  end

  def merge_file_coverage(coverage1, coverage2)
    if coverage1 && coverage2
      coverage1.zip(coverage2).map { |count1, count2|
        sum = count1.to_i + count2.to_i
        if sum.zero? && (count1.nil? || count2.nil?)
          nil
        else
          sum
        end
      }
    else
      (coverage1 || coverage2).dup
    end
  end
end

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 had actually been thinking about adding a method that takes an array of SimpleCov::Result and merges them. ResultMerger does this internally but doesn't expose it as an API. That could certainly build on the API you've proposed here.

Earlier you mentioned putting this code in ResultMerger, but now it sounds like you'd like it in a new RawCoverage module instead? Happy to do whatever seems best to you.

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 have a strong feeling about it except that this something I've been meaning to do or hoping someone would do for months so 👍 to you! Only real concern is that we want to make a small public interface so that we can change internal details. So, maybe a method on ResultMerger that calls the helper or whatever, and we can change the whatever however whenever :)

end
result = SimpleCov::Result.new(merged)
# Specify the command name
Expand Down
24 changes: 21 additions & 3 deletions spec/merge_helpers_spec.rb → spec/merge_helper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require "helper"

if SimpleCov.usable?
describe "merge helpers" do
describe "merge helper" 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 @@ -26,7 +26,7 @@

context "a merge" do
subject do
@resultset1.merge_resultset(@resultset2)
SimpleCov::MergeHelper.merge_resultsets(@resultset1, @resultset2)
end

it "has proper results for sample.rb" do
Expand Down Expand Up @@ -122,5 +122,23 @@
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::MergeHelper.merge_resultsets(@resultset1, @resultset2)
end
end
end
end