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

Use V8 builtin coverage reporting #7062

Closed
thymikee opened this issue Sep 28, 2018 · 15 comments · Fixed by #8596
Closed

Use V8 builtin coverage reporting #7062

thymikee opened this issue Sep 28, 2018 · 15 comments · Fixed by #8596

Comments

@thymikee
Copy link
Collaborator

🚀 Feature Proposal

V8 provides a native way of getting code coverage. Recently a c8 project by @bcoe was released to use this feature in Node land. Would be cool to experiment with this edgy tech.

Here's a blog post about it.

Motivation

Make coverage 2-3x faster and babel-free.

@thymikee thymikee changed the title Use V8 builtin coverage for reporting Use V8 builtin coverage reporting Sep 28, 2018
@SimenB
Copy link
Member

SimenB commented Sep 28, 2018

@aaronabramov you had tried this, or just looked at it?

@bcoe
Copy link
Contributor

bcoe commented Sep 28, 2018

👋 I'd love to help with this as best I can, there are a few outstanding issues that make the experience a little bit clunky, but shouldn't block a proof of concept:

nodejs/node#22919

A strategy for handling source-maps also needs to be developed. I've been chatting with @jdalton about some of these issues, as they relate to ESM -- perhaps we can aim for a community effort.

@SimenB
Copy link
Member

SimenB commented Sep 28, 2018

We talked a tiny bit internally last week, and it was the sourcemaps that we figured would be hard part. How much of that is handled in devtools? Or, do you have a write-up about it somewhere?

@bcoe
Copy link
Contributor

bcoe commented Sep 28, 2018

@SimenB in nyc, we handle source-maps with a fairly tiny abstraction on top of Mozilla's source-map library.

Some of this might be able to be torn out of nyc ... it might be better to start over with fresh eyes, especially since source-map has switched to an async API (nyc is pinned on the old sync API).

@SimenB
Copy link
Member

SimenB commented Sep 28, 2018

For coverage I suppose the async api works, but jest is also stuck on the sync api because of test errors which must be collected synchronously. Although I have hope for mozilla/source-map#331

@kwonoj
Copy link
Contributor

kwonoj commented Oct 31, 2018

for mozilla/source-map#331, I have created POC as discussion mozilla/source-map#369 - while I have low expectation if it could be accepted due to some hacky workaround to achieve sync interfaces. /cc @SimenB for fyi.

@azz
Copy link
Contributor

azz commented May 26, 2019

How can we get the ball rolling on this? Using V8 coverage and using native ES modules would remove the need for Jest to use babel out-of-the box, right?

@SimenB
Copy link
Member

SimenB commented May 26, 2019

You'd still need it to get mock hoisting, but I suppose that's something people can live without.

This was actually unblocked by bcoe/c8#85, so if anyone wants to work on this, that'd be awesome! 🙂

@bcoe
Copy link
Contributor

bcoe commented May 26, 2019

@SimenB 👋 one issue we still have is source-maps that are injected dynamically, because c8 doesn't hook module.require; I'd love to get the ball rolling on implementing source-maps in V8/Node.js itself.

@SimenB
Copy link
Member

SimenB commented May 26, 2019

Jest controls the source code and manually executes it in a sandbox, so we should be able to inject it still, right?

Built-in sourcemap support in the engine(s) would be absolutely amazing!

@bcoe
Copy link
Contributor

bcoe commented May 28, 2019

@SimenB I think with some fiddling we should be able to figure out a way to provide the source-map, might take some work in v8-to-istanbul; the main limitation faced by c8, is that it doesn't intercept source-code requires itself (which is a deprecated API any ways).

@SimenB
Copy link
Member

SimenB commented May 28, 2019

I'm in your slack, happy to try to figure out a way forwards in the not too distant future (swamped at work for the last few months and for a couple more).

I think this might be quite revolutionary if we're able to make it work

@SimenB
Copy link
Member

SimenB commented May 28, 2019

it doesn't intercept source-code requires itself (which is a deprecated API any ways

Could you expand a bit on this? Intercept how?

In Jest's case, we inject our own require function which we can make do whatever we want

@SimenB
Copy link
Member

SimenB commented Jun 19, 2019

For anyone following along:

I've experimented a bit with this, but the current approach is very FS based - you have to set an env variable before running node (we could wrap it a spawning node process, so not a biggie), but the report is only output to when the process exits. That makes it hard to properly deal with sourcemaps (since we manage them in memory, although they do exist on disk). Which means watch mode and coverage is a no-go (and we also have to do more manual process management).

Open issue request for a programmatic approach: nodejs/node#28283

@github-actions
Copy link

This issue 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 a pull request may close this issue.

5 participants