c8 incorrect branch coverage; modules and import globals

[digitalBush]

I'm using node's built-in test runner with c8 to report coverage. When mocking a dependency, that whole file ends up being reported as covered. This is with node v20.9.0

Consider a test like this:

import {test} from "node:test";
import {strict as esmock} from "esmock";

test("coverage", async ()=>{
    const {default: target} = await esmock("./target.js",{
        "./dep.js":{default:()=>console.log("mocked")}
    });
    target();
})

Running that test with npx c8 node --test will report the following coverage:

❯ npx c8 node --test
mocked
✔ coverage (66.962498ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 221.23211
-----------|---------|----------|---------|---------|-------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-----------|---------|----------|---------|---------|-------------------
All files  |     100 |      100 |     100 |     100 |                   
 dep.js    |     100 |      100 |     100 |     100 |                   
 target.js |     100 |      100 |     100 |     100 |                   
-----------|---------|----------|---------|---------|-------------------

The other files involved look like this:
target.js

import dep from "./dep.js"

export default ()=> {
    dep();
}

dep.js

export default ()=>{
    console.log("NOT mocked")
}

I didn't expect dep.js to show any coverage. I have tried the built-in mocking with tap and also vitest and didn't see this issue even though they use c8 for coverage also.

[koshic]

It's a c8 issue - all incoming urls transformed into paths fileURLToPath:
https://github.com/bcoe/c8/blob/a13584d5be5259ebb6a00455d352c3e8b16006de/lib/report.js#L364

As a result, 'dep.js?mocked-by-esmock' transformed into 'dep.js' which leads to parasitic coverage for real 'dep.js.

Because c8 has no options to override _normalizeProcessCov logic, there is only one way: fix coverage .json files (~50 lines of code) before calling c8 by using additional wrapper between c8 and 'node --test'. Or, you can use Node's API to execute tests, and insert that fix before final call to c8 reporter (I've implemented this in my projects).

PS sure, it's possible to change esmock logic and use synthetic file urls for mocks, but I believe that c8 should take into account file url instead of path.

[iambumblehead]

I did not understand and now I do understand; that's a great explanation

[digitalBush]

@koshic Thank you for explaining the issue. That helped me fit the pieces together.
If I dig around in the coverage files from the sample above, I see an object like this:

{
  "scriptId":"171",
  "url":"file:///dev/play/c8-esmock-issue/dep.js?esmkTreeId=1&esmkModuleId=./dep.js&isfound=true&isesm=false&exportNames=default",
  "functions":[{"functionName":"","ranges":[{"startOffset":0,"endOffset":179,"count":1}],"isBlockCoverage":true}]
}

Removing that produces the expected coverage.

PS sure, it's possible to change esmock logic and use synthetic file urls for mocks, but I believe that c8 should take into account file url instead of path.

Looking at this from c8's perspective, I'm not sure how else to interpret file.js?querystring other than to strip the query string. 🤷‍♂️

[koshic]

Looking at this from c8's perspective, I'm not sure how else to interpret file.js?querystring other than to strip the query string.

At first, let's talk about file urls. From Node perspective (in ESM world), 'file.js' / 'file.js?a' / 'file.js?query#hash' are different modules: they loaded separately, they have dedicated entries in module cache, etc. And what is more important - they may have different sources. This is why stripping query string / hash is a wrong (ok, not wrong - but not usiversal) solution.

To map coverage information to the particular file on the disk, we need an additional information. Obvious solutions are to use user-supplied callback or regexp(s) in c8 config file. More complex thing - use sourcesContent from Source Map v3 spec (if available), and compare it to the file contents: 'want to use hook and query strings? ok, but please provide proper source maps'.

[digitalBush]

Are they different modules or just different instances of the same module? From node ESM docs it says:

Modules are loaded multiple times if the import specifier used to resolve them has a different query or fragment.

Today I learned that I can do something like this import dep from "./dep.js?lol=true" 😂

I'm just now treading into ESM territory and I'm not super familiar with source maps, so I'm out of my depth. I appreciate your explanations here!

[koshic]

Are they different modules or just different instances of the same module?

Yeah, different spec == different module.

[iambumblehead]

related link bcoe/c8#325

[shnhrrsn]

I was running into this with c8+ava as well, based on the info in this issue I threw together a rough script to scrub the v8 reports:

import { execFileSync } from 'node:child_process'
import { readdir, readFile, unlink, writeFile } from 'node:fs/promises'
import path from 'node:path'
import { fileURLToPath } from 'node:url'

const c8 = new URL('./bin/c8.js', import.meta.resolve('c8'))
const ava = new URL('./cli.mjs', import.meta.resolve('ava'))

// Run c8+ava
execFileSync(fileURLToPath(c8), ['--reporter=json', fileURLToPath(ava), ...process.argv.slice(2)], {
    stdio: 'inherit',
})
console.log('')

// Scrub files containing esmkModuleId from v8 coverage
const coverage = path.join(process.cwd(), './.c8/coverage')
const tmpdir = path.join(coverage, './tmp/')
await unlink(path.join(coverage, './coverage-final.json'))

for (const filename of await readdir(tmpdir)) {
    const pathname = path.join(tmpdir, filename)
    const json = JSON.parse(await readFile(pathname, 'utf8'))
    json.result = /** @type {{ url: string }[]} */ (json.result).filter(
        ({ url }) => !url.includes('esmkModuleId'),
    )
    await writeFile(pathname, JSON.stringify(json))
}

// Run c8 report
execFileSync(fileURLToPath(c8), ['report'], {
    stdio: 'inherit',
})

@iambumblehead does this seem right to you?

[iambumblehead]

@shnhrrsn I think it does seem right -- thanks for messaging and sharing

There is also a smaller query key used with the target module https://github.com/iambumblehead/esmock/blob/main/src/esmockModule.js#L162C4-L162C43 I don't know if these affect c8 results and haven't investigated deeply myself but am sharing this in case it is helpful

//  file:///path/to/module.js?esmk=1
return moduleFileURL + `?esmk=${treeid}`

[koshic]

I was running into this with c8+ava as well

I suggest you to simplify that script a bit.

Coverage collected by Node, so you only need to set environment variable before ava spawn - c8 doesn't required:

const tempDirectory = await mkdtemp(join(tmpdir(), "node-coverage-"));
process.env["NODE_V8_COVERAGE"] = tempDirectory;

Next, spawn c8 with --temp-directory option or just use it programmatically:

import { Report } from "c8";

await new Report({
  tempDirectory,
  // any other options
}).run();

[iambumblehead]

related #296