Thoughts on c8 & Cie

[demurgos]

Hi,
I am opening this issue to share my thoughts on c8and Node coverage. I spent some time in August working on Node coverage, and I have some free time now so I am back at it.

My high level goal is to leverage V8's builtin coverage tools to get coverage for ES modules (currently behind the flag --experimental-modules).

c8 API

Using the builtin tools is not easy: it requires you to know how to interact with Node's inspector, how to enable profiling and then format the raw results to a human (or computer) readable report. This is were c8 comes in: I want it to be a library that provides an easy API to work with coverage.

c8 is currently a CLI tool only, but I would like to be able to use it as a library. My use case would be to integrate it with Gulp. I think that the CLI part should be a very thin wrapper on top of the API.

The core of the API would be a function called runWithCoverage (or similar name). It spawns an instrumented process and returns a promise for the V8 coverage. Additional functions would then let me generated reports from these coverages.

Since I want to use it as a library, I have strong expectations that the execution should be deterministic and encapsulated.
Determinism is simple: I want to have reproducible results. This means for example that random values should be avoided.
For encapsulation, it means that I can call the API multiple times and these calls do not interfere with each other. I should be able to perform multiple calls sequentially or concurrently. Encapsulation also means for me that it should avoid writing temporary files to the disk. Writing files to the disk may trigger side affects if the FS is watched, it exposes the files to be removed or modified.

Coverage properties

Here are some properties regarding coverage to compare various implementations:

Covered processes

The coverage can be either applied to a single direct child process or to a whole process subtree.
The earliest versions of c8 only supported coverage for direct node child processes. Subprocesses were not covered. This is simple, but often not enough.
The main reason why we need to support process sub-tree coverage is that some test runners (like Mocha) use child processes to run the tests.

The easiest solution is to support sub-tree coverage is to have some support in the covered process and use environment variables. Most programs (shell, Node) propagate the parent environment to the child processes so this is a clean way to pass the info. The environment variable is static though. It means that the covered process must look for it. In the latest Node version, Node looks for NODE_V8_COVERAGE to enable coverage and write the results at the end. This method breaks if a child process is created in a fresh environment, but this is pretty rare in practice.

The other solution is to watch the process subtree and intercept the creation of new processes. The library spawn-wrap can be used for this. It creates a temporary shim directory with a fake node executable. By changing the PATH or rewriting the spawn calls. This solution also depends on the environment to not be reset. It also uses some temporary files so it's not ideal. Still, this solution allows the most flexibility because you can run arbitrary code and do not need any support from the covered process.

Coverage results

Once a covered process finishes, it must return its results.
The two main ways to achieve it is via communication channels or by writing the files in an agreed-upon location.

c8@2 and c8@3 uses a temporary directory where the files are written. Each process in the subtree writes a file and at the end the root process reads all the files in the directory. It's an easy solution, but it breaks encapsulation: you cannot safely run multiple coverages concurrently if there's a risk that they use the same temporary directory.

c8@1 and my version use communication channels. Namely, the inspector API. The inspected processes are started in debug mode and the root process establishes an inspector session with them through a direct connection. Using the inspector API not only allows to get the results directly, it also allows to use the inspector to request more data (for example the module type or the source code).

Comparison

c8@1

• Can only inspect a direct Node child process.
• Uses the inspector API.
• No shim directory, no temporary directory, no dependency on the environment.

This version was a good start. It offers the cleanest implementation but the fact that it only support direct sub-process means that it is unusable with many popular test runners.

c8@2

• Can inspect process subtrees
• Uses spawn-wrap@1: for each process, starts an inspector on itself, executes main, and on exit synchronously writes the results.
• The results are written in a temporary directory, the files use uuid4 for uniqueness (caused real issues with determinism)
• Requires a shim directory and a temporary results directory.

This version enables a usable CLI tool. It's pretty great. The main issues are that it had some issues when generating the final reports (but it's a seconday issue) and that it left many files around.

c8@3

• Can inspect process subtrees.
• Uses the environment variable NODE_V8_COVERAGE and Node's builtin support for coverage.
• The results are written in a temporary directory.
• No shim directory.

The main change in this version is getting rid of spawn-wrap and its shim directories by using Node's builtin support. It still uses temporary files, but it should be possible to get implement some solution just to make sure that concurrent coverage does not interfere.

My current solution

I have a working c8 version on my next branch. It currently relies on unpublished changes in other packages but I'll try to make it available as soon as possible.

• Can inspect process subtrees
• Uses a heavily patched spawn-wrap version ("spawn-wrap@2"). Allows to establish inspector sessions with any process in a subtree.
• Uses the inspector API: no temporary files, has access to extra data.
• Uses a shim directory.

It still relies unfortunately on shim directories, but it has a direct access to the inspector API for any child process. This is extremely powerful.

The ideal solution

The ideal solution would have no shims, no temporary directories and grant full access to the inspector API.
This solution would use the inspector API and Node support to detect child processes.
This requires Node support to reverse the dependency between the inspected and inspector process. Currently, the inspected process is started first and then an inspector can connect to it. In our case, we have the opposite: a single root process and many child processes. The idea would be to start an "inspector server" and then pass its port through an environment variable. Any new process would then started in "inspected mode" and establish a connection with the root process.

This kind of the mechanism is not only useful for coverage, but it could be used by IDEs to support cross-process debugging/break points.

Comments

I hoped to quickly send a PR my c8 issues and move on to run coverage on my ESM files. I ended up going through a rabbit hole of fixes. Here are some unordered comments.

foreground-child

foreground-child is the lib used to display the output of the covered process. It proxys the I/O, IPC and signals of the root process to a child process.

My first issue with this lib is that it takes over control-flow. You can pass a handler to run some code once the child process is closed. You can retake control by not calling the end callback but the API design does not encourage this behavior. It prevents using it in a lib function called multiple times.

My second issue is that the function signature kinda mimicks child_process.spawn, but not completely:

function foregroundChild(program: string | ReadonlyArray<string>, cb?: CloseHandler): ChildProcess;
function foregroundChild(program: string, args: ReadonlyArray<string>, cb?: CloseHandler): ChildProcess;
function foregroundChild(program: string, arg1: string, cb?: CloseHandler): ChildProcess;
function foregroundChild(program: string, arg1: string, arg2: string, cb?: CloseHandler): ChildProcess;
function foregroundChild(program: string, arg1: string, arg2: string, arg3: string, cb?: CloseHandler): ChildProcess;
function foregroundChild(program: string, arg1: string, arg2: string, arg3: string, arg4: string, cb?: CloseHandler): ChildProcess;

foreground-child spawns the child process itself: you don't have control over the stream or environment variables. This is the reason why c8@3 has to mess with the global environment as a workaround.

foreground-child not only spawns the child process itself, it also uses a different spawn function based on the platform. On windows it uses cross-spawn, otherwise the builtin child_process.spawn. This is a workaround around the fact that argument parsing is basically broken for processes spawned on Windows and that you can't pass your own options.

It also has an inconsistency regarding the way it proxys IPC compared to signals. It clears all the message listeners on the parent process and does not "unproxify" its listeners. On the other hand, the signals are well handled IMO by only attaching and detaching its own listeners.

I wrote some patches to address these issues. The main change was to expose an additional function to proxy an existing process instead of spawning a new one. It means that the caller can create th child process however it wants and foreground-child.proxy just sets up the various proxys.

v8-to-istanbul

v8-to-istanbul is the lib used to convert V8 coverage to c8 coverage.

At its core, it should be a simple pure transformation from a ScriptCoverage (or FunctionCoverage[]) to an Istanbul file coverage.

v8-to-istanbul reads the files from disk itself. I don't think it should be it's responsibility. It plays badly with mocked FS (such as Gulp's Vinyl) and brings asynchronicity or blocking calls when they are not that needed.

The current version returns an istanbul report (many scripts) when converting V8 coverage for a single script. There is a mismatch.

For commonJS, there is a mismatch between the files on disk and the files evaluated by V8 due to the CJS wrapper (another reason IMO why it shouldn't read the files directly). Some users change the value of this wrapper, for example to inject "use strict" to every module. It would be nice to be able to control this behavior.

My main issue is that it uses a line-based approximation to get the number of covered functions, processes and branches. It should use syntax analysis to get proper results. This IMO the reason why I had issues with c8 (wrong results).

In my patches, I refactored v8-to-istanbul to focus on the conversion using Babel's parser for syntax analysis. Using my version I am able to get the exact same results as nyc for the fixtures in c8. It properly handles multiline statements, all kinds of functions and simple branches (I still need to add code for some of the branches like switch/case or logical operators).

spawn-wrap

Ok, this is a hack. But it can be pretty powerful.

First issue: it globally patches Node's internals. This immediately rules out any sort of concurrent calls involving child processes. You should be able to create independent wrapped instances of child_process.

The logic to rewrite the intercepted spawn calls is a bit brittle. It relies directly mutates the options and writes some internal values on it. It also uses string concatenation for path manipulation (so it has some bad results for windows). There also some issues around the handling of env variables on Windows.

The wrapper is statically configured. You can enforce some arguments and environment variables, but cannot change them based on the process being spawned (or generate a different value for each child process).

By looking at all the dependents on npm, it seems that the API does not really match the need it solves. The need is to spawn some custom module before the real main and pass some data to this custom modules. Using args and env vars is a bit contrived.

I did some major clean up on spawn-wrap. It still needs a shim directory, but appart from that I think it's pretty good. It solves the issues above. It also adds some helpers to handle the lifetime shim directory in a stack-based way.

c8

This message is already long so I won't go into the details. I just moved most of the logic to the lib part so the the cli script is just a small wrapper. I refactor the argument parsing/configuration detection so you can use it in a library. I also updated it to use my patched versions of the other libraries.
(Obviously, it gained a first class lib API)

Types

One of the main pain points when working with all these various libraries was documentation. The public API had a description in the README, but the internals had nothing. It means that finding out the real signature of foreground-child, or what are the expected fields in an Istanbul source location, or what are invariants involved in spawn-wrap's transformations was a pain.
I wrote type definitions/converted to Typescript most the libs I touched. Yeah, it adds some transpilation facilities and static checker, but the main benefit is that it pushes you to write a mostly unambiguous documentation for your functions. At the size of c8, involving all these dependencies, it pays off.

What next

I worked on an other projects in the last weeks of August/first weeks of September and couldn't submit all the PRs I had in mind. I wanted to finish them together to be sure that they all play nice with each other.
In the meantime, Node got basic builtin support for coverage and c8 was updated. It's great, but I still think that there is room for improvement.
In the long term, I truly believe that Node needs a general way to inspect subprocesses, for coverage and debuggers.
Before that, Node's builtin coverage and my lib both have value and tradeoffs. Even the c8@1 version supporting only direct child processes is valuable: it's most reliable self-contained solution if you only need to inspect a single process.
I think a good way would be to have all 3 versions available in the lib for the moment and I'd like to work on a merge request for this:

• Direct node subprocess with --inspect=0 and the inspector API
• NODE_V8_COVERAGE with temporary files
• "spawn-wrap@2" with the shim directory but no temporary files (inspector API)

Even before that, I'll work on closing my PRs for foreground-child and v8-to-istanbul.

[bcoe]

Had a conversation with @demurgos in slack; we don't quite see eye to eye around the implementation of the inspector dance specifically, but there are a lot of shared components that are important to both our visions: v8-to-istanbul, v8-coverage-merge, foreground-child.

What I think might end up happening is we split into two libraries with a lot of shared components; and with shoutouts in our corresponding READMEs.

• c8 will be a blazing fast test coverage tool built on top of NODE_V8_COVERAGE; which lets me to continue pushing forward my work around getting Node.js itself's tests instrumented (my top priority right now).
• @demurgos' yet unnamed library will manage more of the inspector dance, and will help push forward these features in Node.js (maybe this will lead to changes in the API in Node.js, and our two visions will actually come close together again).

Charles made some really thoughtful arguments about why we might want to be able to fetch more information from the inspector directly: module type, wrapper, source. Honestly makes me wonder if one solution might be extending on Node's implementation of coverage a bit more... I just need to convince Charles that temporary files aren't evil 😛

[bcoe]

let's not close this until we've got the start of a library linked to on @demurgos' end.

[demurgos]

I'm planning on releasing a tool called c88 using spawn-wrap, the inspector API and the AST. Some of my dependencies are incompatible with the design of c8 (v8-to-istanbul for example, I want to use Babel there) but many large dependencies can be shared. I opened #38 to track the updates to the shared libraries.