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

Coverage: give the choice between c8 and nyc #1252

Closed
4 tasks done
tqn-treezor opened this issue May 7, 2022 · 15 comments · Fixed by #1676
Closed
4 tasks done

Coverage: give the choice between c8 and nyc #1252

tqn-treezor opened this issue May 7, 2022 · 15 comments · Fixed by #1676
Labels
enhancement New feature or request feat: coverage Issues and PRs related to the coverage feature

Comments

@tqn-treezor
Copy link

tqn-treezor commented May 7, 2022

Clear and concise description of the problem

I observed huge coverage gaps when transitioning from Jest to Vitest, the reason coming from C8 not ignoring a few things:

  • types.ts were ignored by default in Jest and not in Vitest
  • *.entity.ts were ignored by default in Jest and not in Vitest
  • *.test.ts were ignored by default in Jest and not in Vitest
  • For includes/excludes, Vitest takes absolute paths, Jest accepts relative paths (that may lead to include or exclude paths not working and giving erroneous results in your coverage)
  • Comments are ignored by default in Jest and not in Vitest
  • Import lines are ignored by default in Jest and not in Vitest
  • Objects spread in multiple lines are considered as one line in Jest, and as many lines as there is in Vitest

The "deep reason" seems that Jest uses nyc, and Vitest uses c8, and they don't have the same way of running through the code to make the coverage report.
Besides, the way c8 does it (not ignoring comments) seems objectively wrong, because adding comments in any covered branch in any file will actually increase the code coverage when it really shouldn't. Same for multiple line objects.

Suggested solution

Give the choice to use nyc instead of c8.

Alternative

Fix c8? There is actually an issue dating from 2019 talking about the comments counted as covered here: bcoe/c8#182

Additional context

No response

Validations

@sheremet-va sheremet-va added the enhancement New feature or request label May 7, 2022
@AriPerkkio
Copy link
Member

+1 for this.

I really like the idea of using c8. Using native v8 coverage will definitely be fast since no code instrumentation is needed. But in practice it just doesn't work that well.

In addition to all issues mentioned by @tqn-treezor, I have problems when testing codebase with multiple testing tools. I have a component library which is tested by using vitest for small unit tests, and Cypress Component Test Runner for integration/behaviour testing. When merging the lcov reports generated by nyc and c8 the final coverage report is a complete mess. V8 coverage calculation is so much different. I had no other options than disabling whole vitest coverage of the project. 😑

Having an option for choosing between c8 and nyc would be great.

@pm-pp
Copy link

pm-pp commented May 9, 2022

+1

@sheremet-va
Copy link
Member

sheremet-va commented Jun 10, 2022

I think all described points are true only because Jest has configured nyc to run this way. I'm sure if we just swap them nothing will really change. The problem is with default configuration, and not with the coverage tool.

@Dschungelabenteuer
Copy link

Dschungelabenteuer commented Jun 13, 2022

I get your point. Still, as mentioned above, since c8 still has an open comment-related issue, there does not seem to be any configuration regarding those comments. I'm therefore kind of stuck with this sort of uncovered lines:

image

Just for the record, I'm also running into issues where c8 outputs different coverage depending on whether I run a single or all test files:

Running coverage on a single test file:

---------------------|---------|----------|---------|---------|-------------------------------------
File                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
---------------------|---------|----------|---------|---------|-------------------------------------
strategy.ts          |   99.13 |      100 |     100 |   99.13 | 57-58

Running coverage on all test files:

---------------------|---------|----------|---------|---------|-------------------------------------
File                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
---------------------|---------|----------|---------|---------|-------------------------------------
strategy.ts          |   36.36 |      100 |   52.94 |   36.36 | ...,161-187,195-207,215-220,227-230

This did not happen using Jest and nyc. I know this should rather be an issue on c8's repository, but I feel like the easiest way for me to get this working properly would be to use nyc (even if it requires additional configuration as you stated).

@sheremet-va
Copy link
Member

It does not happen in Jest because it is configured by Jest. What is the difference between us configuring v8 and nyc? One is already configured but not correctly, it seems. But at least it is already an integral part of Vitest, that we can improve.

@zhengjiaqi
Copy link

I got same problem ,and in Vitest React Testing Lib Example coverage is incorrect。
截屏2022-06-15 下午7 09 34

@zhengjiaqi
Copy link

I got same problem ,and in Vitest React Testing Lib Example coverage is incorrect。 截屏2022-06-15 下午7 09 34

I saved this problem by update node to v16.14.0.
截屏2022-06-15 下午8 03 10

@AriPerkkio
Copy link
Member

AriPerkkio commented Jun 15, 2022

What is the difference between us configuring v8 and nyc? One is already configured but not correctly, it seems.

I'm not expert here so please correct me if there's something misunderstood. So, mostly thinking out loud:

Difference between v8 report and any other AST based instrumented code coverage (like nyc) is the ability to actually understand where we need to collect the coverage from. "Coverage counters" are not injected on unnecessary lines.

The v8 coverage report includes every single line. v8-to-istanbul (used by c8) does not remove empty lines, comments, import statements etc. It's a simple format conversion. It does not implement such functionality and is not configurable in such way. This means that adding empty lines or more comments will increase code coverage.

I'm happy to help with this issue but quite unsure where to start from, if c8 is the way to go. My use case here is to be able to use vitest in addition to Cypress.

@sheremet-va
Copy link
Member

sheremet-va commented Jun 22, 2022

I think in the long run we might switch to nyc to support coverage in #1302. If someone is skilled enough, feel free to create a PR. If you need any guidance about the repo, please, use discord.

what do you think, @antfu, @Demivan?

@Demivan
Copy link
Member

Demivan commented Jun 22, 2022

I think it is a good idea to support nyc. I think it should improve performance of coverage collection too. Node native coverage is collecting so much extra data.

I'm not sure whether we need an option to switch between nyc and c8 or just switch to nyc

@antfu
Copy link
Member

antfu commented Jun 22, 2022

I guess the best solution might be we decouple the coverage logic, and make a provider interface for c8 and nyc to make them interchangeable. I currently don't have enough bandwidth to work on it, counting on someone interested in giving it a try.

@sheremet-va
Copy link
Member

sheremet-va commented Jun 22, 2022

I guess the best solution might be we decouple the coverage logic, and make a provider interface for c8 and nyc to make them interchangeable. I currently don't have enough bandwidth to work on it, counting on someone interested in giving it a try.

I definitely think we should create a common interface that can be implemented by different coverage libraries, but I am not sure its possible to run c8 with browser? I wouldn’t want people to have different coverage reports depending on the environment.

Maybe we can disable c8 coverage, if user enables --browser?

@eduardoaraujobtiba
Copy link

I got same problem ,and in Vitest React Testing Lib Example coverage is incorrect。 截屏2022-06-15 下午7 09 34

I'm also having this same issue, I tried switching to node v16.14.0 (I was using 16.13.0) and I still got an empty coverage report

@bjarkihall
Copy link

bjarkihall commented Jun 23, 2022

--browser would likely only support nyc on manual runs but if it's being run in a controlled env, like browser automation / CI, wouldn't it be exporting a c8 like format (at least for chromium-based browsers in puppeteer/playwright: https://playwright.dev/docs/api/class-coverage and bcoe/c8#162 (comment))?

Edit: I'm not too familiar with coverage tools so I had the impression c8 was cli/node based tool while nyc was somehow brower based (given it's usually used with mocha, e.g. mocha-vite-puppeteer). Sorry for my confusion here, I was trying to find good information on nyc to chip in.

@AriPerkkio
Copy link
Member

[...] but I am not sure its possible to run c8 with browser?

Exactly as @bjarkihall mentioned here, c8 limits us to use Chromium/v8 based browsers. Chromium's Profiler is used to actual coverage collection, and c8 is used for coverage report creation. Some early thoughts from couple of months ago here #586 (comment).

Coverage logic decoupled, actions depending on which coverage tool is chosen by user:

  • Before starting vite server:
    • nyc: instrument code with istanbul
    • c8: no actions
  • Before tests are run:
    • nyc: no actions
    • c8: toggle require('v8').takeCoverage()
  • Run tests
  • Before closing up:
    • nyc: Collect the coverage from global __coverage__ variable and process it with istanbul. By default this seems to be added to window so there might be some work-arounds or configuration required.
    • c8: Generate report using c8/lib/report

I think the integration itself will be on istanbul rather than nyc, but naming the API could as well be called nyc I guess.

When c8 is chosen and browser tests are run, maybe log warning related to coverage not being collected.

I did some testing with vitest + vite-plugin-istanbul + istanbul-packages, https://stackblitz.com/edit/vitest-dev-vitest-3sysd2?file=vite.config.ts,README.md. Sourcemaps are still off but at least empty lines are not picked as covered.

@sheremet-va sheremet-va added the feat: coverage Issues and PRs related to the coverage feature label Jun 25, 2022
AriPerkkio added a commit to AriPerkkio/vitest that referenced this issue Jul 17, 2022
@sheremet-va sheremet-va pinned this issue Aug 4, 2022
@sheremet-va sheremet-va unpinned this issue Aug 16, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request feat: coverage Issues and PRs related to the coverage feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants