Proposal: provide a straight-forward approach for collecting coverage via Chrome's Profiler

[bcoe]

The Problem

I've been having an ongoing discussion with @bmeck and a few folks working on the V8 project (@schuay, @fhinkel) about how to move istanbul towards using v8's built-in coverage.

The biggest blocker, tracked in the v8 issue tracker here, is that we need a way to start Node with detailed coverage enabled. -- @schuay can speak better to the specifics, but as I understand it code needs to be put through a preprocessing step that introduces counters, currently we're only able to collect function-level coverage.

There are a few other blockers that I will also outline in this issue.

Collecting Detailed Coverage

• @schuay proposes introducing a v8 flag that could be passed to Node when starting, and would run inspector with detailed reporting enabled; does this mean that we'd need to stat Node.js with --inspector or could we still enable this post-hoc?

Other Challenges

There are a few other technical challenges that will need to be addressed, to make the switch over to v8's test coverage:

• if an application exits using process.exit or by throwing an exception, it's hard to capture this event and output the coverage information (since talking to the inspector's socket is asynchronous); One option might be using spawnSync within process.exit(), but this currently has bugs.
• v8's coverage output is significantly different than Itanbul's coverage.json format we'll need to write something that translates between the formats.

Why this is super cool

• transpilation to add coverage counters is complex and error-prone, it would be neat to have this happen at the v8 level.
• code transpiled for test coverage runs significantly slower than the code prior to transpilation; I'm betting collecting coverage directly from v8 will be fast (although the subprocess shenanigans might add some overhead).
• this would give us a way to collect coverage for .mjs module files, which is currently not possible since the new module system does not execute require.extensions.

[schuay]

Just so we're on the same page: these would be flags added in V8, and AFAIK no action is necessary by Node itself.

cc @hashseed

[bcoe]

@schuay gotcha, we could just configure them using --v8-options it would look like.

I might keep this ticket open for a bit, and rework it a bit today, so that it instead better tracks some of the other challenges making it difficult to use v8's coverage instrumenter for an arbitrary Node.js process.

[bcoe]

an update:

issue #715 has been created on the v8 project, proposing introducing startup & teardown events to the inspector. The idea being that:

• during the startup event the inspector could have flags set for configuring detailed coverage.
• during teardown, a hook could be installed that fetched the final output from the inspector (in the case of Istanbul, this hook could be used to output coverage information).

@eugeneo, @TimothyGu, @jasnell, @Trott, having done a significant amount of work on the inspector; any thoughts about how these hooks would be integrated into Node.js' inspector lifecycle? could we figure out an elegant way to provide a closure that executes when the inspector has finished processing a given script in v8?

[alexkozy]

I prefer solution without flags to make possible coverage and type profiling of web page and Node.js instances using the same protocol.
So imagine following solution:

• run node with --inspect-brk flag;
• call Target.setAutoAttach({waitForDebuggerOnStart: true}) - not implemented yet, @eugeneo is working to support it (Target.targetCreated is our startup event),
• call Profiler.startPreciseCoverage and Runtime.runIfWaitingForDebugger for each target including main,
• query coverage data by Profiler.takePreciseCoverage on Runtime.executionContextDestroyed for main context (is our teardown event).

It definitely won't work as is and we need to tune it a little but it should be fixable.

[bcoe]

@ak239 amazing \o/ thanks for the work you're undertaking on v8's end to help implement this feature. I'm excited to start moving towards built-in coverage, it will help dismantle the Rube-Goldberg machine that currently facilitates most people's JavaScript test-coverage. Please feel free to loop me into patches on the v8 side of things (I would love to be a fly on the wall 😛).

As discussed in puppeteer/puppeteer#1054, one feature request that I think the community has become accustomed to and will immediately request is branch-level coverage -- would this potentially be something we could move towards eventually in v8 (CC: @schuay, @mikeal).

[schuay]

@ak239 sounds great, let's work out the details to be able to enable coverage before any scripts are parsed!

As discussed in puppeteer/puppeteer#1054, one feature request that I think the community has become accustomed to and will immediately request is branch-level coverage -- would this potentially be something we could move towards eventually in v8 (CC: @schuay, @mikeal).

Can you clarify what you mean by branch-level coverage? AFAIK the linked issue discusses providing coverage information for 'a || b' constructs, which in my mind are still block coverage (i.e. a counter for each block, whatever the definition of 'block' is) and not branch coverage (a counter for each branch).

We can definitely extend support to 'a || b' constructs. But since our implementation is based on block counters, branch coverage support is less likely.

[bcoe]

I've been doing some testing with Node.js on canary versions of v8, with a few caveats that I will discuss in other tickets, the approach outlined by @ak239 works like a charm.