feat: support `istanbul` coverage provider #1676

AriPerkkio · 2022-07-19T10:14:04Z

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

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:

AriPerkkio · 2022-07-19T10:14:56Z

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.

packages/vitest/src/node/core.ts

packages/vitest/src/node/plugins/index.ts

packages/vitest/src/runtime/run.ts

packages/vitest/src/integrations/coverage/c8.ts

packages/vitest/src/integrations/coverage/istanbul.ts

Demivan · 2022-07-20T15:00:11Z

packages/vitest/src/node/cli-api.ts

+    // TODO: Requring all these packages to be installed by users may be too much
+    const requiredPackages = ctx.config.coverage.provider === 'istanbul'
+      ? [
+          'istanbul-lib-coverage',
+          'istanbul-lib-instrument',
+          'istanbul-lib-report',
+          'istanbul-reports',
+        ]


We can create @vitest/istanbul package that just reexports what is needed.

@vitest-dev/team What do you think?

Yes, it sounds great to me
#1676 (comment)

packages/vitest/src/integrations/coverage/base.ts

packages/vitest/src/node/core.ts

packages/vitest/src/types/coverage.ts

packages/vitest/src/integrations/coverage/istanbul.ts

wight554 · 2022-07-25T18:28:19Z

packages/vitest/src/integrations/coverage/istanbul.ts

+    this.ctx = ctx
+    this.options = resolveIstanbulOptions(ctx.config.coverage, ctx.config.root)
+
+    const { createInstrumenter } = require('istanbul-lib-instrument')


Would be cool to see this configurable to use custom instrumenters like https://github.com/kwonoj/swc-coverage-instrument/

This seems like a cool idea, thanks for improvement idea @wight554. However I think it might be best if we leave this customization out of this PR and introduce it in a separate feature afterwards.
Do you think there is anything important we need to take into consideration at this point? Leave some abstractions in place, etc?
My Rust knowledge is a bit rusty and to me it's not that clear what exactly import('swc-plugin-coverage-instrument') would return here. Does it match the API of istanbul-lib-instrument?

This seems like a cool idea, thanks for improvement idea @wight554. However I think it might be best if we leave this customization out of this PR and introduce it in a separate feature afterwards. Do you think there is anything important we need to take into consideration at this point? Leave some abstractions in place, etc? My Rust knowledge is a bit rusty and to me it's not that clear what exactly import('swc-plugin-coverage-instrument') would return here. Does it match the API of istanbul-lib-instrument?

I've checked how jest handles this, see https://github.com/facebook/jest/blob/24e0472ac41ea03cdd89ffaea8c5f410ecf6054f/packages/jest-transform/src/ScriptTransformer.ts#L410
In this case all that needs to be added is config value to skip instrumenter part and return unmodified map and code in onFileTransform

wight554 · 2022-07-26T17:47:36Z

looks good overall, noticed few issues:

exclude option seems to be ignored when using instanbul
when setting provider to c8 explicitly vitest run --coverage skips coverage
vitest doesn't suggest installing missing istanbul packages and skips coverage instead
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

antfu · 2022-07-26T18:35:26Z

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.

AriPerkkio · 2022-07-26T19:02:22Z

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. 👍

antfu · 2022-07-26T19:04:58Z

Great, thanks! Have a good time traveling!

packages/vitest/src/types/coverage.ts

AriPerkkio · 2022-08-04T18:35:30Z

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).

sheremet-va · 2022-08-04T18:41:03Z

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.

Demivan · 2022-08-15T10:57:15Z

@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 · 2022-08-15T11:02:03Z

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

Need to add node to tsconfig types

JounQin · 2022-08-17T09:27:04Z

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.

antfu · 2022-08-18T15:10:10Z

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.

JounQin · 2022-08-18T15:26:32Z

@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.

Demivan suggested changes Jul 19, 2022

View reviewed changes

packages/vitest/src/node/core.ts Outdated Show resolved Hide resolved

packages/vitest/src/node/plugins/index.ts Outdated Show resolved Hide resolved

packages/vitest/src/runtime/run.ts Outdated Show resolved Hide resolved

Demivan reviewed Jul 19, 2022

View reviewed changes

packages/vitest/src/integrations/coverage/c8.ts Outdated Show resolved Hide resolved

AriPerkkio commented Jul 20, 2022

View reviewed changes

packages/vitest/src/integrations/coverage/istanbul.ts Outdated Show resolved Hide resolved

Demivan reviewed Jul 20, 2022

View reviewed changes

AriPerkkio force-pushed the feat/coverage-provider-istanbul branch 2 times, most recently from bc73f74 to 36b3f17 Compare July 21, 2022 07:27

AriPerkkio commented Jul 21, 2022

View reviewed changes

packages/vitest/src/integrations/coverage/base.ts Outdated Show resolved Hide resolved

AriPerkkio force-pushed the feat/coverage-provider-istanbul branch 3 times, most recently from e657060 to 5b48782 Compare July 22, 2022 11:43

Demivan reviewed Jul 22, 2022

View reviewed changes

packages/vitest/src/node/core.ts Outdated Show resolved Hide resolved

Demivan reviewed Jul 22, 2022

View reviewed changes

packages/vitest/src/types/coverage.ts Outdated Show resolved Hide resolved

Demivan reviewed Jul 22, 2022

View reviewed changes

packages/vitest/src/integrations/coverage/istanbul.ts Outdated Show resolved Hide resolved

AriPerkkio force-pushed the feat/coverage-provider-istanbul branch 3 times, most recently from fea9cce to fc13750 Compare July 23, 2022 11:14

wight554 reviewed Jul 25, 2022

View reviewed changes

AriPerkkio force-pushed the feat/coverage-provider-istanbul branch 2 times, most recently from 1bfd7e0 to 15d1ddc Compare August 2, 2022 15:30

AriPerkkio commented Aug 2, 2022

View reviewed changes

packages/vitest/src/types/coverage.ts Outdated Show resolved Hide resolved

AriPerkkio force-pushed the feat/coverage-provider-istanbul branch 3 times, most recently from 4c060d9 to 0d28eb2 Compare August 4, 2022 18:26

antfu marked this pull request as ready for review August 15, 2022 06:09

antfu added 4 commits August 15, 2022 14:12

chore: fix types

dd38877

Merge remote-tracking branch 'origin/main' into pr/AriPerkkio/1676

3055192

chore: fix build

ce30200

Merge remote-tracking branch 'origin/main' into pr/AriPerkkio/1676

f8722fa

antfu approved these changes Aug 15, 2022

View reviewed changes

antfu added 4 commits August 15, 2022 17:13

chore: update deps

cd927b9

Merge remote-tracking branch 'origin/main' into pr/AriPerkkio/1676

a5cee1c

chore: lint

380c210

chore: upgrade esno

246e146

antfu requested a review from Demivan August 15, 2022 09:32

Merge branch 'main' into pr/AriPerkkio/1676

4edf2ad

antfu added 7 commits August 15, 2022 23:53

chore: update

016030d

chore: update

43dd177

Merge branch 'main' into pr/AriPerkkio/1676

4fb4dcd

Merge branch 'main' into pr/AriPerkkio/1676

1b1f059

chore: fix tsconfig

bc15516

chore: use rollup

4b4b0a6

chore: update

4ff81ae

antfu enabled auto-merge (squash) August 15, 2022 18:19

antfu disabled auto-merge August 15, 2022 18:20

antfu merged commit 265fdbe into vitest-dev:main Aug 15, 2022

AriPerkkio deleted the feat/coverage-provider-istanbul branch August 15, 2022 18:40

AriPerkkio mentioned this pull request Aug 27, 2022

fix: clean coverage-c8 tmp before reporting (fix #1917) #1925

Merged

multun mentioned this pull request Dec 2, 2023

ci: take integration tests into account for front coverage OpenRailAssociation/osrd#5976

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support `istanbul` coverage provider #1676

feat: support `istanbul` coverage provider #1676

AriPerkkio commented Jul 19, 2022 •

edited

Loading

AriPerkkio commented Jul 19, 2022

Demivan Jul 20, 2022

antfu Jul 26, 2022

wight554 Jul 25, 2022

AriPerkkio Jul 26, 2022

wight554 Jul 26, 2022

wight554 commented Jul 26, 2022 •

edited

Loading

antfu commented Jul 26, 2022

AriPerkkio commented Jul 26, 2022 •

edited

Loading

antfu commented Jul 26, 2022

AriPerkkio commented Aug 4, 2022

sheremet-va commented Aug 4, 2022

Demivan commented Aug 15, 2022

Demivan commented Aug 15, 2022

JounQin commented Aug 17, 2022

antfu commented Aug 18, 2022

JounQin commented Aug 18, 2022

feat: support istanbul coverage provider #1676

feat: support istanbul coverage provider #1676

Conversation

AriPerkkio commented Jul 19, 2022 • edited Loading

Example

AriPerkkio commented Jul 19, 2022

Demivan Jul 20, 2022

Choose a reason for hiding this comment

antfu Jul 26, 2022

Choose a reason for hiding this comment

wight554 Jul 25, 2022

Choose a reason for hiding this comment

AriPerkkio Jul 26, 2022

Choose a reason for hiding this comment

wight554 Jul 26, 2022

Choose a reason for hiding this comment

wight554 commented Jul 26, 2022 • edited Loading

antfu commented Jul 26, 2022

AriPerkkio commented Jul 26, 2022 • edited Loading

antfu commented Jul 26, 2022

AriPerkkio commented Aug 4, 2022

sheremet-va commented Aug 4, 2022

Demivan commented Aug 15, 2022

Demivan commented Aug 15, 2022

JounQin commented Aug 17, 2022

antfu commented Aug 18, 2022

JounQin commented Aug 18, 2022

feat: support `istanbul` coverage provider #1676

feat: support `istanbul` coverage provider #1676

AriPerkkio commented Jul 19, 2022 •

edited

Loading

wight554 commented Jul 26, 2022 •

edited

Loading

AriPerkkio commented Jul 26, 2022 •

edited

Loading