Incorrect branch coverage when loaders used

[coderaiser]

c8 shows uncovered lines, but the whole file is covered.

Version: output of node -v 16.9.0
Platform: output of uname -a Darwin

• Version: latest
• Platform: mac os

Repository https://github.com/coderaiser/c8-reproduce

When I'm using loaders to mock imports the coverage I see is broken.

Code:

import {
    readFile,
} from 'fs/promises';

import {
    execSync,
} from 'child_process';

export default (a, b, c) => {
    if (a)
        return readFile();

      if (c)
          return execSync();

      return 'd';
};

Tests:

import {createMockImport} from 'mock-import';
import {
    test,
    stub,
} from 'supertape';

const {mockImport, reImport, stopAll} = createMockImport(import.meta.url);

test('changelog: a', async (t) => {
    mockImport('fs/promises', {
        readFile: stub().returns('a'),
    });
    const fn = await reImport('./changelog.js');
    stopAll();

    t.equal(fn.default(1), 'a');
});

test('changelog: c', async (t) => {
    mockImport('child_process', {
        execSync: stub().returns('c'),
    });

    const fn = await reImport('./changelog.js');
    stopAll();

    t.equal(fn.default(0, 0, 1), 'c');
});

test('changelog: d', async (t) => {
    const fn = await import('./changelog.js?count=4');

    t.equal(fn.default(0, 0, 0, 1), 'd');
});

What mock-import does is converts source to:

const {
    readFile: readFile
} = global.__mockImportCache.get('fs/promises');

import {
    execSync,
} from 'child_process';

export default (a, b, c) => {
    if (a)
        return readFile();
     
      if (c)
          return execSync();
      
      return 'd';
};

And imports it as ./changelog.js?count=1 on first test, then on second test:

import {
    readFile
} from 'fs/promises'

const {
    execSync,
} = global.__mockImportCache.get('fs/promises');

export default (a, b, c) => {
    if (a)
        return readFile();
     
      if (c)
          return execSync();
      
      return 'd';
};

File imported as ./changelog.js?count=2, and then on third assertion code isn't changed.

[bcoe]

@coderaiser thanks for the bug report. I'm not super familiar with the loader API, so not sure where this bug might be cropping up -- it could be as simple as coverage reports not being output by Node.js in one edge case (which would be relatively easy to fix).

If the problem is missing lines of coverage in v8 itself, this tends to be more difficult to fix.

If you beat me to the punch, and are excited to dig into this, where I would start looking would be in the raw v8 output:

cat coverage/tmp/coverage-661
coverage-66107-1631201558396-0.json  coverage-66110-1631201558266-0.json  

These files contain the raw output from v8 (which is written to disk by Node.js on exit):

{"result":[{"scriptId":"6","url":"internal/per_context/primordials.js","functions":[{"functionName":"copyProps","ranges":[{"startOffset":824,"endOffset":1077,"count":4},{"startOffset":898,"endOffset":1075,"count":13},{"startOffset":954,"endOffset":1071,"count":5}],"isBlockCoverage":true},{"functionName":"SafeIterator","ranges":[{"startOffset":2911,"endOffset":2982,"count":3}],"isBlockCoverage":true},{"functionName":"next","ranges":[{"startOffset":2987,"endOffset":3036,"count":374}],"isBlockCoverage":true},{"functionName":"makeSafe","ranges":[{"startOffset":3246,"endOffset":4378,"count":2},{"startOffset":3323,"endOffset":4170,"count":0}],"isBlockCoverage":true},{"functionName":"desc.value","ranges":[{"startOffset":4015,"endOffset":4082,"count":3}],"isBlockCoverage":true},{"functionName":"SafeMap","ranges":[{"startOffset":4717,"endOffset":4745,"count":16}],"isBlockCoverage":true},{"functionName":"SafeWeakMap","ranges":[{"startOffset":4888,"endOffset":4916,"count":7}]

☝️ my guess is that we'll see output that's slightly off startOffets/endOffset values for code using the loaders.

[coderaiser]

Just landed ability to generate sourcemap according to file url received from loader. Since mock-import does some changes, sourcemap should help to identify correct locations of code parts.

Generated file looks this way:

const hello = 'world';
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImhlbGxvLmpzIl0sIm5    hbWVzIjpbXSwibWFwcGluZ3MiOiJBQUFBLENBQUMsQ0FBQyxDQUFDLENBQUMsRUFBRSxDQUFDLENBQUMsQ0FBQyxDQUFDLEVBQUUsRUFBRSxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyx    DQUFDIiwiZmlsZSI6ImhlbGxvLm1hcCIsInNvdXJjZXNDb250ZW50IjpbImNvbnN0IGhlbGxvID0gJ3dvcmxkJzsiXX0=

And it works good in the browser. The question is: does c8 supports instrumented code?
I see that there is an option --exclude-after-remap but I don't see any changes in coverage information.

Could you please help me to get source map working?

[coderaiser]

Looks like istrumenting with babel-plugin-istanbul helped to solve the problem on a half! No it works full coverage all the time 😄 .

Happy holidays 🎉 !

[bcoe]

@coderaiser If I understand correctly, you're using babel-plugin-istanbul rather than c8, sounds like it's working for you and I can close this?

[coderaiser]

@bcoe Unfortunately it shows full coverage every time, so it’s useless. Could you please tell me is it possible to make c8 read source maps?

Source file is changed inside of loader, and it has different name(with ?mock-import=n) to have ability to import file again with mocked import. It works very good, but sometimes (like in this example) shoes bad lines and columns positions, and shows that some code is uncovered, but it is covered.

[coderaiser]

Looks like it can be related to _normalizeMapCache and _normalizeProcessCov in /lib/report.js. Both uses fileURLToPath which is cut down ?mock-import-count=1 suffixes.

But they are needed to import file again and transform it's content with mocked modules. Is it possible to cut them after we merged reports related to all files with prefix?

[coderaiser]

Looks like this is 1:many case from v8-to-instanbul function named load:

• ✅ 1 source code;
• ✅ many source maps;

We should merge all changes that we find using different source maps (for each test when import is mocked).

[bcoe]

Hey @coderaiser, can you figure out a minimal failing test case? that demonstrates how truncating the query string causes this bug?

I think we could probably figure out an approach to keep the suffix longer, until the source map lookup occurs, if that's the root of the problem (I think I'm not yet fully understanding, what do the entries for sourcemaps look like In the cache, they include the query string?)

[coderaiser]

Hey @bcoe, OK, right now I have 3 tests.

they include the query string?

Sourcemap doesn't includes query string, it includes path of a file on disk.

When readFile mocked:

const {
    readFile: readFile
} = global.__mockImportCache.get('fs/promises');

import {
    execSync,
} from 'child_process';

export default (a, b, c) => {
    if (a)
        return readFile();

      if (c)
          return execSync();

      return 'd';
};


//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImZpbGU6Ly8vVXNlcnMvY29kZXJhaXNlci9jOC1yZXByb2R1Y2UvbGliL2NoYW5nZWxvZy5qcyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiOztpQ0FFTyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQzs7QUFFbkIsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLEVBQUU7SUFDSCxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUM7QUFDWixFQUFFLENBQUMsQ0FBQyxDQUFDLEVBQUUsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUM7O0FBRXRCLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxFQUFFLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLEVBQUUsQ0FBQyxDQUFDLEVBQUUsQ0FBQyxFQUFFLENBQUMsRUFBRSxDQUFDLEVBQUU7SUFDeEIsQ0FBQyxFQUFFLENBQUMsQ0FBQztRQUNELENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxFQUFFLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUM7O01BRW5CLENBQUMsRUFBRSxDQUFDLENBQUM7VUFDRCxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsRUFBRSxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDOztNQUVyQixDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsRUFBRSxDQUFDLENBQUMsQ0FBQztBQUNoQixDQUFDIiwiZmlsZSI6ImZpbGU6Ly8vVXNlcnMvY29kZXJhaXNlci9jOC1yZXByb2R1Y2UvbGliL2NoYW5nZWxvZy5qcy5tYXAiLCJzb3VyY2VzQ29udGVudCI6WyJpbXBvcnQge1xuICAgIHJlYWRGaWxlLFxufSBmcm9tICdmcy9wcm9taXNlcyc7XG5cbmltcG9ydCB7XG4gICAgZXhlY1N5bmMsXG59IGZyb20gJ2NoaWxkX3Byb2Nlc3MnO1xuXG5leHBvcnQgZGVmYXVsdCAoYSwgYiwgYykgPT4ge1xuICAgIGlmIChhKVxuICAgICAgICByZXR1cm4gcmVhZEZpbGUoKTtcbiAgICAgXG4gICAgICBpZiAoYylcbiAgICAgICAgICByZXR1cm4gZXhlY1N5bmMoKTtcbiAgICAgIFxuICAgICAgcmV0dXJuICdkJztcbn07XG5cbiJdfQ==

Here is version Base64 encoded:

{
  "version": 3,
  "sources": [
    "file:///Users/coderaiser/c8-reproduce/lib/changelog.js"
  ],
  "names": [],
  "mappings": ";;iCAEO,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC;;AAEnB,CAAC,CAAC,CAAC,CAAC,CAAC,EAAE;IACH,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC;AACZ,EAAE,CAAC,CAAC,CAAC,EAAE,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC;;AAEtB,CAAC,CAAC,CAAC,CAAC,CAAC,EAAE,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,EAAE,CAAC,CAAC,EAAE,CAAC,EAAE,CAAC,EAAE,CAAC,EAAE;IACxB,CAAC,EAAE,CAAC,CAAC;QACD,CAAC,CAAC,CAAC,CAAC,CAAC,EAAE,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC;;MAEnB,CAAC,EAAE,CAAC,CAAC;UACD,CAAC,CAAC,CAAC,CAAC,CAAC,EAAE,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC;;MAErB,CAAC,CAAC,CAAC,CAAC,CAAC,EAAE,CAAC,CAAC,CAAC;AAChB,CAAC",
  "file": "file:///Users/coderaiser/c8-reproduce/lib/changelog.js.map",
  "sourcesContent": [
    "import {\\n    readFile,\\n} from 'fs/promises';\\n\\nimport {\\n    execSync,\\n} from 'child_process';\\n\\nexport default (a, b, c) => {\\n    if (a)\\n        return readFile();\\n     \\n      if (c)\\n          return execSync();\\n      \\n      return 'd';\\n};\\n\\n"
  ]
}

When execSync mocked:

import {
    readFile,
} from 'fs/promises';

const {
    execSync: execSync
} = global.__mockImportCache.get('child_process');

export default (a, b, c) => {
    if (a)
        return readFile();

      if (c)
          return execSync();

      return 'd';
};


//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImZpbGU6Ly8vVXNlcnMvY29kZXJhaXNlci9jOC1yZXByb2R1Y2UvbGliL2NoYW5nZWxvZy5qcyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQSxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsRUFBRTtJQUNILENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQztBQUNaLEVBQUUsQ0FBQyxDQUFDLENBQUMsRUFBRSxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDOzs7O2lDQUliLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQzs7QUFFckIsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLEVBQUUsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsRUFBRSxDQUFDLENBQUMsRUFBRSxDQUFDLEVBQUUsQ0FBQyxFQUFFLENBQUMsRUFBRTtJQUN4QixDQUFDLEVBQUUsQ0FBQyxDQUFDO1FBQ0QsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLEVBQUUsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQzs7TUFFbkIsQ0FBQyxFQUFFLENBQUMsQ0FBQztVQUNELENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxFQUFFLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUM7O01BRXJCLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxFQUFFLENBQUMsQ0FBQyxDQUFDO0FBQ2hCLENBQUMiLCJmaWxlIjoiZmlsZTovLy9Vc2Vycy9jb2RlcmFpc2VyL2M4LXJlcHJvZHVjZS9saWIvY2hhbmdlbG9nLmpzLm1hcCIsInNvdXJjZXNDb250ZW50IjpbImltcG9ydCB7XG4gICAgcmVhZEZpbGUsXG59IGZyb20gJ2ZzL3Byb21pc2VzJztcblxuaW1wb3J0IHtcbiAgICBleGVjU3luYyxcbn0gZnJvbSAnY2hpbGRfcHJvY2Vzcyc7XG5cbmV4cG9ydCBkZWZhdWx0IChhLCBiLCBjKSA9PiB7XG4gICAgaWYgKGEpXG4gICAgICAgIHJldHVybiByZWFkRmlsZSgpO1xuICAgICBcbiAgICAgIGlmIChjKVxuICAgICAgICAgIHJldHVybiBleGVjU3luYygpO1xuICAgICAgXG4gICAgICByZXR1cm4gJ2QnO1xufTtcblxuIl19

When nothing mocked:

import {
    readFile,
} from 'fs/promises';

import {
    execSync,
} from 'child_process';

export default (a, b, c) => {
    if (a)
        return readFile();

      if (c)
          return execSync();

      return 'd';
};


//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImZpbGU6Ly8vVXNlcnMvY29kZXJhaXNlci9jOC1yZXByb2R1Y2UvbGliL2NoYW5nZWxvZy5qcyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQSxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsRUFBRTtJQUNILENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQztBQUNaLEVBQUUsQ0FBQyxDQUFDLENBQUMsRUFBRSxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDOztBQUVwQixDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsRUFBRTtJQUNILENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQztBQUNaLEVBQUUsQ0FBQyxDQUFDLENBQUMsRUFBRSxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQzs7QUFFdEIsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLEVBQUUsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsRUFBRSxDQUFDLENBQUMsRUFBRSxDQUFDLEVBQUUsQ0FBQyxFQUFFLENBQUMsRUFBRTtJQUN4QixDQUFDLEVBQUUsQ0FBQyxDQUFDO1FBQ0QsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLEVBQUUsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQzs7TUFFbkIsQ0FBQyxFQUFFLENBQUMsQ0FBQztVQUNELENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxFQUFFLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxDQUFDLENBQUM7O01BRXJCLENBQUMsQ0FBQyxDQUFDLENBQUMsQ0FBQyxFQUFFLENBQUMsQ0FBQyxDQUFDO0FBQ2hCLENBQUMiLCJmaWxlIjoiZmlsZTovLy9Vc2Vycy9jb2RlcmFpc2VyL2M4LXJlcHJvZHVjZS9saWIvY2hhbmdlbG9nLmpzLm1hcCIsInNvdXJjZXNDb250ZW50IjpbImltcG9ydCB7XG4gICAgcmVhZEZpbGUsXG59IGZyb20gJ2ZzL3Byb21pc2VzJztcblxuaW1wb3J0IHtcbiAgICBleGVjU3luYyxcbn0gZnJvbSAnY2hpbGRfcHJvY2Vzcyc7XG5cbmV4cG9ydCBkZWZhdWx0IChhLCBiLCBjKSA9PiB7XG4gICAgaWYgKGEpXG4gICAgICAgIHJldHVybiByZWFkRmlsZSgpO1xuICAgICBcbiAgICAgIGlmIChjKVxuICAgICAgICAgIHJldHVybiBleGVjU3luYygpO1xuICAgICAgXG4gICAgICByZXR1cm4gJ2QnO1xufTtcblxuIl19

Hey @coderaiser, can you figure out a minimal failing test case? that demonstrates how truncating the query string causes this bug?

Here is repository and test, to get things working run:

git clone https://github.com/coderaiser/c8-reproduce
cd c8-reproduce
npm i
npm run coverage

You can use npm run debug for debugging.

And you will see that coverage information is not full:

 changelog.js |     100 |       20 |     100 |     100 | 11-14

Here is PR with partial support of sourcemaps, it keeps file URLS as they are, but the problem still exist in https://github.com/bcoe/c8/blob/v7.11.0/lib/report.js#L116, as it works with v8 coverage offsets which are different, I saw that v8-to-instanbul parses this differences according to sourcemaps but I'm not quit understand how it works. It returns Infinity for line and column in some cases.

#345.

[coderaiser]

Here is offsets I receive in v8ProcessCov:

{
    "result": [
        {
            "scriptId": "0",
            "url": "file:///Users/coderaiser/c8-reproduce/lib/changelog.js?mock-import-count=1",
            "functions": [
                {
                    "functionName": "",
                    "ranges": [
                        {
                            "startOffset": 0,
                            "endOffset": 1824,
                            "count": 1
                        }
                    ],
                    "isBlockCoverage": true
                },
                {
                    "functionName": "default",
                    "ranges": [
                        {
                            "startOffset": 144,
                            "endOffset": 271,
                            "count": 1
                        },
                        {
                            "startOffset": 196,
                            "endOffset": 270,
                            "count": 0
                        }
                    ],
                    "isBlockCoverage": true
                }
            ]
        },
        {
            "scriptId": "1",
            "url": "file:///Users/coderaiser/c8-reproduce/lib/changelog.js?mock-import-count=2",
            "functions": [
                {
                    "functionName": "",
                    "ranges": [
                        {
                            "startOffset": 0,
                            "endOffset": 1820,
                            "count": 1
                        }
                    ],
                    "isBlockCoverage": true
                },
                {
                    "functionName": "default",
                    "ranges": [
                        {
                            "startOffset": 144,
                            "endOffset": 271,
                            "count": 1
                        },
                        {
                            "startOffset": 178,
                            "endOffset": 196,
                            "count": 0
                        },
                        {
                            "startOffset": 244,
                            "endOffset": 270,
                            "count": 0
                        }
                    ],
                    "isBlockCoverage": true
                }
            ]
        },
        {
            "scriptId": "2",
            "url": "file:///Users/coderaiser/c8-reproduce/lib/changelog.js?mock-import-count=3",
            "functions": [
                {
                    "functionName": "",
                    "ranges": [
                        {
                            "startOffset": 0,
                            "endOffset": 1929,
                            "count": 1
                        }
                    ],
                    "isBlockCoverage": true
                },
                {
                    "functionName": "default",
                    "ranges": [
                        {
                            "startOffset": 109,
                            "endOffset": 236,
                            "count": 1
                        },
                        {
                            "startOffset": 143,
                            "endOffset": 161,
                            "count": 0
                        },
                        {
                            "startOffset": 191,
                            "endOffset": 209,
                            "count": 0
                        }
                    ],
                    "isBlockCoverage": true
                }
            ]
        }]
}

And they are passed to applyCoverage from v8-to-instanbul.