Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage: expose native V8 coverage #22527

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,32 @@ Path to the file used to store the persistent REPL history. The default path is
`~/.node_repl_history`, which is overridden by this variable. Setting the value
to an empty string (`''` or `' '`) disables persistent REPL history.

### `NODE_V8_COVERAGE=dir`

When set, Node.js will begin outputting [V8 JavaScript code coverage][] to the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neither the link nor the addition here explain what to do with the output coverage file. Generally people just use the inspector UI. does the output file even have a governed format? do we have to semver major when v8 changes the format?

Copy link
Contributor Author

@bcoe bcoe Aug 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally people just use the inspector UI

I would argue it's very rare for people writing command line tools and modules to interact with the inspector, as evidenced by the fact that nyc has over 31,000 dependents and continues to grow in popularity (getting downloaded over 400,000 times a week) despite native test coverage having been available for quite some time.

As the lead maintainer of nyc and Istanbul, I would love to move people away from using these libraries when writing Node.js applications, and towards V8's coverage -- but I think the burden of kicking off and interacting with an inspector session is a usability rough-edge that will prevent adoption.

do we have to semver major when v8 changes the format?

I think you're right that we should point users to a breakdown of what to expect in the output format -- @schuay can you recommend a better document to link to.

The output format is standardized in V8, and I don't expect that there will be major breaking changes to it (and if there are it will be very rare).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I wasn't clear. I'm saying there need to be docs for the format available from the node docs, not that people should only use the inspector UI. the link you have doesn't give any docs on the format of the file.

On the semver-major front, I just want to make sure we're careful, if you think it's fine that's good enough for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's let @schuay or someone else on V8's side chime in too (@TimothyGu?), I think your concern is valid and it would be good to have someone speak to how stable the API is on the Chromium side of things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not too involved in the V8 development process… though it might be telling that the coverage API is already in the RC version of the devtools protocol without an Experimental flag on.

Copy link

@schuay schuay Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this 👍

The inspector protocol documentation here is the source of truth. In particular, takePreciseCoverage will return an array of ScriptCoverage objects.

I'm not sure what DevTools' policies on protocol changes are. You could try asking on their mailing list: https://groups.google.com/forum/#!forum/google-chrome-developer-tools. For V8 coverage specifically, we currently don't have any changes in the output format planned that I'm aware of.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally we are very careful with changing CDP due to backwards compatibility concerns. Unless unavoidable, we will not change the coverage format. If that happens however, the CDP version would be bumped as well.

directory provided as an argument. Coverage is output as an array of
[ScriptCoverage][] objects:

```json
{
"result": [
{
"scriptId": "67",
"url": "internal/tty.js",
"functions": []
}
]
}
```

`NODE_V8_COVERAGE` will automatically propagate to subprocesses, making it
easier to instrument applications that call the `child_process.spawn()` family
of functions. `NODE_V8_COVERAGE` can be set to an empty string, to prevent
propagation.

At this time coverage is only collected in the main thread and will not be
output for code executed by worker threads.

### `OPENSSL_CONF=file`
<!-- YAML
added: v6.11.0
Expand Down Expand Up @@ -691,6 +717,8 @@ greater than `4` (its current default value). For more information, see the
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[Chrome DevTools Protocol]: https://chromedevtools.github.io/devtools-protocol/
[REPL]: repl.html
[ScriptCoverage]: https://chromedevtools.github.io/devtools-protocol/tot/Profiler#type-ScriptCoverage
[V8 JavaScript code coverage]: https://v8project.blogspot.com/2017/12/javascript-code-coverage.html
[debugger]: debugger.html
[emit_warning]: process.html#process_process_emitwarning_warning_type_code_ctor
[experimental ECMAScript Module]: esm.html#esm_loader_hooks
Expand Down
8 changes: 8 additions & 0 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,14 @@ function normalizeSpawnArguments(file, args, options) {
var env = options.env || process.env;
var envPairs = [];

// process.env.NODE_V8_COVERAGE always propagates, making it possible to
// collect coverage for programs that spawn with white-listed environment.
if (process.env.NODE_V8_COVERAGE &&
!Object.prototype.hasOwnProperty.call(options.env || {},
'NODE_V8_COVERAGE')) {
env.NODE_V8_COVERAGE = process.env.NODE_V8_COVERAGE;
}

// Prototype values are intentionally included.
for (var key in env) {
const value = env[key];
Expand Down
6 changes: 6 additions & 0 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@
if (global.__coverage__)
NativeModule.require('internal/process/write-coverage').setup();

if (process.env.NODE_V8_COVERAGE) {
const { resolve } = NativeModule.require('path');
process.env.NODE_V8_COVERAGE = resolve(process.env.NODE_V8_COVERAGE);
NativeModule.require('internal/process/coverage').setup();
}

if (process.config.variables.v8_enable_inspector) {
NativeModule.require('internal/inspector_async_hook').setup();
}
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/print_help.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ const envVars = new Map([
'of stderr' }],
['NODE_REPL_HISTORY', { helpText: 'path to the persistent REPL ' +
'history file' }],
['NODE_V8_COVERAGE', { helpText: 'directory to output v8 coverage JSON ' +
'to' }],
['OPENSSL_CONF', { helpText: 'load OpenSSL configuration from file' }]
].concat(hasIntl ? [
['NODE_ICU_DATA', { helpText: 'data path for ICU (Intl object) data' +
Expand Down
71 changes: 71 additions & 0 deletions lib/internal/process/coverage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
'use strict';
const path = require('path');
const { mkdirSync, writeFileSync } = require('fs');
// TODO(addaleax): add support for coverage to worker threads.
const hasInspector = process.config.variables.v8_enable_inspector === 1 &&
require('internal/worker').isMainThread;
let inspector = null;
if (hasInspector) inspector = require('inspector');

let session;

function writeCoverage() {
if (!session) {
return;
}

const filename = `coverage-${process.pid}-${Date.now()}.json`;
try {
// TODO(bcoe): switch to mkdirp once #22302 is addressed.
mkdirSync(process.env.NODE_V8_COVERAGE);
} catch (err) {
if (err.code !== 'EEXIST') {
console.error(err);
return;
}
}

const target = path.join(process.env.NODE_V8_COVERAGE, filename);

try {
session.post('Profiler.takePreciseCoverage', (err, coverageInfo) => {
if (err) return console.error(err);
try {
writeFileSync(target, JSON.stringify(coverageInfo));
} catch (err) {
console.error(err);
}
});
} catch (err) {
console.error(err);
} finally {
session.disconnect();
session = null;
}
}

exports.writeCoverage = writeCoverage;

function setup() {
if (!hasInspector) {
console.warn('coverage currently only supported in main thread');
return;
}

session = new inspector.Session();
session.connect();
session.post('Profiler.enable');
session.post('Profiler.startPreciseCoverage', { callCount: true,
detailed: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to also expose a way to use other coverage modes? This primarily has performance implications. E.g. callCount: false would allow a function to be inlined after it has been covered. detailed: true would not require coverage-related bytecode to be emitted.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue for simplicity, exposing only the most detailed mode, and making sure it is as fast as possible. callCount: true / detailed: true should also allow inlining (it does require special bytecode emission though).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @schuay on this one, I'd lean towards starting simple. I can imagine following up with the option for more complex configuration.


const reallyReallyExit = process.reallyExit;

process.reallyExit = function(code) {
writeCoverage();
reallyReallyExit(code);
};

process.on('exit', writeCoverage);
}

exports.setup = setup;
4 changes: 4 additions & 0 deletions lib/internal/process/per_thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,10 @@ function setupKillAndExit() {

process.kill = function(pid, sig) {
var err;
if (process.env.NODE_V8_COVERAGE) {
const { writeCoverage } = require('internal/process/coverage');
writeCoverage();
}

// eslint-disable-next-line eqeqeq
if (pid != (pid | 0)) {
Expand Down
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@
'lib/internal/process/worker_thread_only.js',
'lib/internal/querystring.js',
'lib/internal/process/write-coverage.js',
'lib/internal/process/coverage.js',
'lib/internal/readline.js',
'lib/internal/repl.js',
'lib/internal/repl/await.js',
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/v8-coverage/basic.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
const a = 99;
if (true) {
const b = 101;
} else {
const c = 102;
}
7 changes: 7 additions & 0 deletions test/fixtures/v8-coverage/exit-1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const a = 99;
if (true) {
const b = 101;
} else {
const c = 102;
}
process.exit(1);
7 changes: 7 additions & 0 deletions test/fixtures/v8-coverage/sigint.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const a = 99;
if (true) {
const b = 101;
} else {
const c = 102;
}
process.kill(process.pid, "SIGINT");
5 changes: 5 additions & 0 deletions test/fixtures/v8-coverage/spawn-subprocess-no-cov.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const { spawnSync } = require('child_process');
const env = Object.assign({}, process.env, { NODE_V8_COVERAGE: '' });
spawnSync(process.execPath, [require.resolve('./subprocess')], {
env: env
});
6 changes: 6 additions & 0 deletions test/fixtures/v8-coverage/spawn-subprocess.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
const { spawnSync } = require('child_process');
const env = Object.assign({}, process.env);
delete env.NODE_V8_COVERAGE
spawnSync(process.execPath, [require.resolve('./subprocess')], {
env: env
});
8 changes: 8 additions & 0 deletions test/fixtures/v8-coverage/subprocess.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
const a = 99;
setTimeout(() => {
if (false) {
const b = 101;
} else if (false) {
const c = 102;
}
}, 10);
105 changes: 105 additions & 0 deletions test/parallel/test-v8-coverage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
'use strict';

if (!process.config.variables.v8_enable_inspector) return;

const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const path = require('path');
const { spawnSync } = require('child_process');

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

let dirc = 0;
function nextdir() {
return `cov_${++dirc}`;
}

// outputs coverage when event loop is drained, with no async logic.
{
const coverageDirectory = path.join(tmpdir.path, nextdir());
const output = spawnSync(process.execPath, [
require.resolve('../fixtures/v8-coverage/basic')
], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } });
assert.strictEqual(output.status, 0);
const fixtureCoverage = getFixtureCoverage('basic.js', coverageDirectory);
assert.ok(fixtureCoverage);
// first branch executed.
assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
// second branch did not execute.
assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0);
}

// outputs coverage when process.exit(1) exits process.
{
const coverageDirectory = path.join(tmpdir.path, nextdir());
const output = spawnSync(process.execPath, [
require.resolve('../fixtures/v8-coverage/exit-1')
], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } });
assert.strictEqual(output.status, 1);
const fixtureCoverage = getFixtureCoverage('exit-1.js', coverageDirectory);
assert.ok(fixtureCoverage, 'coverage not found for file');
// first branch executed.
assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
// second branch did not execute.
assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0);
}

// outputs coverage when process.kill(process.pid, "SIGINT"); exits process.
{
const coverageDirectory = path.join(tmpdir.path, nextdir());
const output = spawnSync(process.execPath, [
require.resolve('../fixtures/v8-coverage/sigint')
], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } });
if (!common.isWindows) {
assert.strictEqual(output.signal, 'SIGINT');
}
const fixtureCoverage = getFixtureCoverage('sigint.js', coverageDirectory);
assert.ok(fixtureCoverage);
// first branch executed.
assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1);
// second branch did not execute.
assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0);
}

// outputs coverage from subprocess.
{
const coverageDirectory = path.join(tmpdir.path, nextdir());
const output = spawnSync(process.execPath, [
require.resolve('../fixtures/v8-coverage/spawn-subprocess')
], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } });
assert.strictEqual(output.status, 0);
const fixtureCoverage = getFixtureCoverage('subprocess.js',
coverageDirectory);
assert.ok(fixtureCoverage);
// first branch executed.
assert.strictEqual(fixtureCoverage.functions[2].ranges[0].count, 1);
// second branch did not execute.
assert.strictEqual(fixtureCoverage.functions[2].ranges[1].count, 0);
}

// does not output coverage if NODE_V8_COVERAGE is empty.
{
const coverageDirectory = path.join(tmpdir.path, nextdir());
const output = spawnSync(process.execPath, [
require.resolve('../fixtures/v8-coverage/spawn-subprocess-no-cov')
], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } });
assert.strictEqual(output.status, 0);
const fixtureCoverage = getFixtureCoverage('subprocess.js',
coverageDirectory);
assert.strictEqual(fixtureCoverage, undefined);
}

// extracts the coverage object for a given fixture name.
function getFixtureCoverage(fixtureFile, coverageDirectory) {
const coverageFiles = fs.readdirSync(coverageDirectory);
for (const coverageFile of coverageFiles) {
const coverage = require(path.join(coverageDirectory, coverageFile));
for (const fixtureCoverage of coverage.result) {
if (fixtureCoverage.url.indexOf(`${path.sep}${fixtureFile}`) !== -1) {
return fixtureCoverage;
}
}
}
}