vitest icon indicating copy to clipboard operation
vitest copied to clipboard

feat: support `istanbul` coverage provider

Open AriPerkkio opened this issue 2 years ago • 7 comments

Closes #1252

Documentation of the istanbul packages is not perfect. I had to do some analyzing of jest and nyc to get things working.

  • https://github.com/facebook/jest/blob/main/packages/jest-reporters/src/CoverageReporter.ts
  • https://github.com/istanbuljs/nyc/tree/master/lib

Creates common interface for coverage providers to implement and adds new coverage provider:

  • onFileTransform:

    • c8: No file transforms needed
    • istanbul: Instrument files under testing with istanbul-lib-instrument. Coverage objects are added to globalThis.__VITEST_COVERAGE__
  • onBeforeFilesRun:

    • c8: Instruct v8 to enable coverage collection by setting up process.env.NODE_V8_COVERAGE
    • istanbul: No actions needed
  • onAfterSuiteRun:

    • c8: Write v8 coverage reports to file system using v8.takeCoverage()
    • istanbul: Collect the globalThis.__VITEST_COVERAGE__ from workers through birpc into main thread
  • onAfterAllFilesRun:

    • c8: Create report using c8/lib/report
    • istanbul: Process and merge coverage objects with istanbul-lib-coverage and create report with istanbul-reports

Example

vitest/test/coverage-test/src/utils.ts: Istanbul on the left with 57% coverage, v8 on the right with 70% coverage:

image

AriPerkkio avatar Jul 19 '22 10:07 AriPerkkio

This is still a work-in-progress but it might be worth to create a draft PR at this point to prevent duplicate work.

I think the refactor: create interface for coverage logic part is complete, but istanbul integration still requires work. I can continue this later on, but if anyone is eager to finish this PR let me know.

I think it's also good time for Vitest team to take a look at this overall and call for a stop if this doesn't look good.

AriPerkkio avatar Jul 19 '22 10:07 AriPerkkio

looks good overall, noticed few issues:

  1. exclude option seems to be ignored when using instanbul
  2. when setting provider to c8 explicitly vitest run --coverage skips coverage
  3. vitest doesn't suggest installing missing istanbul packages and skips coverage instead
  4. c8 randomly ignores coverage for 1 file in 100% covered code when using c8 with these changes

codebase I used for testing: https://github.com/wight554/blog-template You might notice schemas are added to coverage report when using istanbul [1] if you try

Upd. I use typescript instead of esbuild for compiling tests but ts-jest does the same and uses default jest instrument, so shouldn't be an issue Upd2. if matters:

  System:
    OS: Windows 10 10.0.22622
    CPU: (16) x64 AMD Ryzen 7 3700X 8-Core Processor
    Memory: 17.38 GB / 31.93 GB
  Binaries:
    Node: 16.14.0 - c:\program files\nodejs\node.EXE
    Yarn: 3.1.1 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 8.3.1 - c:\program files\nodejs\npm.CMD
  Browsers:
    Chrome: 103.0.5060.134
    Edge: Spartan (44.22621.436.0), Chromium (103.0.1264.71)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    vite: ^3.0.3 => 3.0.3
    vitest: 0.19.1 => 0.19.1

wight554 avatar Jul 26 '22 17:07 wight554

The overall direction looks great to me. Please ping me when you think the work on your side is done, and I can help to do the refactoring and make the packages more general as @vitest/coverage-istanbul and even @vitest/coverage-c8 later.

antfu avatar Jul 26 '22 18:07 antfu

From my side all that is left to do is unit+manual testing and add documentation. I think everything else (besides refactoring into separate packages) is done. I'm happy to leave these tasks to other people as well. 😄

I'm currently travelling and won't be doing anything for the next ~10 days. @antfu if you'd like to start working on this now, feel free to take over the PR. 👍

AriPerkkio avatar Jul 26 '22 19:07 AriPerkkio

Great, thanks! Have a good time traveling!

antfu avatar Jul 26 '22 19:07 antfu

There's now some initial documentation and unit tests in place. I think the next logical step is to refactor these coverage providers into their own packages - https://github.com/vitest-dev/vitest/pull/1676#issuecomment-1195844043.

AriPerkkio avatar Aug 04 '22 18:08 AriPerkkio

There's now some initial documentation and unit tests in place. I think the next logical step is to refactor these coverage providers into their own packages - #1676 (comment).

I think we should have these packages:

  • @vitest/coverage-v8
  • @vitest/coverage-istanbul

Then Vitest checks for coverage field, if package is installed. We might also expose this as CoverageProvider for extending with custom provider.

sheremet-va avatar Aug 04 '22 18:08 sheremet-va

Pushed some refactorings:

  • [x] Move to separate packages
  • [x] Make it work
  • [x] Prompt for auto-installing
  • [x] Support custom coverage provider

antfu avatar Aug 13 '22 02:08 antfu

@antfu coverage-c8 and coverage-istanbul build is failing locally.

src/provider.ts(6,10): error TS2305: Module '"vitest/config"' has no exported member 'configDefaults'.

Demivan avatar Aug 15 '22 10:08 Demivan

https://github.com/vitest-dev/vitest/runs/7835819196?check_suite_focus=true#step:6:107

Need to add node to tsconfig types

Demivan avatar Aug 15 '22 11:08 Demivan

It should be noticed provider: 'istanbul' is required if the user wants to use istanbul, ideally, if @vitest/coverage-istanbul is detected, istanbul should be used by default IMO.

JounQin avatar Aug 17 '22 09:08 JounQin

I thought about it as once, but it could be complex to handle, like if you have both c8 and Istanbul packages installed, or if we have more providers in the future. Also, it might be dangerous to have dependencies change the behavior implicitly.

antfu avatar Aug 18 '22 15:08 antfu

@antfu

But the current behavior is very strange.

When provide: 'Istanbul' is not enabled but @vitest/coverage-istanbul installed

 MISSING DEP  Can not find dependency '@vitest/coverage-c8'

✖ Do you want to install @vitest/coverage-c8? … no

There is no choice or notice to add provide: 'Istanbul'.

When provide: 'Istanbul' enabled with @vitest/coverage-istanbul installed

Coverage enabled with istanbul

This message is very redundant to appear every time.

if you have both c8 and Istanbul packages installed, or if we have more providers in the future

IMO, something like Coverage enabled with istanbul is just for this case, for the default fallback or conflict providers installed message.

JounQin avatar Aug 18 '22 15:08 JounQin