cenfun / monocart-coverage-reports

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

[Bug] Executing MCR inside a sub-directory doesn't honor the configuration file #23

Closed ericmorand closed 3 months ago

ericmorand commented 4 months ago

Describe the bug

When a configuration file exists in the root of a project, executing mcr in a sub-directory of the project doesn't honor the configuration file.

To Reproduce

module.exports = {
    outputDir: './custom-name-for-the-coverage-reports',
};
cd src/test
npx mcr ts-node index.ts

The name of the coverage report directory is still the default name instead of custom-name-for-the-coverage-reports.

Note that, executing npx mcr ts-node src/test/index.ts from the root of the project works as expected.

Expected behavior

The configuration file is honored, regardless of the directory where mcr is executed.

Additional context

The reason is that mcr looks up for configuration file only in the current working directory. This should not be a big deal to allow the code to find the first configuration file starting from the current working directory, and use it. The most tricky part would be to guarantee that this directory where the configuration file was found is considered as the base directory for the pattern matching (typically source and entry filters). I'll prepare a PR that implements this - we had to do the same thing for c8 a few days ago and it went very well.

cenfun commented 4 months ago

Currently, we can solve this by customizing the config file

npx mcr --config ../mcr.config.js

However, if the same outputDir is used (config file as base dir), it will result in multiple coverage reports being overwritten. It is recommended to generate reports separately with CLI and merge them at the end. see merge reports Or, you can generate reports using the API, see here

ericmorand commented 4 months ago

It doesn't work, since the filter patterns of the config file are matched against the current working directory instead of the directory where the config file is located. In other words, using src/main/**/*.ts as source filter in the config file, and executing mcr in src/test would make mcr expect that src/test/src/main/lib/foo.ts exists for example.

I tested it a few minutes ago and src/main/lib/foo.ts was filtered out by the source filter.

I'm preparing a PR to support this.

ericmorand commented 4 months ago

@cenfun , there are two alternatives there:

The first alternative comes with the benefit of not having to add a new option. It comes with a big (in my opinion) penalty: it makes impossible to import configurations - or at least, doing so is unsafe.

The second alternative comes with the benefit of allowing configuration importation (typically by using basePath: resolve(__dirname)) but at the cost of adding a new option.

Do you have a preference?

Edit:

By configuration import, I mean:

const baseConfiguration = require('../mcr.config');

module.exports = {
    ...baseConfiguration,
    name: "Test Suite Report"
};
cenfun commented 4 months ago

Thanks for the PR. So, the basePath is to solve the problem which is the pattern src/main/**/*.ts does not work? I don't think it's necessary. Could you please try pattern **/src/main/**/*.ts? Basically, all the entry urls are absolute path which is starting with file://, for example (Windows):

{"scriptId":"131","url":"file:///E:/bugs/c8-with-bundle/dist/index.mjs","functions" ...

And the source path is starting with ../ but not src/ in sourcemap (dist/index.mjs.map):

{"version":3,"file":"index.mjs","sources":["../src/main/lib/foo.ts","../src/test/index.ts"],"sourcesContent" ...

This is why pattern src/main/**/*.ts doesn't work, and we need to use **/src/main/**/*.ts.

For outputDir, we can use absolute path base on config path, for example:

// mcr.config.js
const path = require("path");
module.exports = {
    outputDir: path.resolve(__dirname, './custom-name-for-the-coverage-reports'),
};

And I agree to use the first configuration file based on the current work directory, such as using 'find-up'. I will implement it in the next version.

ericmorand commented 4 months ago

This is why pattern src/main/*/.ts doesn't work, and we need to use /src/main//*.ts.

It does not work either. Because ../src/main/lib/foo.ts is evaluated relatively to the current working directory, which is src/test.

Hence, it will be passed to the source filter as main/lib/foo.ts . This won't be matched by the pattern, regardless of starting with **/ or not.

This all comes to the fact that, ultimately, Util.relativePath is executed by getSourcePath with no root argument - which means it uses process.cwd as root. Basically, this is what happens:

import {relative} from "path";

console.log(relative(process.cwd(), '/home/ericmorand/Projects/c8-with-bundle/src/main/lib/foo.ts'));

When executed from the root of the project, this script logs:

src/main/lib/foo.ts

This file is matched by the pattern.

But when executed from src/test, this script logs:

../main/lib/foo.ts

This file is not matched by the pattern.

This is why, in my PR, I updated getSourcePath to pass the base path to Util.relativePath. There may be other ways to deal with this issue, of course.

ericmorand commented 4 months ago

Also, having to use **/... patterns is problematic: by not matching at the beginning of the file path, is catches too many files. For example, imagine the following project:

src
  main
    index.ts
  test
    index.ts
      src
        main
          foo.ts

src/test/src/main/foo.ts would be matched by the source filter **/src/main/**/*.ts, which is not what we want.

I really think that adding a basePath option is the way to go: it removes every uncertainty, doesn't force the filters to rely on **/ patterns anymore, and allows end user to adapt MCR to any usage imaginable. Plus, it is not mandatory and fallback to the current behaviour (process.cwd) when not provided, which means it doesn't create breaking changes.

cenfun commented 4 months ago

Just to confirm, have you made the following changes when using the config file mcr.config.js?

- "mcr": "npx mcr --reports=console-details --sourceFilter=src/main/**/*.ts npm t"
+ "mcr": "npx mcr npm t"

Because the CLI args have higher priority than the config file, so the CLI args must be removed in order to let the config file work. This might be the reason why **/src/main/**/*.ts is not working.

BTW, I have released a new version monocart-coverage-reports@2.8.0

Thank you for the previous suggestion.

cenfun commented 4 months ago

Also, having to use **/... patterns is problematic: by not matching at the beginning of the file path, is catches too many files. For example, imagine the following project:

src
  main
    index.ts
  test
    index.ts
      src
        main
          foo.ts

src/test/src/main/foo.ts would be matched by the source filter **/src/main/**/*.ts, which is not what we want.

For this issue, we need to use multiple patterns, for example:

{
"**/src/test/**": false, // exclude test files first
"**/src/**": true // then include src files
}
ericmorand commented 4 months ago

Just to confirm, have you made the following changes when using the config file mcr.config.js?

Yes, but this is not related since here I use mcr directly. That said, you can easily test the issue using the reproducer repository: https://github.com/ericmorand/c8-with-bundle/tree/master

After installing, just change directory to src/test and execute npx mcr ts-node index.ts. src/main/lib/foo.ts should be missing from the report. If you execute npx mcr ts-node src/test/index.ts from the root of the project, the file will be there.

By, the way, thanks for the "findUp" change. This is great.

cenfun commented 4 months ago

I understand your usage situation now. You want to freely test a specified test or a single file but not just the root project, and view its coverage report. Indeed, it is a requirement. However, the different cwd may cause the filter patterns not to work. Therefore, add the basePath (prefer to baseDir) option to normalize the source path, right?

for example:

after normalize the source path with baseDir:

ericmorand commented 4 months ago

Absolutely.

cenfun commented 4 months ago

Please try monocart-coverage-reports@2.8.1 with baseDir

const { resolve } = require('path');

// mcr.config.js
module.exports = {
    //logging:"debug",
    name: 'My Coverage Report',
    reports: [
        'v8',
        'console-details'
    ],
    outputDir: 'coverage',
    filter: {
        "**/node_modules/**": false,
        "**/dist/**": true,
        "**/test/src/**": false,
        "**/src/main/**/*.ts": true
    },
    // sourceFilter: (sourcePath) => {
    //     console.log(sourcePath)
    //     return true
    // },
    baseDir: resolve(__dirname)
};