Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Directory Provided by -r Flag Not Used as Coverage Root #1902

Closed
6 tasks done
alexlafroscia opened this issue Aug 23, 2022 · 14 comments
Closed
6 tasks done

Directory Provided by -r Flag Not Used as Coverage Root #1902

alexlafroscia opened this issue Aug 23, 2022 · 14 comments
Labels
feat: coverage Issues and PRs related to the coverage feature

Comments

@alexlafroscia
Copy link

Describe the bug

When running vitest from a directory that is not the one that contains the config file, and using the -r flag to point to another directory that should be treated as the "project root", c8 uses the current working directory as the "root" to gather coverage from instead of the actual project root.

The result of this is that, depending on which directory you run the command from, your coverage report can include a bunch of files that it shouldn't!

This is impacting my team because of the way we're running our tests in CI, due to the lack of "projects" support in Vitest (that could test multiple packages all at once). We're running Vitest from the workspace root using the -r flag, but our coverage reports now include the files from the mono-repo siblings of the package actually under test.

Reproduction

This repository demonstrates the bug, as well as describing one (awkward) fix that I came across

https://github.com/alexlafroscia/__vitest-monorepo-coverage

System Info

System:
    OS: macOS 12.5
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
    Memory: 15.00 GB / 64.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.13.2 - ~/.volta/tools/image/node/16.13.2/bin/node
    Yarn: 1.22.17 - ~/.volta/tools/image/yarn/1.22.17/bin/yarn
    npm: 8.1.2 - ~/.volta/tools/image/node/16.13.2/bin/npm
    Watchman: 4.9.0 - /opt/brew/bin/watchman
  Browsers:
    Chrome: 104.0.5112.101
    Safari: 15.6
    Safari Technology Preview: 16.0

Used Package Manager

npm

Validations

@sheremet-va sheremet-va added bug feat: coverage Issues and PRs related to the coverage feature labels Aug 24, 2022
@AriPerkkio
Copy link
Member

I think support from c8 is required as well: bcoe/c8#415

Istanbul provider is already passing the ctx.config.root to test-exclude.

this.testExclude = new _TestExclude({
cwd: ctx.config.root,
include: typeof this.options.include === 'undefined' ? undefined : [...this.options.include],
exclude: [...defaultExclude, ...defaultInclude, ...this.options.exclude],
excludeNodeModules: true,
extension: configDefaults.coverage.extension,
})

@sun0day
Copy link
Contributor

sun0day commented Aug 25, 2022

A quick solution for Vitest is that scan ctx.config.root package.json and then add dependencies to ctx.config.coverage.exclude before construct C8CoverageProvider.

@AriPerkkio
Copy link
Member

Looks like c8 has an undocumented resolve option which might solve this.

https://github.com/bcoe/c8/blob/7f1069dcce6821a6794557b39d6a13f620c64bad/lib/parse-args.js#L136-L139

@alexlafroscia I've tried to reproduce this issue using the reproduction repository. However I'm not seeing the expected erroneous results in the "Error case". In my tests the files of packages/a are never included in report.

Logs

Base case:

$ pwd
/workspaces/__vitest-monorepo-coverage/packages/b

$ npm test

> b@1.0.0 test
> vitest run --coverage


 RUN  v0.22.1 /workspaces/__vitest-monorepo-coverage/packages/b
      Coverage enabled with c8

 ✓ src/add-and-double.test.js (1)

Test Files  1 passed (1)
     Tests  1 passed (1)
  Start at  06:35:48
  Duration  1.29s (transform 652ms, setup 0ms, collect 20ms, tests 3ms)

 % Coverage report from c8
-------------------|---------|----------|---------|---------|-------------------
File               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-------------------|---------|----------|---------|---------|-------------------
All files          |     100 |      100 |     100 |     100 |                   
 add-and-double.js |     100 |      100 |     100 |     100 |                   
-------------------|---------|----------|---------|---------|-------------------
$ pwd
/workspaces/__vitest-monorepo-coverage

$ npm test

> __vitest-monorepo-coverage@1.0.0 test
> vitest run --coverage -r packages/b


 RUN  v0.22.1 /workspaces/__vitest-monorepo-coverage/packages/b
      Coverage enabled with c8

 ✓ src/add-and-double.test.js (1)

Test Files  1 passed (1)
     Tests  1 passed (1)
  Start at  06:34:55
  Duration  1.28s (transform 644ms, setup 0ms, collect 20ms, tests 2ms)

 % Coverage report from c8
-------------------|---------|----------|---------|---------|-------------------
File               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-------------------|---------|----------|---------|---------|-------------------
All files          |     100 |      100 |     100 |     100 |                   
 add-and-double.js |     100 |      100 |     100 |     100 |                   
-------------------|---------|----------|---------|---------|-------------------

@alexlafroscia
Copy link
Author

alexlafroscia commented Sep 14, 2022

Hey @AriPerkkio -- sorry about that! It seems I had checked in the "fix" that I found by mistake (which is documented at the end of the repo's README), which certainly makes reproducing this issue confusing!

I've removed that "fix" from the reproduction repo now

alexlafroscia/__vitest-monorepo-coverage@a985394

now running the tests at the workspace root should show the packages/a files in the coverage report

❯ pwd
/Users/alexlafroscia/Code/tmp/__vitest-monorepo-coverage

❯ npm test

> __vitest-monorepo-coverage@1.0.0 test
> vitest run --coverage -r packages/b


 RUN  v0.22.1 /Users/alexlafroscia/Code/tmp/__vitest-monorepo-coverage/packages/b
      Coverage enabled with c8

 ✓ src/add-and-double.test.js (1)

Test Files  1 passed (1)
     Tests  1 passed (1)
  Start at  12:46:22
  Duration  1.52s (transform 444ms, setup 0ms, collect 15ms, tests 2ms)

 % Coverage report from c8
--------------------|---------|----------|---------|---------|-------------------
File                | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
--------------------|---------|----------|---------|---------|-------------------
All files           |     100 |      100 |     100 |     100 |
 a/src              |     100 |      100 |     100 |     100 |
  index.js          |     100 |      100 |     100 |     100 |
 b/src              |     100 |      100 |     100 |     100 |
  add-and-double.js |     100 |      100 |     100 |     100 |
--------------------|---------|----------|---------|---------|-------------------

@AriPerkkio
Copy link
Member

Thanks @alexlafroscia, I'm able to reproduce the issue now.

We should really wait for support from c8's side for configuring the test-exclude. There is already an on-going PR which is adding a similar flag that could be used. I'm not really eager to add work-arounds like #1902 (comment).

Ideally vitest would be passing { cwd: ctx.config.root } (or projectRoot, depending on the final API name) here on coverage-c8/src/provider.ts#L49 and c8 would be using it here lib/report.js#L39. I tested these changes locally and it works perfectly.

@Jason3S
Copy link

Jason3S commented Jan 4, 2023

@alexlafroscia,

I also have a mono-repo setup. To generate the coverage report, I run coverage in each package folder, not at the root. Once they are finished, they are gathering into a single lcov.info file.

<root>/package.json.scripts

"coverage-collect": "globcat packages/*/coverage/lcov.info > lcov.info",

The aggregate file is sent to Coveralls or CodeCov.

This works fine with Jest, using:

<root>/jest.config.js

    coverageReporters: ['html', 'json', ['lcov', { projectRoot: __dirname }], 'text'],

But with vitest the process doesn't work out of the box. Instead each lcov.info has to be patched.

For example:
All SF lines in <root>/packages/client/coverage/lcov.info like this:

SF:src/index.ts needs to be adjusted to SR:packages/client/src/index.ts

I would rather have the ability to tell instanbul or c8 the coverageRoot so they can correctly generate the lcov.info files without needing to patch them.

@AriPerkkio
Copy link
Member

rather have the ability to tell istanbul or c8 the coverageRoot

@Jason3S If you can share a minimal reproduction repository for the istanbul case, I'm quite sure we can fix that. Clear reproduction steps describing what happens and what should happen, would be perfect for debugging this.

    coverageReporters: ['html', 'json', ['lcov', { projectRoot: __dirname }], 'text'],

The projectRoot is already used by Vitest's istanbul provider:

reports.create(reporter as any, {
skipFull: this.options.skipFull,
projectRoot: this.ctx.config.root,

It was added in #2051.

coverage/lcov.info:
Before: SF:packages/b/src/add-and-double.js
After: SF:src/add-and-double.js

@AriPerkkio
Copy link
Member

AriPerkkio commented Jan 5, 2023

c8@7.13.0 will add support for passing reporter options. I think this issue reported by @alexlafroscia can then be fixed in Vitest. The release has not yet been made though.

I think it's also worth to add some kind of "Coverage in monorepositories" section to documentation, https://vitest.dev/guide/coverage.html.

@Jason3S
Copy link

Jason3S commented Jan 5, 2023

@AriPerkkio,

Thank you for the quick response. I'll see if I can setup a sample repo.

But, you don't need a mono repo to test it out.

The generated <root>/package/client/coverage/coverage-final.json already contains full path names.

Here is a package vitest.config.ts file:

import { defineConfig } from 'vitest/config';
import * as path from 'path';

export default defineConfig({
    test: {
        // reporters: 'verbose',
        coverage: {
            // enabled: true,
            provider: 'istanbul',
            clean: true,
            all: false, // should be true.
            reporter: ['html', 'json', 'lcov', 'text'],
            exclude: [
                '**/*.test.*',
            ],
        },
        include: ['src/**/*.test.{ts,mts}'],
        exclude: ['content/**', 'fixtures/**', 'bin.mjs', '_snapshots_'],
        root: path.join(__dirname, '../..'),
        typecheck: {},
    },
});

run vitest run --coverage

It generates the report, but the lcov is relative to the package directory, not the root.

I tried changing root, but that doesn't seem to make a difference.

Note: it is possible to use nyc to generate the report afterwards.

cd package/client
nyc report --temp-dir "$(pwd)/coverage" --reporter lcov --report-dir "$(pwd)/cov" --cwd ../..

@AriPerkkio
Copy link
Member

I tried changing root, but that doesn't seem to make a difference.

I see the problem now. Using Vitest's config.root as reporters' projectRoot can be used as default value but there are times when it needs to be overwritten. Exactly as here you would need to use config.root as <root>/packages/client and reporters' projectRoot as <root>/.

I've started working on a feature that will expose reporter configuration options in coverage.reporter. It will have identical API as Jest has.

reporter: [
   ["text", { "projectRoot": "../../" }],
   ["json", { "file": "renamed-report.json" }],
],

This can already be implemented for istanbul provider but for c8 we'll need to wait for the new release.
This will solve @Jason3S's case, but the original problem described in this issue still requires more support from c8.

@Jason3S
Copy link

Jason3S commented Jan 13, 2023

@AriPerkkio,

Thank you.

@AriPerkkio
Copy link
Member

@alexlafroscia now that the new @vitest/coverage-v8 is replacing @vitest/coverage-c8 and we are no longer limited by c8, this issue should be resolved. Could you test whether the new package works for you?

@AriPerkkio
Copy link
Member

Closing as there has been no activity since last request. Both coverage providers should be able to provide this functionality now.

@Jason3S
Copy link

Jason3S commented Sep 5, 2023

It is now working for me.
Thank you for adding the ability to pass parameters.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: coverage Issues and PRs related to the coverage feature
Projects
None yet
Development

No branches or pull requests

5 participants