Implicit branches are not considered in coverage

[bencelang]

Describe the bug

Code with implicit branches is not detected in coverage. Consider the following code:

function fn(param) {
  if (param) {
	// If param is falsy, this if never runs
  }
}

If test code is only executing fn with a truthy parameter the implicit else branch is never tested, thus should not count towards covered branches. Interestingly enough, coverage still reports 100% for branch coverage.

This impacts both coverage providers but in case of c8 it is a known issue. With istanbul it is expected to report the correct 50% branch coverage metric, yet running under vitest the problem persists. Read reproduction for details.

Reproduction

Consider the following code (origin):

// cov.js
export const fn = y => {
  if (typeof y !== 'object') {
    y = 'yup'
  }
  console.log(y)
}

// cov.spec.js
import { fn } from "cov"

test("implicit branch coverage bug", () => {
  const x = 'asdf'
  fn(x)
});

The coverage report using @vitest/coverage-c8 reports this as 100% branch coverage, due to a known issue with v8 (bcoe/c8#227 and this v8 ticket). However, running a sligthly different version of this code from the gist I linked above in commonjs syntax through nyc node cov.cjs prints 50%, correctly. (ESM code causes nyc to have a blank output but that is also already a known issue).

Interestingly, switching the coverage provider to @vitest/coverage-istanbul is still impacted, showing full coverage mistakenly.

% Coverage report from istanbul
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |                  
 cov.js   |     100 |      100 |     100 |     100 |                  
----------|---------|----------|---------|---------|-------------------

System Info

system: Windows 11 Build 10.0.22621
node: v19.0.0
pnpm: v6.32.9
nyc: v15.1.0
vitest: v0.24.3

Used Package Manager

pnpm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[AriPerkkio]

This is duplicate of #1887.

@bencelang so even though there is no else branch, it is expected that the branch count includes it? It should not match with the source code's branch count?

// Source code
export const fn = y => {
    if (true) {
        console.log('#1')
    }
    console.log('#2')
}

// Instrumented
coverageMap.s[0]++;
export const fn = y => {
  coverageMap.f[0]++;
  coverageMap.s[1]++;

  if (true) {
    coverageMap.b[0][0]++;
    coverageMap.s[2]++;
    console.log('#1');
  } else {
    coverageMap.b[0][1]++;
  }

  coverageMap.s[3]++;
  console.log('#2');
}

// Coverage map
"s": {
  "0": 1,
  "1": 1,
  "2": 1,
  "3": 1
},
"f": {
  "0": 1
},
"b": {
  "0": [
    1,
    0 // <-- else-branch
  ]
}

Now that we map the coverage back to sources, the else branch is dropped since it is not found in sourcemaps - it does not exist in sources:

"s": {
  "0": 1,
  "1": 1,
  "2": 1,
  "3": 1
},
"f": {
  "0": 1
},
"b": { // Notice how branch count drops
  "0": [
    1
  ]
}

Two more test cases to demonstrate this.

100% branch coverage since all lines are covered

1 | export const fn = () => {
2 |     if (true) {
3 |         console.log('#1')
4 |     }
5 |     console.log('#2')
6 | }

/*
  % Coverage report from istanbul
 ----------|---------|----------|---------|---------|-------------------
 File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
 ----------|---------|----------|---------|---------|-------------------
 All files |     100 |      100 |     100 |     100 |                   
  cov.js   |     100 |      100 |     100 |     100 |                   
 ----------|---------|----------|---------|---------|-------------------
*/

75% coverage, unreached lines are not covered:

1 | export const fn = () => {
2 |     if (false) {
3 |         console.log('#1')
4 |     }
5 |     console.log('#2')
6 | }

/*
 % Coverage report from istanbul
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |      75 |        0 |     100 |      75 |                   
 cov.js   |      75 |        0 |     100 |      75 | 3                 
----------|---------|----------|---------|---------|-------------------
*/

[AriPerkkio]

I debugged this a bit further and found out why nyc is able to show these implicit else branches. Looks like a bug/missing feature in istanbul-lib-source-maps.

In vitest codebase we can apply a work-around for this. I'll set up PR later and write a bit more about the root cause.

 % Coverage report from istanbul
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |     100 |       50 |     100 |     100 |                   
 cov.js   |     100 |       50 |     100 |     100 | 2                 
----------|---------|----------|---------|---------|-------------------

[bencelang]

@AriPerkkio I see why you indicated this as a duplicate and I'm sorry for not catching it earlier. This example is the other way around though - always reaching the branch body - and I think better represents why this is misleading.

It should not match with the source code's branch count?

Depends on what you say branch count is. My point of view (as seem to align with the nyc command line's reasoning) is that a simple if statement convolves two possible ways of execution, whether you count the if statements or the real number of execution paths possible. In other words, every presence of a branch convolves at least two paths of execution. I would expect the branch coverage metric to help identify uncovered code paths due to branching, and not having covered a case where the above example condition is not met is definitely one of these areas. For full theoretical coverage, every branch should be tested for both possibilities: taking the branch or not. In some cases, this involves an explicit else statement but the mentioned code example has two paths of execution without it.

I think it makes more sense to count the actual paths of execution (a standalone if statement as 2) instead of the blocks present.

[AriPerkkio]

@bencelang you are completely right. I read this thread to understand the implicit else case better: gotwarlost/istanbul#35.

I think example below highlights the importance of branch coverage. It has 100% line coverage but only 50% branch coverage.

function get(condition) {
  let output;
  
  if(condition) {
    output = 'text';
  }
  
  return output;
}

// Test
expect(get(true)).toBe('text');

// Production code
get(usuallyTruthyCondition).toString()

Now the untested branch may actually cause NPE in production code. It's not covered by tests due to partial branch coverage.

const usuallyTruthyCondition = false;

get(usuallyTruthyCondition).toString()
                        // ^^^^^^^^^^^ NPE