-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
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; | ||
} |
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); |
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"); |
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 | ||
}); |
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 | ||
}); |
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); |
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; | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ofScriptCoverage
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.
There was a problem hiding this comment.
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.