-
Notifications
You must be signed in to change notification settings - Fork 551
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
Conversation
Coverage.result returns a frozen Hash containing frozen Strings as keys and frozen Arrays as values. The existing merge helper did not work when passed a Coverage.result directly, because it would try to extend the frozen Arrays. This is not a problem in typical SimpleCov usage because usually Coverage.result has been roundtripped through .resultset.json resulting in an unfrozen copy. But when trying to do manual merging of Coverage.result hashes the use of extend gets in the way. The merge helper has now been rewritten a method that takes two Hashes and doesn't modify them or their contents at all. While I was touching this code I tried to clean it up a little bit as well.
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.
This would be a great feature! Some thoughts, not prescriptive.
Related:
- http://technology.indiegogo.com/2015/08/how-we-get-coverage-on-parallelized-test-builds/
- https://gist.github.com/evanwhalen/f74879e0549b67eb17bb
- https://discuss.circleci.com/t/circleci-parallel-builds-and-code-climate/1028
- https://discuss.circleci.com/t/referencing-latest-build-artifacts/931
- https://github.com/tknerr/circleci-redirector
- merge all reports into 1 before submitting codeclimate/ruby-test-reporter#80
@@ -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 |
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.
👍
lib/simplecov/merge_helper.rb
Outdated
module MergeHelper | ||
class << self | ||
# Merges two Coverage.result hashes | ||
def merge_resultsets(a, b) |
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 think this is a long needed feature. Any reason it shouldn't just be a function on the ResultMerger?
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.
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.
lib/simplecov/result_merger.rb
Outdated
@@ -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) |
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.
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
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 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.
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 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 :)
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 taking a look!
lib/simplecov/merge_helper.rb
Outdated
module MergeHelper | ||
class << self | ||
# Merges two Coverage.result hashes | ||
def merge_resultsets(a, b) |
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.
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.
lib/simplecov/result_merger.rb
Outdated
@@ -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) |
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 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.
RawCoverage.merge_results can now merge an arbitrary number of hashes.
This just extracts the logic from .merged_result into a method that takes a set of SimpleCov::Results as input. This lets clients merge their own SimpleCov::Result objects without roundtripping through .resultset.json.
@bf4 OK I added the new APIs. I didn't add a specific test for |
# 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 comment
The 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 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?
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 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::Result
s, 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
.
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.
@bf4 Let me know how you'd like to proceed here. ❤️
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 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Getting rid off those has been on my todo list :D
This many changes make me a bit scared that anything might have been broken, but I'll trust the tests.
Thanks!
|
||
# Merges two Coverage.result hashes | ||
def merge_resultsets(result1, result2) | ||
(result1.keys | result2.keys).each_with_object({}) do |filename, merged| |
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
# 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 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.
spec/raw_coverage_spec.rb
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
👍
To clarify intent we could do expect {}.not_to raise_error
or something, not strictly needed though
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.
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 example
for the 'this is a valid usage', though I know some don't like it
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 added expect {}.not_to raise_error
to this test.
lib/simplecov/raw_coverage.rb
Outdated
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 comment
The 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
require 'benchmark/ips'
Benchmark.ips do |x| 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.compare! end
GC disabled
Warming up --------------------------------------
zip.map 5.039k i/100ms
map.with_index 7.615k i/100ms
Calculating -------------------------------------
zip.map 40.456k (±10.8%) i/s - 201.560k in 5.040494s
map.with_index 76.949k (± 2.9%) i/s - 388.365k in 5.051459s
Comparison:
map.with_index: 76949.5 i/s
zip.map: 40455.6 i/s - 1.90x slower
GC enabled
Warming up --------------------------------------
zip.map 5.860k i/100ms
map.with_index 7.899k i/100ms
Calculating -------------------------------------
zip.map 59.204k (± 3.4%) i/s - 298.860k in 5.053878s
map.with_index 80.775k (± 3.0%) i/s - 410.748k in 5.089637s
Comparison:
map.with_index: 80774.6 i/s
zip.map: 59204.5 i/s - 1.36x slower
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.
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
Benchmark.ips do |x| 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
Warming up --------------------------------------
zip.map 5.909k i/100ms
map.with_index 7.878k i/100ms
each_with_index 7.434k i/100ms
Calculating -------------------------------------
zip.map 60.240k (± 3.2%) i/s - 301.359k in 5.007900s
map.with_index 80.260k (± 5.7%) i/s - 401.778k in 5.025614s
each_with_index 84.993k (± 5.0%) i/s - 423.738k in 5.000672s
Comparison:
each_with_index: 84993.4 i/s
map.with_index: 80260.1 i/s - same-ish: difference falls within error
zip.map: 60239.5 i/s - 1.41x slower
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.
Good digging into that, thanks! :)
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.
Looks like we can have the best of both worlds by using lazy.zip.map
:
bench.rb
require "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
Warming up --------------------------------------
lazy.zip.map 17.413k i/100ms
zip.map 7.960k i/100ms
map.with_index 10.779k i/100ms
each_with_index 9.897k i/100ms
Calculating -------------------------------------
lazy.zip.map 186.025k (± 3.4%) i/s - 940.302k in 5.060879s
zip.map 81.511k (± 4.0%) i/s - 413.920k in 5.087448s
map.with_index 110.948k (± 3.4%) i/s - 560.508k in 5.058655s
each_with_index 115.360k (± 2.2%) i/s - 583.923k in 5.064205s
Comparison:
lazy.zip.map: 186024.9 i/s
each_with_index: 115360.3 i/s - 1.61x slower
map.with_index: 110947.8 i/s - 1.68x slower
zip.map: 81510.6 i/s - 2.28x slower
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.
Whoops, that was so lazy that it didn't even create a result array. A correct implementation (with a .to_a
on the end) is slower:
Warming up --------------------------------------
lazy.zip.map.to_a 1.450k i/100ms
zip.map 7.869k i/100ms
map.with_index 10.748k i/100ms
each_with_index 9.976k i/100ms
Calculating -------------------------------------
lazy.zip.map.to_a 13.688k (±14.1%) i/s - 68.150k in 5.097768s
zip.map 79.369k (± 8.6%) i/s - 393.450k in 5.005312s
map.with_index 110.505k (± 4.0%) i/s - 558.896k in 5.067126s
each_with_index 114.300k (± 4.2%) i/s - 578.608k in 5.071720s
Comparison:
each_with_index: 114300.5 i/s
map.with_index: 110505.2 i/s - same-ish: difference falls within error
zip.map: 79368.7 i/s - 1.44x slower
lazy.zip.map.to_a: 13688.2 i/s - 8.35x slower
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've reworked this to use map.with_index
. Thanks for the attention to performance!
$ cat bench.rb require "benchmark/ips" a = Array.new(100) { 1 }.freeze b = Array.new(100) { 2 }.freeze Benchmark.ips do |x| x.report('lazy.zip.map.to_a') { a.lazy.zip(b).map {|l,r| l + r }.to_a } 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 $ ruby bench.rb Warming up -------------------------------------- lazy.zip.map.to_a 1.450k i/100ms zip.map 7.869k i/100ms map.with_index 10.748k i/100ms each_with_index 9.976k i/100ms Calculating ------------------------------------- lazy.zip.map.to_a 13.688k (±14.1%) i/s - 68.150k in 5.097768s zip.map 79.369k (± 8.6%) i/s - 393.450k in 5.005312s map.with_index 110.505k (± 4.0%) i/s - 558.896k in 5.067126s each_with_index 114.300k (± 4.2%) i/s - 578.608k in 5.071720s Comparison: each_with_index: 114300.5 i/s map.with_index: 110505.2 i/s - same-ish: difference falls within error zip.map: 79368.7 i/s - 1.44x slower lazy.zip.map.to_a: 13688.2 i/s - 8.35x slower
8846810
to
794cecb
Compare
Now explicitly assert that no error is raised.
I think that rubocop https://travis-ci.org/colszowka/simplecov/jobs/202748911
|
@bf4 if |
@PragTob |
spec/raw_coverage_spec.rb
Outdated
it "can merge without exceptions" do | ||
expect { | ||
SimpleCov::RawCoverage.merge_results(@resultset1, @resultset2) | ||
}.not_to raise_error |
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.
Let's make this a positive assertion and avoid rubocop style for now
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(source_fixture('sample.rb')).to eq([whatever])
expect(source_fixture('app/models/user.rb')).to eq([nil, 1, 5, 1, nil, nil, 1, 0, nil, nil])
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.
OK, adopted that test and pushed.
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.
why solve a problem when you can avoid it altogether? :)
RUbocop doesn't like single quotes - sorry :)
|
Hm, we need to teach RuboCop to lint comments on GitHub 😁. Fixed!
|
I'm trying this out. |
@bf4 what do you mean by "trying this out" ? Like you run it locally on your projects to make sure it's good for an impeding release? :) |
Yup... It looks good... But since have a use case, I wanted to try it
B mobile phone
… On Feb 23, 2017, at 1:04 PM, Tobias Pfeiffer ***@***.***> wrote:
@bf4 what do you mean by "trying this out" ? Like you run it locally on your projects to make sure it's good for an impeding release? :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@@ -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 | |||
@files = SimpleCov::FileList.new(original_result.map do |filename, coverage| |
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.
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!
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.
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 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).
@PragTob missing https://github.com/colszowka/simplecov/blob/master/CHANGELOG.md entry. We can add it if @aroben isn't around |
I usually add it on master after a merge so we should be fine :) |
@PragTob Then I'll leave it to you to bring this home :) |
0.14.1 2017-03-18 ([changes](simplecov-ruby/simplecov@v0.14.0...v0.14.1)) ======== ## Bugfixes * Files that were skipped as a whole/had no relevant coverage could lead to Float errors. See [#564](simplecov-ruby/simplecov#564) (thanks to @stevehanson for the report in [#563](simplecov-ruby/simplecov#563)) 0.14.0 2017-03-15 ([changes](simplecov-ruby/simplecov@v0.13.0...v0.14.0)) ========== ## Enhancements * Officially support JRuby 9.1+ going forward (should also work with previous releases). See [#547](simplecov-ruby/simplecov#547) (ping @PragTob when encountering issues) * Add Channel group to Rails profile, when `ActionCable` is loaded. See [#492](simplecov-ruby/simplecov#492) (thanks @BenMorganIO) * Stop `extend`ing instances of `Array` and `Hash` during merging results avoiding problems frozen results while manually merging results. See [#558](simplecov-ruby/simplecov#558) (thanks @aroben) ## Bugfixes * Fix parallel_tests when a thread ends up running no tests. See [#533](simplecov-ruby/simplecov#533) (thanks @cshaffer) * Skip the `:nocov:` comments along with the code that they skip. See [#551](simplecov-ruby/simplecov#551) (thanks @ebiven) * Fix crash when Home environment variable is unset. See [#482](simplecov-ruby/simplecov#482) (thanks @waldyr) * Make track_files work again when explicitly setting it to nil. See [#463](simplecov-ruby/simplecov#463) (thanks @craiglittle) * Do not overwrite .last_run.json file when refuse_coverage_drop option is enabled and the coverage has dropped (lead to you being able to just rerun tests and everything was _fine_). See [#553](simplecov-ruby/simplecov#553) (thanks @Miloshes) 0.13.0 2016-01-25 ([changes](simplecov-ruby/simplecov@v0.12.0...v0.13.0)) ========== ## Enhancements * Faster run times when a very large number of files is loaded into SimpleCov. See [#520](simplecov-ruby/simplecov#520) (thanks @alyssais) * Only read in source code files that are actually used (faster when files are ignored etc.). See [#540](simplecov-ruby/simplecov#540) (tahks @yui-knk) ## Bugfixes * Fix merging of resultsets if a file is missing on one side. See [#513](simplecov-ruby/simplecov#513) (thanks @hanazuki) * Fix Ruby 2.4 deprecation warnings by using Integer instead of Fixnum. See [#523](simplecov-ruby/simplecov#523) (thanks @nobu) * Force Ruby 2 to json 2. See [dc7417d50](simplecov-ruby/simplecov@dc7417d) (thanks @amatsuda) * Various other gem dependency fixes for different gems on different ruby versions. (thanks @amatsuda)
Coverage.result
returns a frozen Hash containing frozen Strings as keys and frozen Arrays as values. The existing merge helper did not work when passed aCoverage.result
directly, because it would try toextend
the frozen Arrays. This is not a problem in typical SimpleCov usage because usually Coverage.result has been roundtripped through.resultset.json
resulting in an unfrozen copy. But when trying to do manual merging ofCoverage.result
hashes the use ofextend
gets in the way.The merge helper has now been rewritten a method that takes two Hashes and doesn't modify them or their contents at all. While I was touching this code I tried to clean it up a little bit as well.