From 60ccdf5b01d73fc94f9bc76223db582f23e3203a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 29 Dec 2018 18:44:11 +0800 Subject: [PATCH 1/2] 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. --- lib/internal/bootstrap/loaders.js | 9 +++- lib/internal/bootstrap/node.js | 42 +++++++++++++++---- .../with_instrumentation.js} | 23 ++++------ .../with_profiler.js} | 41 ++++++------------ lib/internal/process/per_thread.js | 2 +- node.gyp | 4 +- 6 files changed, 67 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 93fb186574165c..ec32d5ab9afc89 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/loaders.js @@ -356,7 +356,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 is used in a build without inspector'); + } } function internalBindingWhitelistHas(name) { diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index e229d2a2c6b246..560a1e9f8fefbc 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -158,13 +158,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(); } @@ -297,6 +290,41 @@ 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. + 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. + const coverageDireoctry = resolve(cwd, process.env.NODE_V8_COVERAGE); + process.env.NODE_V8_COVERAGE = coverageDireoctry; + const { + writeCoverage, + setCoverageDirectory + } = NativeModule.require('internal/coverage-gen/with_profiler'); + setCoverageDirectory(coverageDireoctry); + 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 a0baf052be74d9..bede306a005f5a 100644 --- a/node.gyp +++ b/node.gyp @@ -98,6 +98,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', @@ -152,8 +154,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', From 3ead96b112447d03c2d46f91bf04eb1cd138d4de Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 9 Jan 2019 21:57:48 +0800 Subject: [PATCH 2/2] fixup! process: refactor coverage setup during bootstrap --- lib/internal/bootstrap/loaders.js | 2 +- lib/internal/bootstrap/node.js | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js index ec32d5ab9afc89..36dd40fea5367c 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/loaders.js @@ -362,7 +362,7 @@ if (process.env.NODE_V8_COVERAGE) { // Inform the profiler to start collecting coverage coverage.startCoverageCollection(); } else { - process._rawDebug('NODE_V8_COVERAGE is used in a build without inspector'); + process._rawDebug('NODE_V8_COVERAGE cannot be used without inspector'); } } diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 560a1e9f8fefbc..178514b2cfc917 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -292,9 +292,11 @@ 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. + // 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 @@ -310,14 +312,16 @@ function startup() { 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. - const coverageDireoctry = resolve(cwd, process.env.NODE_V8_COVERAGE); - process.env.NODE_V8_COVERAGE = coverageDireoctry; + // 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(coverageDireoctry); + setCoverageDirectory(coverageDirectory); process.on('exit', writeCoverage); process.reallyExit = (code) => { writeCoverage();