Function bodies don't count as branches?

[mhart]

The following file:

function a() {
  return 1
}
if (1) console.log('done')

Yields:

Branches     : 50% ( 1/2 )

And adding in a call to a():

function a() {
  return 1
}
a()
if (1) console.log('done')

Yields no change:

Branches     : 50% ( 1/2 )

So clearly the execution path of the function doesn't count as a branch.

However, if I add an if statement to the original:

function a() {
  return 1 ? 2 : 3
}
if (1) console.log('done')

Then suddenly I have two more branches (ie, the function body is now "counted"):

Branches     : 25% ( 1/4 )

Even though I only actually added one extra code path

Now I understand if you have a strict definition that a "branch" has to involve an if/else statement, then this perhaps makes sense.

But in a broader code coverage perspective, I would argue the original function a() should count as a single branch in an execution tree. One that would be covered by calling it - and not covered if you don't call it. And by introducing the ternary statement, you only added one more branch.

It seems inconsistent that adding only one extra path suddenly results in two extra "branches".

Would it be possible to have a setting that would allow code paths like this to be counted in the "branch" metric, even if they don't have an if/else statement?

We would love to just use the "branches" metric on its own when tracking our coverage as we believe it gives the best illustration of actual path coverage (given the limitations) - but this particular issue means that we have to somehow include the "functions" or "statements" metrics as well and come up with some sort of weighted average to reach a single figure.

[mhart]

I guess what I'm trying to say is probably summed up better by good old Wikipedia: "Branch coverage - ... has every edge in the program been executed?"

And I don't believe the current "branch" metric actually tests every edge

[gotwarlost]

I've been waiting for someone to tell me that my mechanism of counting branches is full of it :)

I looked for a clear spec to implement but couldn't find one - so I just brainstormed with a couple of folks and we made up something that works in the sense of being useful, relatively easy to understand and easy to report on (e.g. colorization in HTML reports) without paying too much attention to the theory.

To clarify, istanbul considers the following as branches:

• if/else statements
• switch/ case statements
• ternary operator ( ? : )
• subexpressions in a logical expression (a == b || c)

The reason your branch count went up is because the ternary counts as 2 branches and the if/else accounts for another 2. Every edge counts as 1 which is why the number went up by 2 (I think this is consistent with lcov).

If you actually look at the HTML report for the program, it will kind of make sense. Missing branches are colored only if some of them are taken (and not if all/ none of them are taken) etc.

Anyway, I need to think about how to best making changes while preserving backwards compatibility. It's not clear to me that a function that is not called is a missing branch...

[mhart]

Yeah - it definitely comes down to how you define "branch".

When you say "every edge counts as 1", you're absolutely correct, but you're discounting the original edge that exists before you introduce the if statement.

There's no debate that this function has one execution path:

function a() {
  return 1
}

And this one has two:

function a() {
  return 1 ? 2 : 3
}

(even though it'll only ever execute one... unless the rules of JS suddenly change 😸 )

As far as Istanbul is concerned, the first one has no "branch" and the second has "two branches". So by only introducing one more edge, we suddenly ended up with two new branches.

I'm still not sure this entirely makes sense...

Or if it does, it'd be great to have an option for it to count "edges" :-)

[mhart]

Or to put it another way... how many more edges on the right than the left?

[mhart]

Any updates on this?

[mhart]

Been a few years now – any updates on this?

[silkentrance]

@gotwarlost looking at the test results from https://github.com/raszi/node-tmp I can definitely tell that his has already been dealt with. Close?