cenfun / monocart-coverage-reports

A code coverage tool to generate native V8 reports or Istanbul reports.
MIT License
69 stars 6 forks source link

[Bug] The `sourceFilter` option seems to not be honored #22

Closed ericmorand closed 5 months ago

ericmorand commented 5 months ago

Describe the bug

The sourceFilter option seems to not be honored - if I understand well its meaning that is "the pattern used to identify the files that will be part of the coverage report".

To Reproduce

I created a repository to easily test and reproduce the issue: https://github.com/ericmorand/c8-with-bundle

Just execute npm mcr and wait for the report to be displayed: it includes every file that is referenced in the V8 coverage data (that is node_modules files), plus src/main/lib/foo.ts but excluding all the other files present in src/main that also matches the passed sourceFilter pattern src/main/**/*.ts (like src/main/lib/bar.ts).

Expected behavior

The report only contains all the files that match the passed sourceFilter pattern. In this case, all the files matching src/main/**/*.ts

cenfun commented 5 months ago

I can't pass the npm run build:main, could you please help to check it:

> build:main
> node build.mjs src/main/index.ts dist --treeshake

\c8-with-bundle\node_modules\typescript\lib\typescript.js:113963
    Debug.assert(contains(commandLine.fileNames, inputFileName), `Expected fileName to be present in command line`);
          ^

Error: Debug Failure. False expression: Expected fileName to be present in command line
    at Object.getOutputFileNames (\c8-with-bundle\node_modules\typescript\lib\typescript.js:113963:11)
ericmorand commented 5 months ago

Sorry, you need to build the test suite with npm run build:test. I assmue my main build script is flawed but we dont need it here. :)

cenfun commented 5 months ago

same errors

> build:test
> node build.mjs src/test/index.ts dist --source-map

\c8-with-bundle\node_modules\typescript\lib\typescript.js:113963
    Debug.assert(contains(commandLine.fileNames, inputFileName), `Expected fileName to be present in command line`);
          ^

Error: Debug Failure. False expression: Expected fileName to be present in command line

Im using Node.js v20, Windows 10

ericmorand commented 5 months ago

Very interesting. Windows 10 being the difference between our setups, I assume my build tool doesn't support it properly. Let me fix this.

cenfun commented 5 months ago

So back to your issue. I think your sourceFilter pattern should be **/src/**/*.ts, it used the tool minimatch see https://github.com/isaacs/minimatch

ericmorand commented 5 months ago

It doesn't remove node_modules from the coverage but adds src/test/index.ts, which seems to demonstrate that the pattern is honored.

By the way, I pushed a new version of the project that comes with the dist folder already generated. You can now execute npm run mcr, without having to build the test suite before.

cenfun commented 5 months ago

Thanks, I got it. The entry file is dist/index.mjs not src/**, so we need both entryFilter and sourceFilter and It's recommended to use a config file mcr.config.js

// mcr.config.js
module.exports = {
    // logging: "debug",
    name: 'My Coverage Report',
    reports: [
        'v8',
        'console-details'
    ],
    outputDir: './coverage-reports',
    entryFilter: {
        "**/node_modules/**": false,
        "**/dist/**": true
    },
    sourceFilter: {
        "**/src/test/**": false,
        "**/src/**": true
    }
};

Then run npx mcr npm t again. Let me know if it works.

[MCR] My Coverage Report
┌──────────────┬─────────┬────────────┬──────────┬───────────┬─────────┬─────────────────┐
│ Name         │   Bytes │ Statements │ Branches │ Functions │   Lines │ Uncovered Lines │
├──────────────┼─────────┼────────────┼──────────┼───────────┼─────────┼─────────────────┤
│ src/main/lib │         │            │          │           │         │                 │
│ └ foo.ts     │ 74.38 % │    80.00 % │          │   50.00 % │ 57.14 % │ 5-7             │
├──────────────┼─────────┼────────────┼──────────┼───────────┼─────────┼─────────────────┤
│ Summary      │ 74.38 % │    80.00 % │          │   50.00 % │ 57.14 % │                 │
└──────────────┴─────────┴────────────┴──────────┴───────────┴─────────┴─────────────────┘
cenfun commented 5 months ago

BTW, the --sourceFilter dose not support multiple patterns for now, and I will fix it in next version:

--entryFilter "{'**/node_modules/**': false, '**/src/*.js': true}"
ericmorand commented 5 months ago

I'm not sure I understand the point of having an entry filter, but I probably misunderstand the meaning of the source filter. My asumption was that it filtered the files that are considered as "sources" - that is, the files that we want to get the coverage. With this asumption, there is no need for an entry filter because none of the files that are hit during the execution of the command should be emitted in the report, except if they have been added explicitely to the source filter.

Typically, in my case, using sourceFilter "src/main/*/.ts", I would only get those files in the report, regardless of the files that were hit when executing command. Would I want node_modules files to be included in the report, I would just have to add them to the sourceFilter pattern.

Also, another question: how do I include every files matched by sourceFilter into the report - typically here src/main/index.ts.

cenfun commented 5 months ago

1, Due to the characteristics of v8 coverage data, we need to use two filters entryFilter and sourceFilter see here for more infomation 2, src/main/index.ts should be a untest file, which is not included in the bundle and coverage data, we can try all option see here

module.exports = {
     logging: "debug",
    name: 'My Coverage Report',
    reports: [
        'v8',
        'console-details'
    ],
    outputDir: './coverage-reports',
    entryFilter: {
        "**/node_modules/**": false,
        "**/src/test/**": false,
        "**/dist/**": true,
        "**/src/**": true
    },
    sourceFilter: {
        "**/src/test/**": false,
        "**/src/**": true
    },
    all: {
        dir: ['./src']
    }
};

and report

[MCR] My Coverage Report
┌─────────────────┬─────────┬────────────┬──────────┬───────────┬─────────┬─────────────────┐
│ Name            │   Bytes │ Statements │ Branches │ Functions │   Lines │ Uncovered Lines │
├─────────────────┼─────────┼────────────┼──────────┼───────────┼─────────┼─────────────────┤
│ src/main        │         │            │          │           │         │                 │
│ ├ index.ts      │  0.00 % │     0.00 % │          │           │  0.00 % │ 1-2             │
│ ├ tsconfig.json │  0.00 % │            │          │           │  0.00 % │ 1-3             │
│ └ lib           │         │            │          │           │         │                 │
│   ├ bar.ts      │  0.00 % │     0.00 % │          │    0.00 % │  0.00 % │ 1-3             │
│   ├ empty.ts    │         │            │          │           │         │                 │
│   └ foo.ts      │ 74.38 % │    80.00 % │          │   50.00 % │ 57.14 % │ 5-7             │
├─────────────────┼─────────┼────────────┼──────────┼───────────┼─────────┼─────────────────┤
│ Summary         │ 21.95 % │    23.53 % │          │   20.00 % │ 18.18 % │                 │
└─────────────────┴─────────┴────────────┴──────────┴───────────┴─────────┴─────────────────┘
ericmorand commented 5 months ago

Oh, I didn't see the all option in the help of the CLI, I assumed it didn't exist. It works when using a configuration file. Thanks.

cenfun commented 5 months ago

Yes, the CLI arguments only can pass the string and number, so some of the options not supported by CLI, please use config file for CLI.

ericmorand commented 5 months ago

About source and entry filter, I have the feeling that the second one is not needed and that every file (present in the V8 report or found in the source map) can be filtered against only the source filter - which is ultimately the declaration of the files that we want to cover.

I'll try locally to remove the option, and will create a PR only for discussion and proof of concept, if you agree.

cenfun commented 5 months ago

No, I don't think so. The entryFilter is used to filter all entry files from the V8 coverage data. like huge node_modules, otherwise, performance would suffer due to the parsing too many meaningless files. The sourceFilter is only used to filter source files from the sourcemap of the entry file. In other words, if an entry file does not have a sourcemap, then the sourceFilter will not be executed.

cenfun commented 5 months ago

I've thought about it, and perhaps it's feasible to merge the entryFilter and sourceFilter into one filter. I will continue to think about it.