node icon indicating copy to clipboard operation
node copied to clipboard

NYC style code coverage

Open isaacs opened this issue 2 years ago • 8 comments

What is the problem this feature will solve?

With the advent of "type":"module" and .mjs files no longer supporting any level of module loading hooks that might inject coverage information from the JS layer, it seems C8 is the way to do things.

It's easy to shrug and say "oh, well, V8 has coverage built in now, we don't need to do anything". However, V8's built-in coverage does not provide an easy way to generate ergonomic reports.

But C8 exists, and can convert V8's data dump into istanbul reports, so what is still lacking?

  1. NYC tracks every process that is run, connecting the process id, argv, and a custom name to its parent process, child processes, and coverage information. C8 does not provide this information, and it is tremendously useful to test frameworks and other analytic tools.

  2. NYC allows the coverage data to be much more precisely targeted, only dumping JSON output for the files included in the report. However, C8 dumps JSON coverage and sourcemaps for every file loaded, including those from node: internal modules, packages in node_modules, etc. This results in massive data dumps, the overwhelming majority of which are completely useless.

For example, in the same exact test run, comparing nyc's data output to c8's:

$ du -h .c8_info
191M	.c8_info

$ du -h .nyc_output
204K	.nyc_output/processinfo
1.2M	.nyc_output

Well over 100x as much stuff to parse and read. It's faster to generate the coverage, but dealing with it is a bit of a nightmare.

What is the feature you are proposing to solve the problem?

Add the following flags to node:

--coverage
  Run with code coverage enabled

--coverage-output=<directory>
  the place to put the C8 style coverage json results

--coverage-include=<expression>
  Regular expression of files to track coverage on.  If set, then everything
  else is not included, by default.

--coverage-exclude=<expression>
  Regular expression of files to _not_ track coverage for.  Defaults to
  `^node:.*|^file://${cwd}/node_modules/`. if `--coverage-include` is not set.  Defaults to '.*'
  if --coverage-include is set (ie, if you provide an include list, everything else
  is excluded).  Overridden by --coverage-include (ie, if a file matches both
  it is included).

--coverage-process-info
  Output a map of process information in the coverage output directory under
  `process-info/...`.  The `process-info/index.json` file contains a reference of each
  process metadata file's externalId (if set), parent process key, and child process keys.

Coverage JSON files should only contain coverage reporting and sourcemap caches for the files that where coverage was requested. (I believe it's a single switch in the V8 invocation, so it may just need to be filtered on the way out to disk.)

This would make it possible for test frameworks like tap to gather and report coverage information, track which test files ran which source files, and all manner of other post-run analytics, without resorting to unsupported hacks.

What alternatives have you considered?

Adding processinfo tracking and output filtering to c8.

isaacs avatar Mar 07 '22 06:03 isaacs

@nodejs/modules

targos avatar Mar 07 '22 07:03 targos

I think given we've had source-map support adding a better/nicer API for this (that tools like nyc can extend and build on) is a pretty reasonable ask. I'm not sure if this belongs in core but I definitely think it's worth exploring

cc @bcoe who built most of and worked on source map support

benjamingr avatar Mar 07 '22 08:03 benjamingr

So basically the ask is:

--coverage Run with code coverage enabled --coverage-output= the place to put the C8 style coverage json results

This is to add flag options for the currently supported environment variable NODE_V8_COVERAGE=dir? (c8 uses it internally too and both it and the Node source-map support and coverage work are mostly @bcoe)

--coverage-include= Regular expression of files to track coverage on. If set, then everything else is not included, by default.

IIRC the API (Profiler.startPreciseCoverage #) doesn't actually support filtering files and they are filtered "applicatively" but Benjamin can elaborate?

benjamingr avatar Mar 07 '22 08:03 benjamingr

Yeah, this is going to require changes to V8, I think. Unfortunately, that means that I can use c8 for reporting coverage (which is lovely and parses it all out pretty much perfectly), but I can't just set NODE_V8_COVERAGE=<dir> and be done with it, or else I get dozens of GB of worthless json for each project using tap. (It's less bad if your test framework doesn't run tests in a subprocess, because then you only get deps/core covered one time instead of every time, but still pretty bad.)

Closing this for now, but we can revisit if https://bugs.chromium.org/p/v8/issues/detail?id=12702 goes anywhere, I guess.

isaacs avatar Mar 10 '22 16:03 isaacs

I'm already biting the bullet of creating a loader hook for ejs (and a cjs --require) to do processinfo tracking, so I may as well just do coverage manually instead of relying on Node to do it for me, or just edit the files on process exit, I guess.

isaacs avatar Mar 10 '22 16:03 isaacs

To avoid large files on disk, I can't see why we couldn't introduce a filtering option (I think we could do so as we write the coverage output to disk in Node.js, so need not necessarily be a change in v8).

Unfortunately, I probably don't have the cycles to work on this any time soon -- might be a good opportunity to ask for community contributions though.

bcoe avatar Mar 19 '22 16:03 bcoe

Is this worth keeping open @isaacs? I'd love to avoid userland implementations over coverage needing to open an inspector session, which motivated NODE_V8_COVERAGE.

bcoe avatar Mar 19 '22 16:03 bcoe

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] avatar Sep 21 '22 01:09 github-actions[bot]

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] avatar Oct 22 '22 01:10 github-actions[bot]