feat: switch to using Node's built in coverage

[bcoe]

switches to using Node's built in coverage, which we can enable by setting NODE_V8_COVERAGE rather than kicking off an inspector session.

I'm beyond thrilled by how much better this implementation is working.

BREAKING CHANGE: switches to using NODE_V8_COVERAGE

CC: @demurgos, @schuay

[bcoe]

CC: @addaleax, @bengl, @boneskull, @hashseed, @evanlucas

[bcoe]

this will be required for the Node.js codebase itself.

[bcoe]

see also https://github.com/bcoe/v8-coverage-merge, which handles merging coverage from multiple inspector sessions (this will be necessary for Node.js' test suite).

[bcoe]

@TimothyGu any thoughts about how to easily tell whether the coverage outputted was for one of Node.js' built in libraries, e.g., internal/inspector.

[addaleax]

How about checking whether the URL was an absolute path or not?

[demurgos]

This sounds great but I'll have to take a deep look at it.
It's nice to have a solution natively supported by Node, but using the inspector session also has advantages.
spawn-wrap is awful but it enables the cleanest c8 implementation IMO: no temporary files, no concurrence issues (can run multiple coverage at once), enables a Node API (lib usage), no messing with exit handlers, etc. (demurgos/c8#next)
Using the env variable (NODE_V8_COVERAGE) is more reliable than spawn wrap, but it brings back a few issues (mainly the fact that the c8 cli and covered process cannot communicate directly but through files stored in a temporary directory).

(This is just a quick comment after reading the first message and checking the Node PR to add NODE_V8_COVERAGE, I did not look at the details yet.)

[bcoe]

@demurgos spawn-wrap and signal-exit are essentially end of life and will not have much support any longer.

Behind the scenes Node.js is kicking off the same inspector session that c8 was spawning, but in a much more reliable manner because it happens before the isolate executes and there is no need to pause the inspector; the inspector dance was bug prone, didn’t work for a variety of edge-cases (non-zero application exit codes being a major issue, which c8 handled even worse than nyc).

With regards to concurrency, coverage files are written by Node.js with a unique-identifier and c8 merges output from multiple subprocesses, so this will not be an issue.

With regards to temporary files, this isn’t an issue in my opinion: nyc, c8, jest, etc., already use temporary files during the process of aggregating test coverage. One lovely side-effect of retiring spawn-wrap is that we actually eliminate the need for temporary binary files; spawn wrap facilitates its behavior by writing temporary binaries to disk that are executed rather than process.execPath.

I recommend forking c8, if you’d like to continue interacting with the inspector directly in some cases — maybe with a more explicit name like “inspector-coverage”; I’d happily link to this in the README.

[bcoe]

@demurgos I'm going to take my thoughts to email with you, let's work together to make sure your hard work on the next branch of c8 isn't taken for granted. Even though on c8 itself, I'd like to migrate away from using this approach for test coverage.

[addaleax]

Looks very nice! :)

[addaleax]

How about checking whether the URL was an absolute path or not?

[bcoe]

I need this for some Node.js mentorship work I'm doing tonight, so I've gone ahead and landed. I'm still open to more conversation about whether we maintain the old approach in another library.