v8 Coverage for integrators

[Toxicable]

This is derived from the conversations had here: bcoe/c8#116
Is your feature request related to a problem? Please describe.
The problem is with intergrating V8 coverage into downstream systems such as test runners or processes which run nodejs programs.

Describe the solution you'd like
The most ideal solution would be something like

// where it comes from isn't too important
const coverage = require('some-nodejs-module-maybe?');

const collection = coverage.collectCoverage({reporters: ['text-summary', 'lcov']});
jasmine.execute();
const reports = await collection.completeCoverage();

const textSummaryReport = reports.get(''text-summary');
console.log(textSummaryReport);

cons lcovReport = reports.get('lcov');
fs.writeFileSync('to/some/location', lcovReport);
// or maybe merge/process it in some other way

However it's my understanding that this approach wouldn't work very well since internally the collectCoverage would have to activate covergae via the inspector api which isn't the best.
bcoe/c8#116 (comment)

With that in mind another approach is using the NODE_V8_COVERAGE=/some/dir env var.
However an issue came accross with this one where we need to somehow signal Nodejs to write out the coverage file since we need to process it in in the same process.
bcoe/c8#116 (comment)

Describe alternatives you've considered
The two points above

[joyeecheung]

IIUC the feature request that actually applies here is the ability to turn on coverage at run time and write out the reports at some time other than right before exiting? That should be possible with two bindings. However off the top my head, we need to notify (at least) the (subsequently spawned) child processes about the change of coverage directory after the API is invoked. One option might be to set process.env. NODE_V8_COVERAGE but that could also be too surprising.

[Toxicable]

the ability to turn on coverage at run time and write out the reports at some time other than right before exiting

Yeah I think that would solve our problem here, thanks for summing that up.
I'm not very familar with whats going on here inside Nodejs so leaning on your knowledge to come up with the best solution.

I'm not sure what you mean by the spawned child process in this situation though.
What part of this is spawning a sub process?

[bnoordhuis]

I'm not sure what you mean by the spawned child process in this situation though. What part of this is spawning a sub process?

I think @joyeecheung is talking about child processes being spawned by the script being profiled.

---

In the linked issue @bcoe says that working with the inspector API is problematic but it's not clear to me what the difficulties are. I'm inclined to say no change to Node is necessary because all the building blocks already exist:

$ grep 'command .*Coverage' deps/v8/src/inspector/js_protocol.pdl
  command getBestEffortCoverage
  command startPreciseCoverage
  command stopPreciseCoverage
  command takePreciseCoverage

@bcoe Can you expand? Maybe @SimenB can chime in too?

[bcoe]

@bnoordhuis old issues on the c8 repo are a pretty good history of why interacting with the Inspector protocol doesn't work great for test coverage:

1. it's difficult to write coverage when the process exists, because the process.exit needs to be synchronous.
2. you need to have enabled precise coverage before an isolate loads, or you will be unable to collect block coverage.
3. you now end up in a position where, rather than your application needing to set an environment variable (pretty painless), your application needs to be a harness that:

• intercepts a program starting.
• kicks off the inspector session.
• collects and processes the coverage information.

@Toxicable is the problem that you want to collect coverage for an application before it exits? I'm not fully understanding why running Node.js with an environment variable is a blocker.

[bcoe]

@SimenB brings up the need for "running tests in watch mode", which I think means you'd want coverage before the process exits?

In this case, I wonder could you run the script with NODE_V8_COVERAGE set, so that source is instrumented for block coverage immediately, and then use an inspector session like @bnoordhuis suggests?

[SimenB]

@bnoordhuis see my post here: bcoe/c8#116 (comment)

Using the inspector API is not as accurate as the env var. Running with NODE_V8_COVERAGE makes no difference. Is it because some new session is created instead of connecting to the current one? I have to admit I don't know why they differ, but point 2 brought up above here sounds like a likely suspect to me.

If I can run with NODE_V8_COVERAGE to trigger some instrumentation and access that through the inspector API, that'd work for us. The API is a bit clunky with i's callbacks and whatnot, but I'll happily go for that! More than good enough 🙂

@SimenB brings up the need for "running tests in watch mode", which I think means you'd want coverage before the process exits?

Yes - I'd like to print the coverage after each run. The process might be running for hours (I've had it going for the 8 hours I'm at work multiple times without ever exiting it). I'd also like to clear collected coverage before kicking off a new run

[Toxicable]

@Toxicable is the problem that you want to collect coverage for an application before it exits? I'm not fully understanding why running Node.js with an environment variable is a blocker.

@bcoe Being able to gather and process the report in memory, without touching the FS is the high level thing I want here

So possibly some API such as

// the process should be started with `NODE_V8_COVERAGE`
require('assert').notNull(process.env['NODE_V8_COVERAGE']);

//run some code that needs to be instrumented
jasmine.execute();

// stop the current run of coverage
const v8Report = require('inspector').send('stopPreciseCoverage');

// process the report in memory

[bnoordhuis]

@bcoe Apropos (1), calling inspector.close() from process.exit() should work. If it doesn't, please file a new issue with a test case and I'll take a look.

W.r.t. (2) and cc @SimenB: start the inspector like this to get block coverage:

const options = {callCount: true, detailed: true};
session.post('Profiler.startPreciseCoverage', options, () => { /* ... */ });

As far as I'm aware that works fine (at least with Node.js master) with an already running isolate. If it doesn't, I'd like to know what exactly doesn't work.

Is that sufficient? Is anything else needed?

[joyeecheung]

I agree with @bnoordhuis that this can be done with using the inspector protocol directly. NODE_V8_COVERAGE is also implemented with inspector but it's in core because we also want to use it to collect coverage of internals which requires the connection to be created earlier before the bootstrap, and other implications mentioned by @bcoe in #28283 (comment).

If the request is to get user land coverage on demand, then none of those issues apply here. I don't really see why it can't be done with the inspector module directly?

[SimenB]

Didn't know about those options! I'll test it out and report back.

It would be nice with a cleaner api, but that should be very much possible to build in userland