Branch merging broken when multiple files source-map to same origin file

[isaacs]

Demonstrated here: https://github.com/isaacs/c8-merge-issue/

The source, clearly showing that the branches get covered:

console.error('start of module')
const branchSwitch = process.env.BRANCH_SWITCH
export const x = () => {
  console.error('start of function')
  let x: number = 0
  if (branchSwitch !== undefined) {
    console.error('branch 1')
    x = Number(branchSwitch)
  } else {
    console.error('branch 2')
  }
  console.error('end of function')
  return x
}
console.error('end of module')

The test, covering all branches in the origin by loading the file from two different modules that sourcemap to the same origin:

import assert from 'node:assert'

delete process.env.BRANCH_SWITCH
const { x: first } = await import('./dist/esm/index.js')

process.env.BRANCH_SWITCH = '420'
const { default: { x: second } } = await import('./dist/commonjs/index.js')

assert.equal(first(), 0)
assert.equal(second(), 420)

Result:

$ npm test

> pretest
> npm run prepare


> prepare
> tshy


> test
> c8 node test.js

start of module
end of module
start of module
end of module
start of function
branch 2
end of function
start of function
branch 1
end of function
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |     100 |    33.33 |     100 |     100 |                   
 index.ts |     100 |    33.33 |     100 |     100 | 6-9               
----------|---------|----------|---------|---------|-------------------

It's clear that all branches in the origin are being covered, but when v8-to-istanbul merges them together, it reports missing branches.

The HTML report looks like this:

Explicitly ignoring the { in the first branch and the else { in the second makes it pass, but this should not be necessary.

diff --git a/src/index.ts b/src/index.ts
index 6ecae9b..54fdddb 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -3,10 +3,16 @@ const branchSwitch = process.env.BRANCH_SWITCH
 export const x = () => {
   console.error('start of function')
   let x: number = 0
-  if (branchSwitch !== undefined) {
+  if (branchSwitch !== undefined)
+    /* c8 ignore start */
+  {
+    /* c8 ignore stop */
     console.error('branch 1')
     x = Number(branchSwitch)
-  } else {
+  }
+    /* c8 ignore start */
+  else {
+    /* c8 ignore stop */
     console.error('branch 2')
   }
   console.error('end of function')

[SimenB]

Do you know if the underlying v8 coverage says it's covered?

[isaacs]

When I split out the two covered files and remove the SourceMappingURL comments, I see this for the commonjs file:

And this for the ESM:

When they're source mapped and merged together, the statements are being properly recorded, but instead of merging the covered branches, it's merging the uncovered branches or something? Note the { in the first branch remains uncovered, despite being covered in the CJS build, and the else { remains uncovered, despite being covered in the ESM build.

The relevant blocks from V8's coverage output:

{
  "result": [
    {
      "scriptId": "134",
      "url": "file:///Users/isaacs/dev/isaacs/c8-merge-issue/src/esm.index.js",
      "functions": [
        {
          "functionName": "",
          "ranges": [{ "startOffset": 0, "endOffset": 447, "count": 1 }],
          "isBlockCoverage": true
        },
        {
          "functionName": "x",
          "ranges": [
            { "startOffset": 99, "endOffset": 380, "count": 1 },
            { "startOffset": 198, "endOffset": 274, "count": 0 }
          ],
          "isBlockCoverage": true
        }
      ]
    },
    {
      "scriptId": "138",
      "url": "file:///Users/isaacs/dev/isaacs/c8-merge-issue/src/commonjs.index.js",
      "functions": [
        {
          "functionName": "",
          "ranges": [{ "startOffset": 0, "endOffset": 552, "count": 1 }],
          "isBlockCoverage": true
        },
        {
          "functionName": "x",
          "ranges": [
            { "startOffset": 189, "endOffset": 470, "count": 1 },
            { "startOffset": 364, "endOffset": 416, "count": 0 }
          ],
          "isBlockCoverage": true
        }
      ]
    }
  ]
}

[isaacs]

So, according to v8, these are the uncovered blocks:

> fs.readFileSync('src/esm.index.js').subarray(198,274).toString()
"{\n        console.error('branch 1');\n        x = Number(branchSwitch);\n    }"
> fs.readFileSync('src/commonjs.index.js').subarray(364,416).toString()
"\n    else {\n        console.error('branch 2');\n    }"

Looking those positions up in the source map, they correspond to:

cjs: ' else {\n    console.error('branch 2')\n  }'
esm: '{\n    console.error('branch 1')\n    x = Number(branchSwitch)\n  }'

So, according to v8 and the source maps, the else { is uncovered by cjs, but is covered by the esm script, and the { of the first branch is not covered by esm, but is covered by cjs.

[isaacs]

Ok, digging through this, I think the issue might actually not be v8-to-istanbul, but rather istanbul-lib-coverage.

In the mergeProp function in lib/file-coverage.js, when merging branch coverage, I see this:

mergeProp [
  ####   first pass, hit the entire function, but un-hit the else {} branch
  { '0': [ 1 ], '1': [ 0 ] },
  {
    '0': { ### the function
      type: 'branch',
      line: 3,
      loc: { start: { line: 3, column: 17 }, end: { line: 14, column: 1 } },
      locations: [
        {
          start: { line: 3, column: 17 },
          end: { line: 14, column: 1 }
        }
      ]
    },
    '1': { ### the else {}
      type: 'branch',
      line: 9,
      loc: { start: { line: 9, column: 3 }, end: { line: 11, column: 3 } },
      locations: [
        { start: { line: 9, column: 3 }, end: { line: 11, column: 3 } }
      ]
    }
  },

  #### second pass, hit the entire function, but un-hit the if {} branch
  { '0': [ 1 ], '1': [ 0 ] },
  {
    '0': { #### the function
      type: 'branch',
      line: 3,
      loc: { start: { line: 3, column: 17 }, end: { line: 14, column: 1 } },
      locations: [
        {
          start: { line: 3, column: 17 },
          end: { line: 14, column: 1 }
        }
      ]
    },
    '1': { #### the if {}
      type: 'branch',
      line: 6,
      loc: { start: { line: 6, column: 34 }, end: { line: 9, column: 9 } },
      locations: [
        { start: { line: 6, column: 34 }, end: { line: 9, column: 9 } }
      ]
    }
  }
]
merged [
  { '0': [ 2 ], '1': [ 0 ], '2': [ 0 ] },
  {
    '0': { #### hit the function
      type: 'branch',
      line: 3,
      loc: { start: { line: 3, column: 17 }, end: { line: 14, column: 1 } },
      locations: [
        {
          start: { line: 3, column: 17 },
          end: { line: 14, column: 1 }
        }
      ]
    },
    '1': { #### un-hit the else
      type: 'branch',
      line: 9,
      loc: { start: { line: 9, column: 3 }, end: { line: 11, column: 3 } },
      locations: [
        { start: { line: 9, column: 3 }, end: { line: 11, column: 3 } }
      ]
    },
    '2': { #### unhit the if {}
      type: 'branch',
      line: 6,
      loc: { start: { line: 6, column: 34 }, end: { line: 9, column: 9 } },
      locations: [
        { start: { line: 6, column: 34 }, end: { line: 9, column: 9 } }
      ]
    }
  }
]

It seems like, when merging, it needs to determine if a range in one of the sets is fully contained by a range in the other. If so, add 1 to the hit count, rather than recording it as a new range with the original hit count.

Ie, the expected hit-count in the result should be { '0': [ 2 ], '1': [ 1 ], '2': [ 1 ] }, because the un-hit range is contained by a range that has a hit count of 1 in the other set, and it doesn't identify that range specifically.

[isaacs]

@SimenB istanbuljs/istanbuljs#750

[SimenB]

https://github.com/istanbuljs/istanbuljs/releases/tag/istanbul-lib-coverage-v3.2.1