It's possible to get 100% branch coverage without testing all flows through a branch

[mdlavin]

After adding a new condition to an if statement, I noticed that my code coverage did not decrease. It turns out that it's possible to get 100% coverage in all aspects without really getting good coverage.

Here is an example testcase:

function test(first, second, divisor) {
    if ((first < second) || (divisor !== 0)) {
        return 1/divisor;
    } else {
        return 2;
    }
}

test(1,2,1);
test(2,1,0);

When I run the code above through istanbul it says 100% coverage in every aspect, but the program has an interesting behavior when test(1,2,0) is executed.

It would be great if istanbul would report that not all paths through the if block had been properly covered. The case test(1,2,0) should also be required to get 100% coverage.

[gotwarlost]

So what you are looking for is really path coverage which IMO is outside istanbul scope.

Assume a more complex expressions like a || b || c || d || e || f

There are 64 possibilities with each condition being off or on. Did you want istanbul to track them all? How would you meaningfully represent the missing paths in a way the developer could understand the output?

Happy to keep this conversation alive and trade ideas. Just not ready to implement anything concrete yet :)

[mdlavin]

For some perspective, I work on a team that uses code coverage tools quite a bit and we recently moved from Java (using EclEmma for coverage) to more serious development on node.js. With EclEmma it is common to have an error like "1 of 4 branches missed" for an if statement. I noticed that istanbul didn't count coverage the same way and I figured I'd start a conversation about it.

You are right that it could get out of control with different conditions, but in my experience code like that is rare. I picture the 'or' operator to be like a shortened version of multiple if statements. If you were to write the code as multiple if statements, then istanbul would report the missing branch, so having it combined with the 'or' operator would ideally report the missing branch.

[silkentrance]

@mdlavin DivisionByZero should be captured by your test, e.g. expect.throws(...), so your tests are off. And this has not something to with istanbul at all.

[silkentrance]

@gotwarlost why not close this for good, I mean, DivisionByZero?.

[silkentrance]

@gotwarlost and these are not even tests, @mdlavin have you since evolved in your testing ability, at all?

[mdlavin]

@silkentrance I was viewing Istanbul as a tool that can sometimes help you understand what testcases were missed. In my example it would be good to add a testcase about divide by zero, and I was hoping that branch coverage metrics would point that sort of mistake out to the developer. This isn't really my code; it's just an example of how 100% branch coverage can be deceiving and an example of the impact it might have.

[silkentrance]

@mdlavin a division by zero causes your "test" program to fail early, with an exception, most notably the division by zero. How would istanbul reflect on this since the rest of your code is never run?

and yes, branch/line coverage can be deceiving, that is why I mentioned that these are no tests at all and that your code example lacks as it must prevent the division by zero from happening in the first place.

[mdlavin]

I'm sorry I'm so bad at explaining myself. I meant the divide by zero to be irrelevant to the example. That line could be replaced by anything. My intention was to highlight that branch coverage could be deceiving and it sounds like @gotwarlost understood what I was trying to point out.

I'm fine with this being closed; I understand that it could lead to more complexity but it would be a win ,in my eyes, if the tool could provide as many insights as possible into possible execution flows.