v8: implement v8.takeCoverage() and v8.stopCoverage()

[joyeecheung]

v8: implement v8.takeCoverage()

Add an v8.takeCoverage() API that allows the user to write the
coverage started by NODE_V8_COVERAGE to disk on demand.
The coverage can be written multiple times during the lifetime
of the process, each time the execution counter will be reset.
When the process is about to exit, one last coverage will
still be written to disk.

Also refactors the internal profiler connection code
so that we use the inspector response id to identify
the profile response instead of using an ad-hoc flag in C++.

v8: implement v8.stopCoverage()

Add a v8.stopCoverage() API to stop the coverage collection
started by NODE_V8_COVERAGE - this would be useful in
conjunction with v8.takeCoverage() if the user don't want
to emit the coverage at the process exit but still want
to collect it on demand at some point.

cc @atian25

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31794/

[targos]

@bcoe

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31795/

[joyeecheung]

Added v8.stopCoverage() at the request of @atian25. Also added docs. This should be ready for review now.

@bcoe @addaleax PTAL, thanks

[devsnek]

it would be nice if this was available as an error callback or something, since the calling code can't really read stderr

[bnoordhuis]

The only reason I could see it failing is when message.length() > kStringMaxLength which:

1. I don't think can realistically happen, and
2. Isn't actionable for users when it does (except by opening a bug report here)

Maybe the error message could include the size and perhaps the first N bytes of the message.

Similar reasoning applies to the error code paths below. Failure likely means we need to fix something on our end.

[bcoe]

@joyeecheung I think this could be useful for correlating test cases to the source code the execute. To facilitate this, might it be worth having a .resumeCoverage method too?

[atian25]

since it has takeCovergae / stopCoverage, how about also provide startCoverage?

[bcoe]

how about also provide startCoverage?

@atian25 my concern with this variable name was that it might imply to folks that they can start coverage after the process has already started, whereas @joyeecheung indicated that we would still need to set the variable NODE_V8_COVERAGE.

@joyeecheung could we potentially make it so v8.startCoverage works, even if we didn't start Node with NODE_V8_COVERAGE?

[atian25]

how about also provide startCoverage?

@atian25 my concern with this variable name was that it might imply to folks that they can start coverage after the process has already started, whereas @joyeecheung indicated that we would still need to set the variable NODE_V8_COVERAGE.

@joyeecheung could we potentially make it so v8.startCoverage works, even if we didn't start Node with NODE_V8_COVERAGE?

NODE_V8_COVERAGE is equals to: (but only dump coverage when process exit)

const inspector = require('inspector');
const { promisify } = require('util');

const session = new inspector.Session();
session.connect();
session.post = promisify(session.post);

await session.post('Profiler.enable');
await session.post('Profiler.startPreciseCoverage', { callCount: true, detailed: true });

that is what I'm using now（workaroud), but since it's async so if require some file before startPreciseCoverage, it will missing the init structure to cause c8 report inaccuracy.(all file will at lease 1x)

so I ask @joyeecheung for these APIs.

[bcoe]

@atian25 @joyeecheung I like this addition to the API 😄 and think that, if possible, adding the startCoverage would also be useful (for folks who want to programmatically start coverage.

@SimenB I wonder, could this work simplify your implementation in Jest?

[atian25]

another question:

v8.takeCoverage() will it return the result or we had to read file?

how could we know it had done it's job?

[SimenB]

@SimenB I wonder, could this work simplify your implementation in Jest?

Sure! We currently use https://github.com/SimenB/collect-v8-coverage, so it seems like it'd be pretty much the same API from Jest's standpoint. Would love to get it from core rather than rolling our own! Would need the discussed startCoverage though, and reading from disk is not ideal

[bnoordhuis]

Reorder to avoid a potential handle leak:

Suggested change

	Local<Context> context = env->context();
	HandleScope handle_scope(isolate);
	HandleScope handle_scope(isolate);
	Local<Context> context = env->context();

('Potential' because env->context() reinterpret_casts a persistent handle but better safe than sorry.)

[bnoordhuis]

The only reason I could see it failing is when message.length() > kStringMaxLength which:

1. I don't think can realistically happen, and
2. Isn't actionable for users when it does (except by opening a bug report here)

Maybe the error message could include the size and perhaps the first N bytes of the message.

Similar reasoning applies to the error code paths below. Failure likely means we need to fix something on our end.

[bcoe]

reading from disk is not ideal

Yeah, if we could make the API be that stopCoverage() returns an object similar to what serializes to disk, And if we had startCoverage() ... I feel like this would be a pretty killer feature.

If we want to use this as a starting point, and we're labeling it experimental, I'm also happy with moving towards this incrementally.

[joyeecheung]

Thanks for the feedback, everyone. To summerize the discussion above:

1. Add a v8.startCoverage() method to reuse the per-env connection, if NODE_V8_COVERAGE is not set (would be useful to restart after v8.stopCoverage())
2. It would be good to have an API that (optionally?) returns the coverage result. I think it should be v8.takeCoverage() as the Profiler.stopPreciseCoverage command doesn't return this but the Profiler.takePreciseCoverage does. Also, I realize that this method should be asynchronous as there's no guarantee that we can get the response from inspector synchronously (it just happens to be this way right now), so it should probably return a promise.
3. Error reporting can be improved (return a rejection?)

Do people feel like these should all be done in this PR or it's OK to have experimental methods that returns undefined (with no synchronicity guaranteed) first and leave the promise implementation to another PR?

I've been a bit busy this week, will finish the requests above next week.

[bcoe]

It would be good to have an API that (optionally?) returns the coverage result.

I think that it might be a bit cleaner to make takeCoverage only return an object (or promise of an object) at which point consumers could choose whether or not to write to disk.

This would also mean that we wouldn't need to make startCoverage take a path argument (keeping in mind that the API today expects that NODE_V8_COVERAGE has the path coverage is being written to provided as an argument).

Do people feel like these should all be done in this PR or it's OK to have experimental methods that returns undefined (with no synchronicity guaranteed) first and leave the promise implementation to another PR

It feels like it might be a bit cleaner to get the API fleshed out in this PR? I'm worried if we decide that returning an object is nicer than writing to disk, we might end up landing a feature we immediately move away from.

I think startCoverage could land in a future PR though?

[jasnell]

It would be most helpful to have an example of these apis used together included in the docs.

[jasnell]

Just something to consider: Now that we have the experimental AbortController landed, it would make sense to start coalescing around it as a standard way of canceling things that can be canceled, including this.

For instance, if we recast this API around a CoverageController that extends from AbortController, we could have an API like:

v8.coverage.abort()   // Calling the AbortController API
v8.coverage.write()   // Write the currently collected coverage (alternative to v8.takeCoverage())

[joyeecheung]

I wonder under which circumstance would one want to abort coverage generation, though?