Ensure branches Hash is present when supported

[marcotc]

While working on a project that supports both Ruby 2.3 and 2.5, we merge our coverage results across all versions.

While trying to enable branch, coverage we came across an issue with merging coverage results.

NoMethodError: undefined method `flat_map' for nil:NilClass

From lib/simplecov/source_file.rb:269:

simplecov/lib/simplecov/source_file.rb

Lines 268 to 269 in dd35066

	coverage_branch_data = coverage_data.fetch("branches", {})
	branches = coverage_branch_data.flat_map do |condition, coverage_branches|

This issue happens because merging coverage results that don't include "branches" data inside a Ruby process that supports branch coverage can lead to a uninitialized combination["branches" (value nil) here:

simplecov/lib/simplecov/combine/files_combiner.rb

Line 19 in dd35066

combination["branches"] = Combine.combine(BranchesCombiner, coverage_a["branches"], coverage_b["branches"]) if SimpleCov.branch_coverage?

This nil value then causes the issue mentioned earlier, when coverage_data.fetch("branches", {}).flat_map gets invoked.

This PR changes the FilesCombiner to ensure that, when SimpleCov.branch_coverage? is supported, branches is always a Hash.

[PragTob]

Hi there,

thanks for your contribution. 🎉

I'm a bit torn about this. We don't support Ruby 2.3. So, consuming Ruby 2.3 in Ruby 2.5+ seems odd... I can see where you're coming from though. It sets a bit of a dangerous precedent though for the direction of the project - should we always assume that we must be able to consume coverage reports generated by a ruby version that supports a feature set different from our own?
There's a non Ruby 2.3 example though: merging JRuby reports with CRuby reports.

How and where are you doing the merge of the coverage reports? Are you product the resultset and then merging them via collate?

I know it's very little code right now, but if we took this in I wonder if it'd be in the best place for it.

Anyhow, have a bunny picture:

[PragTob]

You can filter on the spec level: https://relishapp.com/rspec/rspec-core/v/3-10/docs/filtering/conditional-filters

[marcotc]

TIL, thank you for that! Addressed.

[PragTob]

As for if we wanted to support this I'm thinking where the best place for it would be...

putting it into Combine might be most general as another fallback that'd make sense:

simplecov/lib/simplecov/combine.rb

Lines 26 to 28 in 75e1c63

	def existing_coverage(coverage_a, coverage_b)
	coverage_a || coverage_b
	end

on the other hand, the code you linked deals specifically with building branches - why would we invoke this with empty data rather than avoid doing it? Maybe that'd be the better move if we don't have branch data.

simplecov/lib/simplecov/source_file.rb

Lines 268 to 269 in dd35066

	coverage_branch_data = coverage_data.fetch("branches", {})
	branches = coverage_branch_data.flat_map do |condition, coverage_branches|

Like if we did something like:

coverage_branch_data = coverage_data["branches"]
# some why comment on how we might end up in this state and why we should keep it
return unless coverage_branch_data

...

Would that also solve the problem or just lead to more problems as branches is not expected to be nil? Plus of course now memoization doesn't work any more...

If you get nil from coverage_branch_data = coverage_data.fetch("branches", {}) it also means there is a value there and it is nil. Probably because in the combiner one file doesn't have branch coverage data and the other one didn't load the file at all meaning (which is why it also has no branch data about it).

Interesting case 🤔

[PragTob]

The JRuby CI failure should hopefully be fixed in #973

[marcotc]

👋 @PragTob, thank you for the bunny!

We are collating the coverage results running our test suite with multiple versions of Ruby (2.0 to 3.0 + latest JRuby): https://github.com/DataDog/dd-trace-rb/blob/d6e0e934fa97330ed4934853aa2442649d070e84/Rakefile#L942-L956. We collate the results under Ruby 2.7.

We do this because some code paths are exclusive to a specific version of Ruby, so the combined coverage is what we are after. We also keep a separate report per language, but that one is not affected by this issue.

The error is triggered when SimpleCov processes files that are only loaded by versions of Ruby that do not support branch coverage (e.g. Rails 4.0 tests run on Ruby 2.3, but are not even loaded in Ruby 3.0). Files that are loaded in all Ruby versions are merged correctly across all versions of Ruby, regardless of their branch coverage support.

Our .simplecov file, for reference: https://github.com/DataDog/dd-trace-rb/blob/branch-coverage/.simplecov

Regarding possible solutions:

Making the change in

simplecov/lib/simplecov/combine.rb

Lines 26 to 28 in 75e1c63

	def existing_coverage(coverage_a, coverage_b)
	coverage_a || coverage_b
	end

has the complication of SimpleCov::Combine being used for both arrays (for line coverage) and hash (for branch coverage). We would have to have distinct empty default values for each case. Nothing that can't be achieved with an extra argument, but I thought that leaving this responsibility to the caller made more sense when I was writing this patch.

why would we invoke this with empty data rather than avoid doing it? Maybe that'd be the better move if we don't have branch data.

I interpreted this line, coverage_data.fetch("branches", {}), as a fallback in case something "bad" happened and we weren't able to populate "branches" for some reason. My inkling was to "correct" that bad path that did not populate "branches" earlier, so that we wouldn't have to fetch with a fallback default here. But this was just my narrow, non-holistic reading of it.

Because this error happens in my gem when we do expect branch coverage to be reported, I'm not sure if interpreting the lack of coverage_data["branches"] as an opportunity to skip this kind of coverage sounds correct (again, my untrained opinion here).

Overall, I agree with your sentiment that this might not be the very best solution to the issue at hand. The change I introduced, in the place it was introduced, wasn't my first option, but it seem like themost reasonable one after browsing the code base for a while. I'm more than willing to make any changes required here, if you have a suggestion.

Thank you so much for such a detailed review so far! 🙇