From f760bc363b9fd5972c125e4ff7f8473039c12ec4 Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Sat, 14 Sep 2019 05:39:46 +0530 Subject: [PATCH 1/5] bootstrap: adds exception handling for profiler bootstrap exception handling for the case when profile is not bootstrapped when coverage is enabled. Fixes: https://github.com/nodejs/node/issues/29542 --- lib/internal/bootstrap/pre_execution.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index d18d7099280f62..29f217c722de9f 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -120,8 +120,13 @@ function setupCoverageHooks(dir) { const { resolve } = require('path'); const coverageDirectory = resolve(cwd, dir); const { sourceMapCacheToObject } = require('internal/source_map'); - internalBinding('profiler').setCoverageDirectory(coverageDirectory); - internalBinding('profiler').setSourceMapCacheGetter(sourceMapCacheToObject); + + try { + internalBinding('profiler').setCoverageDirectory(coverageDirectory); + internalBinding('profiler').setSourceMapCacheGetter(sourceMapCacheToObject); + } catch { + process.emitWarning('Profiler is not enabled.', 'Warning'); + } return coverageDirectory; } From c18a0dde1523a69eb644376816bb2b1939d3886a Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Sun, 15 Sep 2019 12:43:47 +0530 Subject: [PATCH 2/5] bootstrap: updates the warning and adds test --- lib/internal/bootstrap/pre_execution.js | 4 +++- test/fixtures/v8-coverage/subprocess.js | 2 ++ .../test-coverage-with-inspector-disabled.js | 23 +++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-coverage-with-inspector-disabled.js diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 29f217c722de9f..f99383b0f1461c 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -125,7 +125,9 @@ function setupCoverageHooks(dir) { internalBinding('profiler').setCoverageDirectory(coverageDirectory); internalBinding('profiler').setSourceMapCacheGetter(sourceMapCacheToObject); } catch { - process.emitWarning('Profiler is not enabled.', 'Warning'); + process.emitWarning('The inspector is disabled, ' + + 'coverage could not be collected', + 'Warning'); } return coverageDirectory; } diff --git a/test/fixtures/v8-coverage/subprocess.js b/test/fixtures/v8-coverage/subprocess.js index e363c7614e7f73..c7b80d1643d1e7 100644 --- a/test/fixtures/v8-coverage/subprocess.js +++ b/test/fixtures/v8-coverage/subprocess.js @@ -6,3 +6,5 @@ setTimeout(() => { const c = 102; } }, 10); + +process.on('warning', console.log); \ No newline at end of file diff --git a/test/parallel/test-coverage-with-inspector-disabled.js b/test/parallel/test-coverage-with-inspector-disabled.js new file mode 100644 index 00000000000000..a6bda301e1da3f --- /dev/null +++ b/test/parallel/test-coverage-with-inspector-disabled.js @@ -0,0 +1,23 @@ +'use strict'; + +require('../common'); +const fixtures = require('../common/fixtures'); + +const assert = require('assert'); +const { spawnSync } = require('child_process'); +const env = Object.assign({}, process.env, { NODE_V8_COVERAGE: '/foo/bar' }); +const childPath = fixtures.path('v8-coverage/subprocess'); +const { status, stderr } = spawnSync( + process.execPath, + [childPath], + { env: env } +); + +const warningMessage = 'The inspector is disabled, ' + + 'coverage could not be collected'; + +assert.strictEqual(status, 0); +assert.strictEqual( + stderr.toString().includes(`Warning: ${warningMessage}`), + true +); From 8a7be6ba33232bc6b4127624aebaffc74be2845d Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Sun, 15 Sep 2019 12:45:26 +0530 Subject: [PATCH 3/5] bootstrap: return empty string when no cov --- lib/internal/bootstrap/pre_execution.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index f99383b0f1461c..47f8ef9d8b4693 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -128,6 +128,7 @@ function setupCoverageHooks(dir) { process.emitWarning('The inspector is disabled, ' + 'coverage could not be collected', 'Warning'); + return ''; } return coverageDirectory; } From 180959ecfe51cf5f6f45e34f49d4d6cc6fe0159f Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Sun, 15 Sep 2019 18:48:34 +0530 Subject: [PATCH 4/5] bootstrap: refactor + skip test on inspector enabled --- test/common/index.js | 7 +++++++ test/fixtures/v8-coverage/subprocess.js | 2 -- test/parallel/test-coverage-with-inspector-disabled.js | 9 +++++---- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 98a26872223cb9..00ebd283a0c3e9 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -651,6 +651,12 @@ function skipIfInspectorDisabled() { } } +function skipIfInspectorEnabled() { + if (process.features.inspector) { + skip('V8 inspector is enabled'); + } +} + function skipIfReportDisabled() { if (!process.config.variables.node_report) { skip('Diagnostic reporting is disabled'); @@ -783,6 +789,7 @@ module.exports = { skipIf32Bits, skipIfEslintMissing, skipIfInspectorDisabled, + skipIfInspectorEnabled, skipIfReportDisabled, skipIfWorker, diff --git a/test/fixtures/v8-coverage/subprocess.js b/test/fixtures/v8-coverage/subprocess.js index c7b80d1643d1e7..e363c7614e7f73 100644 --- a/test/fixtures/v8-coverage/subprocess.js +++ b/test/fixtures/v8-coverage/subprocess.js @@ -6,5 +6,3 @@ setTimeout(() => { const c = 102; } }, 10); - -process.on('warning', console.log); \ No newline at end of file diff --git a/test/parallel/test-coverage-with-inspector-disabled.js b/test/parallel/test-coverage-with-inspector-disabled.js index a6bda301e1da3f..0b0c2aea43fa60 100644 --- a/test/parallel/test-coverage-with-inspector-disabled.js +++ b/test/parallel/test-coverage-with-inspector-disabled.js @@ -1,16 +1,17 @@ 'use strict'; -require('../common'); -const fixtures = require('../common/fixtures'); +const common = require('../common'); +common.skipIfInspectorEnabled(); +const fixtures = require('../common/fixtures'); const assert = require('assert'); const { spawnSync } = require('child_process'); -const env = Object.assign({}, process.env, { NODE_V8_COVERAGE: '/foo/bar' }); +const env = { ...process.env, NODE_V8_COVERAGE: '/foo/bar' }; const childPath = fixtures.path('v8-coverage/subprocess'); const { status, stderr } = spawnSync( process.execPath, [childPath], - { env: env } + { env } ); const warningMessage = 'The inspector is disabled, ' + From bbb315d9b5654e13e62526cb275a221092cbaa87 Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Mon, 16 Sep 2019 11:47:04 +0530 Subject: [PATCH 5/5] bootstrap: removes try-caatch and check for inspector --- lib/internal/bootstrap/pre_execution.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 47f8ef9d8b4693..6e7d946458d44e 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -121,10 +121,10 @@ function setupCoverageHooks(dir) { const coverageDirectory = resolve(cwd, dir); const { sourceMapCacheToObject } = require('internal/source_map'); - try { + if (process.features.inspector) { internalBinding('profiler').setCoverageDirectory(coverageDirectory); internalBinding('profiler').setSourceMapCacheGetter(sourceMapCacheToObject); - } catch { + } else { process.emitWarning('The inspector is disabled, ' + 'coverage could not be collected', 'Warning');