Add method coverage and decision coverage metrics

[srawlins]

This is a working proof-of-concept for tracking more advanced code coverage metrics in the Coverage module. Namely, Method Coverage (Function Coverage), and Decision Coverage (as defined in Steve Cornett's Code Coverage Analysis paper). I'd love feedback!

Since Ruby 1.9, the format of a Coverage report looks like:

{"foo.rb" => [1, 1, 10, nil, nil, 1, 1, nil, 0, nil]}

(from the rdoc) which says that, within "foo.rb":

• lines 1 and 2 were executed once,
• line 3 was executed 10 times,
• lines 4 and 5 were "disabled" as far as Coverage is concerned,
• etc.

This pull request changes the format to be more complicated. For the following ruby file:

def sum
  s = 0
  10.times do |x|
    s += x
  end

  if s == 45
    p :okay
    p :ok
  else
    p :ng
  end
end

def never_executed
  "badwrong"
end

def hello(name)
  puts "Hello #{name}"
end

sum; sum; hello("Sam")

Coverage.result would yield the following:

{"/Users/sam/code/ruby/cov_method.rb" => {
  lines: [1, 2, 2, 20, nil, nil, 2, 2, 2, nil, 0, nil, nil, nil, 1, 0, nil, nil, 1, 1, nil, nil, 1],
  methods: {1 => 2, 15 => 0, 19 => 1},
  decisions: {7=> [1, 0]}
}}

So the following things change about the coverage report:

1. The coverage report for a given file is a Hash, changed from an Array.
2. The execution count for each line is in Coverage.result[file][:lines], changed from Coverage.result[file].
3. The coverage report includes execution counts for each method in Coverage.result[file][:methods]. This object is a hash, with line number keys (where each method was defined), and execution count values ("calls"). This could be changed to be the same format as [:lines], but a Hash presumably saves space and time.
4. The coverage report includes execution counts for each "decision" in Coverage.result[file][:decisions]. This object is a Hash, with line number keys (the condition of an if, a when line, etc.), and count values for true decisions and false decisions. 7=>[2,1] states that the condition resulted in a truthy value 2 times, and a falsey value 1 time.

Decisions

A decision is the result of an if, elsif, unless, ?:, when, while, or until condition, which are all tracked with this new feature. Since the results of the conditions are tracked, void bodies will be noted. For example:

:yes if 2+2 == 4

will report that a conditional statement exists at line 1, and that statement resulted in a truthy decision 1 time, and a falsey decision 0 times, which means not all decisions were tested.

More Coverage

• I think that Statement Coverage, and Condition Coverage could be added to this feature, using the same strategies.
• I think that Method Coverage could be changed to a more general Function Coverage, which would also track calls to Procs (and lambda Procs) as well as Methods.

Caveats

I was not very clear on the magic numbers used in ruby.h, and just used values for the new macros that seemed to work.

for a in b is not tracked in Decision Coverage because that syntax does not really involve "Decisions". The sequence for a in b; code; end is just transformed into b.each { |a| code } at compile time. This makes me want to add Proc call coverage to the Method Coverage metric.

Updates

I completely rewrote and rebranded "Branch" coverage into "Decision" coverage after reading more of Steve Cornett's paper. The resultant metric is much more robust, tracking void elses, and one-line if/else combos much better.

[brianphillips]

I'd love to see this make its way into ruby core. Is this effectively a dead proposal?

[srawlins]

@brianphillips , I let this one drift for a looooong time. I should add one commit just to fix merge conflicts 😛 All of the discussion has been at Ruby bug #9508. Not sure why I never linked back to that bug here...

After 2.2.0-preview1 was announced, I let this go, fairly certain that it wouldn't be accepted into 2.2.0 after preview1 was already out. Now that 2.2.0 is out, I should fix the merge conflicts and poke Yusuke, who has generously dedicated a lot of time reviewing this already.

We left off on a discussion about how this PR makes testing-with-Coverage.start a bit slower than it was (testing-with-Coverage.start is already slower than testing-normally). IMHO you should kind of expect that coverage runs will be slower, but that sucks for anyone who doesn't want to wrap their Coverage.start line with if/else garbage. So I think what would make my proposal much more attractive is an optional arg to Coverage.start. I think it would go like this:

• Coverage.start before this PR: track line coverage; Coverage.result yields a Hash with Array values.
• Coverage.start after this PR: track line coverage; Coverage.result yields a Hash with Array values.
• Coverage.start(:all) after this PR: track line, method, and decision coverage; Coverage.result yields a Hash with Hash values, explained in the description above; each Hash value has keys :lines, :methods, :decisions.
• Coverage.start(:lines, :methods) after this PR: track line and method coverage only; Coverage.result yields a Hash with Hash values; each Hash value has keys :lines and :methods.

The only problem with this solution is that it makes tons of the deep C code wrapped in gross if/else statements. But it should make it faster...

[jlhonora]

Any updates on branch/decision coverage support? I believe this is specially important in ruby, given that statements like:

do_something if condition?

are reported as 100% covered with the current approach.

[hedgesky]

@srawlins, first of all, thanks a lot for all the effort you've put into this PR!

I'd like to make a suggestion about offered syntax: we could add an optional hash instead of a symbol (or a list of them) as an argument. It would make possible to add more options to the method in future without problems with backward compatibility. For example, something like

Coverage.start(only_lines: true)

Regarding speed issues: personally, I think that more complete coverage report overweight tests slowing down. After all, people use coverage tools to track down bugs and false 100% report may lead to missed issues. That's why I also think splitting internal coverage modes into :lines, :methods and :decisions isn't required: all of them are important. Also, as you mentioned earlier, such splitting would complicate internal code significantly and cause harm in future development of this feature.

So, from my point of view, we could just format output differently depending on an option given to Coverage.start. Those who cares about speed of their test suite may add a conditional statement like SimpleCov Readme suggests.

Looking forward to merging this feature into Ruby!

[dark-panda]

Any update's on this? Coverage reports are really misleading when conditional branches are excluded from reports and makes using single-line conditionals riskier as a result, even though they're considered in some cases to be idiomatic Ruby. This leads to a case where using the idiomatic syntax style results in riskier code since you can't tell from a coverage report if you've covered all of your code paths correctly. This effectively leads to having to make a decision over whether you want your code to be easier to cover but less readable or more readable but more difficult to cover.

[mame]

I'm VERY SORRY for whopping three-year delay... I'm now working to implement branch coverage and method coverage. I plan to finish and include it in Ruby 2.5.

This pull request is so inspiring, but there seem some issues. I show them below. Anyway, I really appreciate what you've done.
Based on your proposal, I'm now organizing my own idea. I will create another ticket for branch and method coverage in a few days.

Branch coverage of case statement

For example:

 1:def f(x)
 2:  case x
 3:  when :foo
 4:    p :foo
 5:  else
 6:    p :other
 7:  end
 8:end
 9:
10:# tests
11:f(:foo)
12:f(:bar)

When measuring the coverage of this code above by using your patch, the result is:

{"/tmp/ruby/t.rb"=>
  {:lines=>[1, 2, nil, 1, nil, 1, nil, nil, nil, nil, 1, 1],
   :methods=>{1=>4},
   :decisions=>{3=>[1, 0], 7=>[1, 0]}}}

There is 0 in 3=>[1, 0], which seems that "branch coverage" is not 100%. Actually, there is no way to change the 0 to 1. IMO, it is undesirable as a software metric to measure for understanding the degree of a test suite.

Branch coverage identifier

I'm afraid if identifying a branch by using only its lineno would be troublesome because there may be multiple branches in one line. Also, for convenience, I'd like the information to tell if from while. I'm thinking of identifying it by [<label>, <numbering-id>, <lineno>] for branch coverage, and [<method-name>, <lineno>] for method coverage, like:

{"/tmp/ruby/t.rb"=>
  {:lines=>[1, 2, nil, 1, nil, 1, nil, nil, nil, nil, 1, 1],
   :methods=>{[:f, 1]=>4},
   :branches=>{[:case, 0, 3]=>{[:when, 1, 4]=>1, [:else, 2, 6]=>1}}}

Branch coverage vs. decision coverage

This is just nitpiking, but I think that "branch coverage" is more correct name than "decision coverage." You attributed this document. This is slightly difficult to understand, but the original document that it cites says:

4.2.3 Branch Coverage Versus Decision Coverage (Issue 5).

snip This
is based partly on the fact that a Boolean expression evaluated outside of a flow control construct
may indeed be saved and used to control program flow later.

So, target of decision coverage is not only if and while statements, but also any Boolean expression such as:

cond = some? && complex? || condition?

In fact, your patch measures only if and while statements, i.e., branch coverage.

Decision coverage might be also useful, but to define it, we need to decide "what is a Boolean expression in Ruby". For now, I'd like to focus on only branch coverage.

New event types

As you may have noticed, adding many new event types is not a good idea. @ko1, the main developer of Ruby VM, dislikes it. And the magic number must be the power of 2 so that we can mask it by bit operation.

I'll work without adding new event types.

[magneland]

This PR is obsolete with Ruby 2.5 introducing this feature. Thanks very much for the effort, though! I learned a lot by reading this PR first.

[mame]

Yeah, I'm closing this. This PR was really helpful to serve as the fundamental API design of advanced coverage measurement. Thank you very much!