native v8 test coverage - remaining blockers

[bcoe]

Background

we've been working on getting it to the point where it's elegant and simple to instrument scripts executed with Node.js with v8's native test-coverage feature.

It's my hope that this could represent a future direction for the Istanbul project (the tool that's currently most commonly used for facilitating test coverage for Node.js). Istanbul currently uses babel for instrumenting source-files; I believe it will make it easier to keep up with the evolving JavaScript language if we move to using the v8 engine itself.

I've created this issue to track the progress on this project, as it relates to Node.js.

Work So Far

• we need a way to collect coverage regardless of how a process exits:
  • we are now able to do this using the event Runtime.executionContextDestroyed (see Proposal: provide a straight-forward approach for collecting coverage via Chrome's Profiler #16531).
• we need to be able to collect coverage for binary and conditional operations (users have come to expect this):
  • we still have some work on the go, but we're now instrumenting most of what folks would expect (https://chromium-review.googlesource.com/c/v8/v8/+/754564/1, https://chromium-review.googlesource.com/c/v8/v8/+/568307).

Current Blockers

• the approach outlined in Proposal: provide a straight-forward approach for collecting coverage via Chrome's Profiler #16531 does not appear to work for .mjs files (see bug: Runtime.executionContextDestroyed event does not fire for .mjs files #17336 --inspect-brk does not work with ESM #17340).
• we need to take into account origin line and origin column when calculating test coverage offsets (see proposal: standardize header size prepended before executing scripts in v8 #17256, JS source should not be wrapped. #17396).
  • there's currently work in progress on this (https://bugs.chromium.org/p/v8/issues/detail?id=7119, https://chromium-review.googlesource.com/c/v8/v8/+/789373).
• we need a way to run the inspector in silent mode.
  • the inspector currently spits out quite a bit of text to the terminal when running, this is bad for tests that rely on terminal output, e.g., tests outputting tap format.
  • I'd be happy to take on this work, @addaleax, @bmeck is there a precedent for how I might toggle this feature? perhaps an environment variable or flag?
• when booting a subprocess with the inspector enabled, how do we know when it's actually bound to its port?
  • I've been using a library that polls on the port, but this feels like a hack; any other suggestions, thoughts? tldr; I would like to be able to spawn a subprocess in such a way that I know when it's safe to connect on the inspector ws://.
• spawn-wrap will need modifications to support the new inspector dance (spawn a subprocess with --inspect-brk, connect to the port, etc.).
  • @isaacs I know you have some thoughts around this.

I've been updating and maintaining the module c8 as we work on this feature (if you feel like playing, it's already partially functional).

👋 thanks a ton for everyone's help so far (as you may have noticed, I'm really excited about this project).

CC: @schuay, @bmeck, @hashseed, @ak239, @mikeal, @isaacs

[TimothyGu]

@bcoe thanks so much for all your work on performant test coverage with V8! Some thoughts below.

the inspector currently spits out quite a bit of text to the terminal when running, this is bad for tests that rely on terminal output, e.g., tests outputting tap format.

FWIW the inspector emits messages only to stderr, while I would expect TAP output etc. to use stdout. But I agree, having a mode that disables such behavior would be ideal.

when booting a subprocess with the inspector enabled, how do we know when it's actually bound to its port? I've been using a library that polls on the port, but this feels like a hack; any other suggestions, thoughts?

I would've suggested sniffing stderr for Debugger listening on ws://, but as you've mentioned it's bad idea for this application, polling seems to be the best solution for now. But it's important to realize that a race condition could result when another program starts listening on the port number returned by getPort() between the getPort() call and when the child node instance starts listening, which is why I generally encourage using --inspect-brk=0 and then sniffing stderr for the exact port number.

In the perfect world, we would send the listening info to any parent process through IPC. Not sure how plausible that is though; @addaleax and/or @eugeneo should be able to better answer this question.

[bcoe]

@TimothyGu I kind of like the idea of listening on stderr and parsing the port that gets selected by --inspect-brk=0 (unless @addaleax/@eugeneo come back with a better suggestion. It feels like this would be less breakable, once we start talking about running a test suite with 100s of files.

one idea: perhaps we could add a common prefix to all the debugger output to stderr, e.g., debugger: attached, debugger: listening on ws://127.0.0.1:61269/5c73249e-5aff-41ce-92b7-3352de756e43. This would make it easier to strip out the debugger specific messages, and you could in turn print everything else out to stderr.

[bcoe]

@TimothyGu we're down to two blockers on this one (and one is barely a blocker):

1. suppressing the debugger log messages (CC: @addaleax).

• I currently rely on the log message that indicates the port #, but the others end up in the terminal.
• we could potentially work around this with a list of known debug messages, which we simply skip outputting; I'd love to think of something less hacky though.

2. I'd love to be able to hook subprocess with spawn-wrap; I haven't looked into how we'd do this logistically yet.

Once we have something that's fully functional, a thought comes to mind; it would be interesting to experiment with using this approach on the Node.js project itself (CC: @refack, @addaleax). Given that the coverage information is now injected into v8 bytecode, I expect it will be significantly faster (we might be able to run with coverage real-time). If it's not fast enough, @schuay has some thoughts about optimizations we could attempt.

[ofrobots]

@bcoe have you considered using the in-process inspector JS api rather than using spawn? See https://nodejs.org/dist/latest-v9.x/docs/api/inspector.html.

[bcoe]

@ofrobots yes I was looking at this initially. Unfortunately, it's too late to enable detailed coverage by the time you're able to pause the inspector (the isolate has already been created, and the source would need to be reprocessed to add block level coverage).

I know @schuay was investigating making it possible to enable block level coverage post-hoc, but it's not a small task -- one random idea, I wonder if we could pass a flag to v8 that told it to enable detailed coverage for any inspector sessions that are spawned.

[refack]

Giving it a whirl.

RE using the inspector api: is it an intrinsic limitation of the api or just of the triggering implementation in node? Because having a way to inspect JS from actualy the first line could be useful

[schuay]

@refack there was some discussion at https://crbug.com/v8/7015#c3 about the startup event when using the in-process API. IIRC the event is currently fired too late but it should be possible to tweak the timing.

I know @schuay was investigating making it possible to enable block level coverage post-hoc, but it's not a small task -- one random idea, I wonder if we could pass a flag to v8 that told it to enable detailed coverage for any inspector sessions that are spawned.

Right, we have ideas for reload-less block coverage, but we don't have plans to spend time on this for now. That might change if we find out the current implementation does not support important use-cases, but that currently seems fine.

We've already discussed adding a flag. I agree it'd be convenient in this case, but the intention is to make the entire workflow possible using only the inspector protocol. At the moment this requires jumping through a few hoops (in-process API event fires too late, issues around script offsets), but people are working on both and this should improve over time.

Awesome to see progress on this :)

[bcoe]

@refack just getting back in the swing of things post break. I've switched back to starting the inspector programmatically and everything seems to be working with spawn-wrap:

https://github.com/bcoe/c8

I was a bit surprised that things worked, because @schuay had indicated to me that you could not enable block-level coverage once a script was already parsed ... but, I think with the spawn-wrap flow, we're enabling the inspector before we require any dependencies so, block-level coverage is enabled before any application code is loaded.

To actually get things working for real, we'll need to get a newer version of v8 in Node.js or cherry-pick commits related to coverage. Any thoughts on how we should approach this (CC: @addaleax, @TimothyGu).

I intend to continue submitting some small patches to polish the coverage work (there are a few source-ranges that are a bit weird, etc), but these should be small enough changes that we could simply cherry-pick.

[addaleax]

To actually get things working for real, we'll need to get a newer version of v8 in Node.js or cherry-pick commits related to coverage. Any thoughts on how we should approach this

Just to quickly answer that: We do have https://github.com/nodejs/node-v8 as the repo for Node’s canary branch, so if you want to work on a recent version of V8, that might be a good place to start?

[bcoe]

This issue is a bit stale, and covers a lot of topics. I'm going to re-open another issue, and potentially write an RFC, discussing how we'd better support native test coverage without a tool like c8.