Repository navigation
native v8 test coverage - remaining blockers #17337
Description
Activity
@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 bygetPort()between thegetPort()call and when the childnodeinstance starts listening, which is why I generally encourage using--inspect-brk=0and 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.
@TimothyGu I kind of like the idea of listening on
stderrand 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 tostderr.@TimothyGu we're down to two blockers on this one (and one is barely a blocker):
- 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.
- 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.
Reacted by Refael AckermannReacted by Timothy Gu and Jakob Linke@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.@ofrobots yes I was looking at this initially. Unfortunately, it's too late to enable
detailedcoverage 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.
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
@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 :)
@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:
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.
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?
Reacted by F. HinkelmannThis 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.
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
Runtime.executionContextDestroyed(see Proposal: provide a straight-forward approach for collecting coverage via Chrome's Profiler #16531).Current Blockers
.mjsfiles (seebug: Runtime.executionContextDestroyed event does not fire for .mjs files #17336--inspect-brk does not work with ESM #17340).proposal: standardize header size prepended before executing scripts in v8 #17256, JS source should not be wrapped. #17396).ws://.--inspect-brk, connect to the port, etc.).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