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

feat: add support for v8 code coverage #8596

Merged
merged 26 commits into from
Dec 17, 2019
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Jun 23, 2019

Summary

Fixes #7062
Fixes #8557

A first somewhat functioning pass at adding support for v8 based code coverage instead of a babel one. This is mostly untested and completely undocumented, but feedback and usage on real code would be nice.

I opted for a new flag --v8-coverage rather than just repurposing the existing one mostly because I'm quite certain this new implementation is buggy, and people might want to keep using the babel based approach.

Also note that this requires node 8, so we need to wait for Jest 25 to land this.

Lastly, this needs istanbuljs/v8-to-istanbul#36, so make sure to yarn link that locally if you want to test this PR out

Test plan

Will need to test this more, but running Jest on the new "integration test" results in this:

image

@@ -0,0 +1,22 @@
{
"name": "@jest/coverage",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this module does not (and should not) have to live here. Maybe we can make it part of istanbul-lib-instrument? All it does is start and stop the coverage collection using the v8 inspector API.

/cc @bcoe @coreyfarrell

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have time to really look at this currently but I wouldn't object to creating a new module under the istanbuljs umbrella to accomplish this. Maybe @istanbuljs/v8-coverage would be a good name? I think it should be a separate module from istanbul-lib-instrument which pulls in a bunch of babel modules.

FWIW under the istanbuljs org I would not want TS source code but I don't have a problem with having the module include an index.d.ts with public definitions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I honestly have less concerns about TypeScript, since I've been using it at work these days; would be happy to put up my hand and help take on maintenance burden for this one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to take the code from this. Or create the repo and I can push it there.

If you want a separate d.ts. file, here's the built definition:

/// <reference types="node" />
import { Profiler } from 'inspector';
export declare type V8Coverage = ReadonlyArray<Profiler.ScriptCoverage>;
export default class CoverageInstrumenter {
    private readonly session;
    startInstrumenting(): Promise<void>;
    stopInstrumenting(): Promise<V8Coverage>;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async startInstrumenting() {
this.session.connect();

await new Promise((resolve, reject) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lost type info if I used util.promisify, so I just wrapped everything in promises manually

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting problem to solve. It's possible to strongly type the function returned by util.promisify by using some of the techniques described in this issue or this SO answer (kudos to their authors btw, ingenious ideas).

I tried this myself after reading both of the links above and managed to get it working this way:

  1. Create a conditional type which will be the union type of the variadic arguments.

    export type GetOverloadArgs<T> =
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
        (...o: infer U4) : void,
        (...o: infer U5) : void,
        (...o: infer U6) : void,
        (...o: infer U7) : void
    } ? U | U2 | U3 | U4 | U5 | U6 | U7:
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
        (...o: infer U4) : void,
        (...o: infer U5) : void,
        (...o: infer U6) : void,
    } ? U | U2 | U3 | U4 | U5 | U6:
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
        (...o: infer U4) : void,
        (...o: infer U5) : void,
    } ? U | U2 | U3 | U4 | U5:
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
        (...o: infer U4) : void,
    } ? U | U2 | U3 | U4 :
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
    } ? U | U2 | U3 :
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
    } ? U | U2 :
    T extends {
        (...o: infer U) : void,
    } ? U :
    never;
  2. Add type aliases for the Callbacks and the promisify function itself.
    PromisifyOne is just a conditional type which depends on the number of arguments passed (as they're variadic) and the Callback's generic type for reply.
    This makes sure that the return type will have the same type as the Callbacks second argument.

      export type Callback<T> = (err: Error | null, reply: T) => void;
    
      export type Promisify<T extends any[]> =
          T extends [Callback<infer U>?] ? () => Promise<U> :
          T extends [infer T1, Callback<infer U>?] ? (arg1: T1) => Promise<U> :
          T extends [infer T1, infer T2, Callback<infer U>?] ? (arg1: T1, arg2: T2) => Promise<U> :
          T extends [infer T1, infer T2, infer T3, Callback<infer U>?]? (arg1: T1, arg2: T2, arg3: T3) => Promise<U> :
          T extends [infer T1, infer T2, infer T3, infer T4, Callback<infer U>?] ? (arg1: T1, arg2: T2, arg3: T3, arg4: T4) => Promise<U> :
          never;
  3. Use the type of util.promisify's argument to obtain it's return type.

    declare module 'util' {
        function promisify<T>(fn: T): Promisify<T>;
    }

With the tree snippets above you can get the type info for func correctly in the example below:

import { promisify } from 'util';

const asyncWithCallback = (s: string, callback: (error: Error | null, r: number) => void): void => {
    let err = null;
    if (s === "err") err = new Error();
    callback(err, 10);
}

const func = promisify(asyncWithCallback);
func("not an error");
// `:YcmCompleter GetType` (which uses TSServer) gives me:
// const func: (arg1: string) => Promise<number>

I'm not sure why the other examples had to use UnionToIntersection to achieve that so I might have missed something there.

I'm also not sure if the tradeoff between having these complex type definitions versus manually wrapping these in promises is worth it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into it! I don't think it's worth the complexity here, but maybe you could upstream those changes to DT? The definitions are here: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/796b838a15fad73287bad7a88707a9ca04e60640/types/node/util.d.ts#L86-L105

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree, I think this actually belongs to DT's codebase since it's essentially covering what the type defs should be there.

I'll have a look at that, thanks @SimenB 😊

packages/jest-reporters/src/coverage_reporter.ts Outdated Show resolved Hide resolved
throw new Error('You need to `stopCollectingV8Coverage` first');
}
const filtered = this._v8CoverageResult
.filter(res => res.url.startsWith('file://'))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this removes node core modules

packages/jest-transform/src/ScriptTransformer.ts Outdated Show resolved Hide resolved
@thymikee
Copy link
Collaborator

Gave it a quick try locally within Jest, found this issue:

Screenshot 2019-06-23 at 13 58 33

Failing line: node_modules/istanbul-reports/lib/lcovonly/index.js:52:23

Copy link
Contributor

@lucasfcosta lucasfcosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello 😊

I've been reading recent PRs and the util.promisify issue you've had seemed very interesting so I went after a solution for it. Hope it helps.

I'm also amazed by the great work you all have been doing 💖

packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
async startInstrumenting() {
this.session.connect();

await new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting problem to solve. It's possible to strongly type the function returned by util.promisify by using some of the techniques described in this issue or this SO answer (kudos to their authors btw, ingenious ideas).

I tried this myself after reading both of the links above and managed to get it working this way:

  1. Create a conditional type which will be the union type of the variadic arguments.

    export type GetOverloadArgs<T> =
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
        (...o: infer U4) : void,
        (...o: infer U5) : void,
        (...o: infer U6) : void,
        (...o: infer U7) : void
    } ? U | U2 | U3 | U4 | U5 | U6 | U7:
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
        (...o: infer U4) : void,
        (...o: infer U5) : void,
        (...o: infer U6) : void,
    } ? U | U2 | U3 | U4 | U5 | U6:
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
        (...o: infer U4) : void,
        (...o: infer U5) : void,
    } ? U | U2 | U3 | U4 | U5:
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
        (...o: infer U4) : void,
    } ? U | U2 | U3 | U4 :
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
        (...o: infer U3) : void,
    } ? U | U2 | U3 :
    T extends {
        (...o: infer U) : void,
        (...o: infer U2) : void,
    } ? U | U2 :
    T extends {
        (...o: infer U) : void,
    } ? U :
    never;
  2. Add type aliases for the Callbacks and the promisify function itself.
    PromisifyOne is just a conditional type which depends on the number of arguments passed (as they're variadic) and the Callback's generic type for reply.
    This makes sure that the return type will have the same type as the Callbacks second argument.

      export type Callback<T> = (err: Error | null, reply: T) => void;
    
      export type Promisify<T extends any[]> =
          T extends [Callback<infer U>?] ? () => Promise<U> :
          T extends [infer T1, Callback<infer U>?] ? (arg1: T1) => Promise<U> :
          T extends [infer T1, infer T2, Callback<infer U>?] ? (arg1: T1, arg2: T2) => Promise<U> :
          T extends [infer T1, infer T2, infer T3, Callback<infer U>?]? (arg1: T1, arg2: T2, arg3: T3) => Promise<U> :
          T extends [infer T1, infer T2, infer T3, infer T4, Callback<infer U>?] ? (arg1: T1, arg2: T2, arg3: T3, arg4: T4) => Promise<U> :
          never;
  3. Use the type of util.promisify's argument to obtain it's return type.

    declare module 'util' {
        function promisify<T>(fn: T): Promisify<T>;
    }

With the tree snippets above you can get the type info for func correctly in the example below:

import { promisify } from 'util';

const asyncWithCallback = (s: string, callback: (error: Error | null, r: number) => void): void => {
    let err = null;
    if (s === "err") err = new Error();
    callback(err, 10);
}

const func = promisify(asyncWithCallback);
func("not an error");
// `:YcmCompleter GetType` (which uses TSServer) gives me:
// const func: (arg1: string) => Promise<number>

I'm not sure why the other examples had to use UnionToIntersection to achieve that so I might have missed something there.

I'm also not sure if the tradeoff between having these complex type definitions versus manually wrapping these in promises is worth it.

@jeysal
Copy link
Contributor

jeysal commented Jun 24, 2019

Awesome! Getting a bunch of

 ENOENT: no such file or directory, open '/private/var/folders/nh/_6t8lz3913q6z9346w579mj00000gp/T/jest_dy/jest-transform-cache-30e418979c5b1c567eb69895a4988d5a-ee2f6998f80ad1ca60bb674d9b0eef67/5c/index_5cd7dd727b8225beda9e1a4dd538feed.m
ap'

s on a work project, will try to isolate a repro if I have time

Copy link
Contributor

@scotthovestadt scotthovestadt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing.

@scotthovestadt
Copy link
Contributor

Just reviewed the code; haven't tried to actually run it locally yet. I'll take a quick pass at that tomorrow! Exciting stuff!

@SimenB
Copy link
Member Author

SimenB commented Jun 26, 2019

@jeysal a repro would be awesome 🙂 Seems somehow either the sourcemap is not written to disk or it got somehow moved/deleted as part of the test run. Do you have a custom transformer?

@scotthovestadt Nice, looking forward to it! Something about lcov coverage seems broken (ref @thymikee's simple repro above) and @jeysal's managed to crash it, but it'd be interesting if you got any different errors 🙂 Feel free to push any changes to this branch if you make any adjustments btw

@jeysal
Copy link
Contributor

jeysal commented Jul 10, 2019

@jeysal a repro would be awesome 🙂 Seems somehow either the sourcemap is not written to disk or it got somehow moved/deleted as part of the test run. Do you have a custom transformer?

Hey Simen, I've found the cause: A custom transform for CSS files that makes them empty modules.
I've created a minimal repro with instructions :)

@SimenB
Copy link
Member Author

SimenB commented Jul 10, 2019

Thanks @jeysal! Handling it now. It now fails with the lcov report thing than @thymikee reported above.

Added your reproduction as a test (just verifying it doesn't explode)

@sandersn
Copy link

sandersn commented Jul 15, 2019

In case you want a stress test, here is my silly model of 1/10 of typescript's test suite:

// @ts-check
var fs = require('fs')
var ts = require('./typescript.js')
const dirs = [
    '../../ts/tests/cases/compiler',
    '../../ts/tests/cases/conformance/ambient',
    '../../ts/tests/cases/conformance/salsa',
    '../../ts/tests/cases/conformance/jsdoc',
    '../../ts/tests/cases/conformance/es6',
    '../../ts/tests/cases/conformance/es7',
    '../../ts/tests/cases/conformance/es2017',
    '../../ts/tests/cases/conformance/es2018',
    '../../ts/tests/cases/conformance/es2019',
]
for (const dir of dirs) {
    for (const f of fs.readdirSync(dir)) {
        test(f, () => {
            const p = ts.createProgram({ rootNames: [dir + f], options: { "lib": ["es2015", "dom"] } })
            const x = ts.getPreEmitDiagnostics(p)
            // TODO: Emit baselines
            // TODO: Compare them
        })
    }
}

You'll need a clone of typescript in the right location relative to this test; this example uses ../../ts.

On my machine, this runs 5272 tests with coverage in 3,000 seconds. I'm running without coverage right now to get a time comparison.

Edit: 500 seconds, or about 6 times faster. That's surprisingly good.

(I'm also getting the crash that @thymikee reports.)

@SimenB
Copy link
Member Author

SimenB commented Jul 16, 2019

You can probably avoid the crash by setting coverageReporters to exclude lcov, e.g. just ['html']

@ballercat
Copy link

ballercat commented Jul 19, 2019

Thank you for the good work @SimenB !

We have a ton of coverage jobs for a very large repo so perf improvements are welcomed.

I gave this a shot on a pretty large subset of our tests and noticed a problem with outputting lcov results to HTML. There are fatal errors when the istambul lib tries to write HTML files with coverage to disk. After some light debugging I noticed that some files in the CoverageMap set had pretty long relative paths prefixed kind of like /../../../../actual/path/to/file.js, but only for some files, not all of them. This caused istambul to attempt to write to wrong /missing directories (assuming it's doing some path resolving here). I've also tried outputting just the lcov.info files and then post-processing them with genhtml with similar results (seems like the relative paths were baked into .info file).

Wasn't sure if you are aware of this so I figured I'd report it. I have not had a ton of luck making a minimal replicable example but if I have some free time I'll try to spin up a small GH repo for it. We have a pretty complex setup with custom resolver, not sure if that's been tested yet.

EDIT:

Attempted to replicate with a custom resolver but got a different issue this time. GitHub repo here https://github.com/ballercat/jest-v8-coverage-test

Each file (from src) which was resolved via custom resolver ended up not outputting properly in the HTML report. They all have similar error strings, just not the files which were resolved via defaultResolver!

sourceText.split is not a function
TypeError: sourceText.split is not a function
    at Object.annotateSourceCode (/Users/abuldauskas/nonwork/jest/node_modules/istanbul-reports/lib/html/annotator.js:218:33)
    at HtmlReport.onDetail (/Users/abuldauskas/nonwork/jest/node_modules/istanbul-reports/lib/html/index.js:256:19)
    at Visitor.(anonymous function) [as onDetail] (/Users/abuldauskas/nonwork/jest/node_modules/istanbul-lib-report/lib/tree.js:34:30)
    at ReportNode.Node.visit (/Users/abuldauskas/nonwork/jest/node_modules/istanbul-lib-report/lib/tree.js:114:17)
    at getChildren.forEach.child (/Users/abuldauskas/nonwork/jest/node_modules/istanbul-lib-report/lib/tree.js:118:15)
    at Array.forEach ()
    at ReportNode.Node.visit (/Users/abuldauskas/nonwork/jest/node_modules/istanbul-lib-report/lib/tree.js:117:24)
    at getChildren.forEach.child (/Users/abuldauskas/nonwork/jest/node_modules/istanbul-lib-report/lib/tree.js:118:15)
    at Array.forEach ()
    at ReportNode.Node.visit (/Users/abuldauskas/nonwork/jest/node_modules/istanbul-lib-report/lib/tree.js:117:24)

@codecov-io
Copy link

codecov-io commented Dec 11, 2019

Codecov Report

Merging #8596 into master will decrease coverage by 0.24%.
The diff coverage is 25.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8596      +/-   ##
==========================================
- Coverage   64.93%   64.69%   -0.25%     
==========================================
  Files         278      280       +2     
  Lines       11905    11978      +73     
  Branches     2935     2956      +21     
==========================================
+ Hits         7731     7749      +18     
- Misses       3544     3597      +53     
- Partials      630      632       +2
Impacted Files Coverage Δ
packages/jest-config/src/Defaults.ts 100% <ø> (ø) ⬆️
e2e/v8-coverage/no-sourcemap/x.css 100% <ø> (ø)
packages/jest-config/src/normalize.ts 77.74% <ø> (ø) ⬆️
packages/jest-config/src/index.ts 11.94% <ø> (ø) ⬆️
packages/jest-config/src/ValidConfig.ts 100% <ø> (ø) ⬆️
packages/jest-runner/src/runTest.ts 2.19% <0%> (-0.25%) ⬇️
...ckages/jest-reporters/src/generateEmptyCoverage.ts 66.66% <0%> (-22.23%) ⬇️
packages/jest-transform/src/ScriptTransformer.ts 68.63% <100%> (+0.14%) ⬆️
e2e/v8-coverage/no-sourcemap/Thing.js 100% <100%> (ø)
packages/jest-reporters/src/coverage_reporter.ts 51.21% <13.95%> (-10.62%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 012fc8b...9c7c353. Read the comment docs.

docs/Configuration.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results differ quite a bunch from istanbul, but this seems like a good starting point to release an experimental version and let people test it :)

@SimenB
Copy link
Member Author

SimenB commented Dec 16, 2019

@jeysal wanna look over one last time?

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeysal it works if specifying testEnvironment in config.

Does not for me unfortunately. Did you use Node 12? Might be OS-specific, I can try on macOS tomorrow?

docs/CLI.md Outdated Show resolved Hide resolved
docs/CLI.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
@SimenB
Copy link
Member Author

SimenB commented Dec 17, 2019

Does not for me unfortunately. Did you use Node 12? Might be OS-specific, I can try on macOS tomorrow?

Node 10, 12 and 13 all work for me. The example added as e2e test is from your example repo, does running ../../../jest --coverage --coverage-provider v8 work for you? The test passes on both linux and windows on CI

@SimenB SimenB merged commit 89c151b into jestjs:master Dec 17, 2019
@SimenB SimenB deleted the v8-coverage branch December 17, 2019 09:52
@jeysal
Copy link
Contributor

jeysal commented Dec 17, 2019

Alright I made another very simple repro https://github.com/jeysal/jest-v8-no-coverage
Instructions in readme, I used Node 12.13.1.
--collectCoverageFrom=index.js doesn't help either, forces the file to show up but still 0%

@jeysal
Copy link
Contributor

jeysal commented Dec 17, 2019

Or it might be something else wrong? #9319

@SimenB
Copy link
Member Author

SimenB commented Dec 17, 2019

@jeysal something to do with linking and how we look up envs methinks.

image

EDIT: Yeah, linking jest-environment-node as well works

@jeysal
Copy link
Contributor

jeysal commented Dec 17, 2019

Oh ok that works. Do you just delete / not install node_modules before running ../jest/jest? Because if I do that after yarn it still seems to use the local jest-cli.

@SimenB
Copy link
Member Author

SimenB commented Dec 17, 2019

linking both jest-cli and the node env makes yarn test work. But yeah, I didn't run install before the first attempt

@jeysal
Copy link
Contributor

jeysal commented Dec 17, 2019

Yes I got that working, was referring to how you did it without linking just with running a jest from somewhere else

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest runs out of memory when producing coverage for a large file Use V8 builtin coverage reporting