Repository navigation
Proposal: provide a straight-forward approach for collecting coverage via Chrome's Profiler #16531
Description
Activity
@schuay gotcha, we could just configure them using
--v8-optionsit 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.
- changed the title
[-]Proposal: expose flags to enable block code coverage and type profiling[/-][+]Proposal: provide a straight-forward approach for collecting coverage via Chrome's Profiler[/+]on Oct 28, 2017 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?
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.
Reacted by Benjamin E. Coe and Jakob Linke@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).
@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.
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
--inspectoror 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:
process.exitor 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 usingspawnSyncwithinprocess.exit(), but this currently has bugs.Why this is super cool
.mjsmodule files, which is currently not possible since the new module system does not executerequire.extensions.