From 02f1e6415543315e979b9d5a0c44a4873a1e45fe Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 29 Dec 2018 18:44:11 +0800 Subject: [PATCH] process: refactor coverage setup during bootstrap - Renamed `internal/process/write-coverage.js` to `internal/coverage-gen/with_instrumentation.js`, `internal/process/coverage.js` to `internal/coverage-gen/with_profiler.js` to distinguish the two better and added comments. - Separate the coverage directory setup and the connection setup, moves the directory setup into `node.js` and closer to the exit hooks because that's where it's used. - Moves the `process.reallyExit` overwrite and `process.on('exit')` hooks setup into bootstrap/node.js for clarity, and move them to a later stage of bootstrap since they do not have to happen that early. PR-URL: https://github.com/nodejs/node/pull/25398 Reviewed-By: Ben Coe --- lib/internal/bootstrap/loaders.js | 9 +++- lib/internal/bootstrap/node.js | 46 ++++++++++++++++--- .../with_instrumentation.js} | 23 ++++------ .../with_profiler.js} | 41 +++++------------ lib/internal/process/per_thread.js | 2 +- node.gyp | 4 +- 6 files changed, 71 insertions(+), 54 deletions(-) rename lib/internal/{process/write-coverage.js => coverage-gen/with_instrumentation.js} (64%) rename lib/internal/{process/coverage.js => coverage-gen/with_profiler.js} (70%) diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js index 00776029f926f3..813ea8b82d7b4f 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/loaders.js @@ -357,7 +357,14 @@ NativeModule.prototype.cache = function() { // Coverage must be turned on early, so that we can collect // it for Node.js' own internal libraries. if (process.env.NODE_V8_COVERAGE) { - NativeModule.require('internal/process/coverage').setup(); + if (internalBinding('config').hasInspector) { + const coverage = + NativeModule.require('internal/coverage-gen/with_profiler'); + // Inform the profiler to start collecting coverage + coverage.startCoverageCollection(); + } else { + process._rawDebug('NODE_V8_COVERAGE cannot be used without inspector'); + } } function internalBindingWhitelistHas(name) { diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 365b8c3287c527..c2dbb4601833e3 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -150,13 +150,6 @@ function startup() { setupProcessStdio(getStdout, getStdin, getStderr); } - if (global.__coverage__) - NativeModule.require('internal/process/write-coverage').setup(); - - if (process.env.NODE_V8_COVERAGE) { - NativeModule.require('internal/process/coverage').setupExitHooks(); - } - if (config.hasInspector) { NativeModule.require('internal/inspector_async_hook').setup(); } @@ -311,6 +304,45 @@ function startup() { } }); + // Set up coverage exit hooks. + let originalReallyExit = process.reallyExit; + // Core coverage generation using nyc instrumented lib/ files. + // See `make coverage-build`. This does not affect user land. + // TODO(joyeecheung): this and `with_instrumentation.js` can be + // removed in favor of NODE_V8_COVERAGE once we switch to that + // in https://coverage.nodejs.org/ + if (global.__coverage__) { + const { + writeCoverage + } = NativeModule.require('internal/coverage-gen/with_instrumentation'); + process.on('exit', writeCoverage); + originalReallyExit = process.reallyExit = (code) => { + writeCoverage(); + originalReallyExit(code); + }; + } + // User-facing NODE_V8_COVERAGE environment variable that writes + // ScriptCoverage to a specified file. + if (process.env.NODE_V8_COVERAGE) { + const cwd = NativeModule.require('internal/process/execution').tryGetCwd(); + const { resolve } = NativeModule.require('path'); + // Resolve the coverage directory to an absolute path, and + // overwrite process.env so that the original path gets passed + // to child processes even when they switch cwd. + const coverageDirectory = resolve(cwd, process.env.NODE_V8_COVERAGE); + process.env.NODE_V8_COVERAGE = coverageDirectory; + const { + writeCoverage, + setCoverageDirectory + } = NativeModule.require('internal/coverage-gen/with_profiler'); + setCoverageDirectory(coverageDirectory); + process.on('exit', writeCoverage); + process.reallyExit = (code) => { + writeCoverage(); + originalReallyExit(code); + }; + } + const perf = internalBinding('performance'); const { NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE, diff --git a/lib/internal/process/write-coverage.js b/lib/internal/coverage-gen/with_instrumentation.js similarity index 64% rename from lib/internal/process/write-coverage.js rename to lib/internal/coverage-gen/with_instrumentation.js index 17da7ee609dbb7..711a3c3f12b1c8 100644 --- a/lib/internal/process/write-coverage.js +++ b/lib/internal/coverage-gen/with_instrumentation.js @@ -1,12 +1,16 @@ 'use strict'; -const path = require('path'); -const { mkdirSync, writeFileSync } = require('fs'); +// This file contains hooks for nyc instrumented lib/ files to collect +// JS coverage for core. +// See `make coverage-build`. function writeCoverage() { if (!global.__coverage__) { return; } + const path = require('path'); + const { mkdirSync, writeFileSync } = require('fs'); + const dirname = path.join(path.dirname(process.execPath), '.coverage'); const filename = `coverage-${process.pid}-${Date.now()}.json`; try { @@ -27,15 +31,6 @@ function writeCoverage() { } } -function setup() { - const reallyReallyExit = process.reallyExit; - - process.reallyExit = function(code) { - writeCoverage(); - reallyReallyExit(code); - }; - - process.on('exit', writeCoverage); -} - -exports.setup = setup; +module.exports = { + writeCoverage +}; diff --git a/lib/internal/process/coverage.js b/lib/internal/coverage-gen/with_profiler.js similarity index 70% rename from lib/internal/process/coverage.js rename to lib/internal/coverage-gen/with_profiler.js index 95235c8ac913a7..6b17073939b658 100644 --- a/lib/internal/process/coverage.js +++ b/lib/internal/coverage-gen/with_profiler.js @@ -1,4 +1,8 @@ 'use strict'; + +// Implements coverage collection exposed by the `NODE_V8_COVERAGE` +// environment variable which can also be used in the user land. + let coverageConnection = null; let coverageDirectory; @@ -48,15 +52,7 @@ function disableAllAsyncHooks() { hooks_array.forEach((hook) => { hook.disable(); }); } -exports.writeCoverage = writeCoverage; - -function setup() { - const { hasInspector } = internalBinding('config'); - if (!hasInspector) { - process._rawDebug('inspector not enabled'); - return; - } - +function startCoverageCollection() { const { Connection } = internalBinding('inspector'); coverageConnection = new Connection((res) => { if (coverageConnection._coverageCallback) { @@ -75,27 +71,14 @@ function setup() { detailed: true } })); - - try { - const { cwd } = internalBinding('process_methods'); - const { resolve } = require('path'); - coverageDirectory = process.env.NODE_V8_COVERAGE = - resolve(cwd(), process.env.NODE_V8_COVERAGE); - } catch (err) { - process._rawDebug(err.toString()); - } } -exports.setup = setup; - -function setupExitHooks() { - const reallyReallyExit = process.reallyExit; - process.reallyExit = function(code) { - writeCoverage(); - reallyReallyExit(code); - }; - - process.on('exit', writeCoverage); +function setCoverageDirectory(dir) { + coverageDirectory = dir; } -exports.setupExitHooks = setupExitHooks; +module.exports = { + startCoverageCollection, + writeCoverage, + setCoverageDirectory +}; diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js index efa736fa619387..6d64c636b24435 100644 --- a/lib/internal/process/per_thread.js +++ b/lib/internal/process/per_thread.js @@ -155,7 +155,7 @@ function wrapProcessMethods(binding) { function kill(pid, sig) { var err; if (process.env.NODE_V8_COVERAGE) { - const { writeCoverage } = require('internal/process/coverage'); + const { writeCoverage } = require('internal/coverage-gen/with_profiler'); writeCoverage(); } diff --git a/node.gyp b/node.gyp index f5dfe097fdbc20..03be5a05ea7813 100644 --- a/node.gyp +++ b/node.gyp @@ -99,6 +99,8 @@ 'lib/internal/console/constructor.js', 'lib/internal/console/global.js', 'lib/internal/console/inspector.js', + 'lib/internal/coverage-gen/with_profiler.js', + 'lib/internal/coverage-gen/with_instrumentation.js', 'lib/internal/crypto/certificate.js', 'lib/internal/crypto/cipher.js', 'lib/internal/crypto/diffiehellman.js', @@ -153,8 +155,6 @@ 'lib/internal/process/warning.js', 'lib/internal/process/worker_thread_only.js', 'lib/internal/querystring.js', - 'lib/internal/process/write-coverage.js', - 'lib/internal/process/coverage.js', 'lib/internal/queue_microtask.js', 'lib/internal/readline.js', 'lib/internal/repl.js',