From d64aea059902efb32f01c46c9ac0467662d93628 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Fri, 22 Feb 2019 14:52:42 -0500 Subject: [PATCH 1/3] report: refactor triggerReport() This commit fixes the triggerReport() argument validation. The existing test is also updated, as it was not passing the Error object to triggerReport(). PR-URL: https://github.com/nodejs/node/pull/26268 Reviewed-By: Richard Lau Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- lib/internal/process/report.js | 24 +++++++++++------------- src/node_report_module.cc | 9 ++++----- test/node-report/test-api-pass-error.js | 4 ++-- 3 files changed, 17 insertions(+), 20 deletions(-) diff --git a/lib/internal/process/report.js b/lib/internal/process/report.js index 87fad4c1958e5a..50ddc8c4b379a5 100644 --- a/lib/internal/process/report.js +++ b/lib/internal/process/report.js @@ -75,20 +75,18 @@ const report = { }, triggerReport(file, err) { emitExperimentalWarning('report'); - if (err == null) { - if (file == null) { - return nr.triggerReport(new ERR_SYNTHETIC().stack); - } - if (typeof file !== 'string') - throw new ERR_INVALID_ARG_TYPE('file', 'String', file); - return nr.triggerReport(file, new ERR_SYNTHETIC().stack); - } - if (typeof err !== 'object') - throw new ERR_INVALID_ARG_TYPE('err', 'Object', err); - if (file == null) - return nr.triggerReport(err.stack); - if (typeof file !== 'string') + + if (typeof file === 'object' && file !== null) { + err = file; + file = undefined; + } else if (file !== undefined && typeof file !== 'string') { throw new ERR_INVALID_ARG_TYPE('file', 'String', file); + } else if (err === undefined) { + err = new ERR_SYNTHETIC(); + } else if (err === null || typeof err !== 'object') { + throw new ERR_INVALID_ARG_TYPE('err', 'Object', err); + } + return nr.triggerReport(file, err.stack); }, getReport(err) { diff --git a/src/node_report_module.cc b/src/node_report_module.cc index f56da94bac196d..0e3f8981b71aac 100644 --- a/src/node_report_module.cc +++ b/src/node_report_module.cc @@ -48,12 +48,11 @@ void TriggerReport(const FunctionCallbackInfo& info) { std::string filename; Local stackstr; - if (info.Length() == 1) { - stackstr = info[0].As(); - } else { + CHECK_EQ(info.Length(), 2); + stackstr = info[1].As(); + + if (info[0]->IsString()) filename = *String::Utf8Value(isolate, info[0]); - stackstr = info[1].As(); - } filename = TriggerNodeReport( isolate, env, "JavaScript API", __func__, filename, stackstr); diff --git a/test/node-report/test-api-pass-error.js b/test/node-report/test-api-pass-error.js index 13e92570a72ead..9bf59ec425c3a8 100644 --- a/test/node-report/test-api-pass-error.js +++ b/test/node-report/test-api-pass-error.js @@ -7,8 +7,8 @@ const assert = require('assert'); if (process.argv[2] === 'child') { try { throw new Error('Testing error handling'); - } catch { - process.report.triggerReport(); + } catch (err) { + process.report.triggerReport(err); } } else { const helper = require('../common/report.js'); From cae7232e7aa82ad57c81b55b0f8852ea3e70c5b3 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Fri, 22 Feb 2019 15:04:18 -0500 Subject: [PATCH 2/3] test: consolidate triggerReport() tests PR-URL: https://github.com/nodejs/node/pull/26268 Reviewed-By: Richard Lau Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- test/node-report/test-api-nohooks.js | 58 +++++++++++++++++-- test/node-report/test-api-pass-error.js | 30 ---------- .../test-api-trigger-with-filename.js | 29 ---------- .../test-api-trigger-with-options.js | 30 ---------- 4 files changed, 54 insertions(+), 93 deletions(-) delete mode 100644 test/node-report/test-api-pass-error.js delete mode 100644 test/node-report/test-api-trigger-with-filename.js delete mode 100644 test/node-report/test-api-trigger-with-options.js diff --git a/test/node-report/test-api-nohooks.js b/test/node-report/test-api-nohooks.js index 478586fe8d8e14..6e7195d6ab84f7 100644 --- a/test/node-report/test-api-nohooks.js +++ b/test/node-report/test-api-nohooks.js @@ -5,6 +5,8 @@ const common = require('../common'); common.skipIfReportDisabled(); const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); const helper = require('../common/report'); const tmpdir = require('../common/tmpdir'); @@ -13,7 +15,55 @@ common.expectWarning('ExperimentalWarning', 'change at any time'); tmpdir.refresh(); process.report.setOptions({ path: tmpdir.path }); -process.report.triggerReport(); -const reports = helper.findReports(process.pid, tmpdir.path); -assert.strictEqual(reports.length, 1); -helper.validate(reports[0]); + +function validate() { + const reports = helper.findReports(process.pid, tmpdir.path); + assert.strictEqual(reports.length, 1); + helper.validate(reports[0]); + fs.unlinkSync(reports[0]); + return reports[0]; +} + +{ + // Test with no arguments. + process.report.triggerReport(); + validate(); +} + +{ + // Test with an error argument. + process.report.triggerReport(new Error('test error')); + validate(); +} + +{ + // Test with a file argument. + const file = process.report.triggerReport('custom-name-1.json'); + const absolutePath = path.join(tmpdir.path, file); + assert.strictEqual(helper.findReports(process.pid, tmpdir.path).length, 0); + assert.strictEqual(file, 'custom-name-1.json'); + helper.validate(absolutePath); + fs.unlinkSync(absolutePath); +} + +{ + // Test with file and error arguments. + const file = process.report.triggerReport('custom-name-2.json', + new Error('test error')); + const absolutePath = path.join(tmpdir.path, file); + assert.strictEqual(helper.findReports(process.pid, tmpdir.path).length, 0); + assert.strictEqual(file, 'custom-name-2.json'); + helper.validate(absolutePath); + fs.unlinkSync(absolutePath); +} + +{ + // Test with a filename option. + const filename = path.join(tmpdir.path, 'custom-name-3.json'); + process.report.setOptions({ filename }); + const file = process.report.triggerReport(); + assert.strictEqual(helper.findReports(process.pid, tmpdir.path).length, 0); + assert.strictEqual(file, filename); + helper.validate(filename); + fs.unlinkSync(filename); +} diff --git a/test/node-report/test-api-pass-error.js b/test/node-report/test-api-pass-error.js deleted file mode 100644 index 9bf59ec425c3a8..00000000000000 --- a/test/node-report/test-api-pass-error.js +++ /dev/null @@ -1,30 +0,0 @@ -'use strict'; - -// Testcase for passing an error object to the API call. -const common = require('../common'); -common.skipIfReportDisabled(); -const assert = require('assert'); -if (process.argv[2] === 'child') { - try { - throw new Error('Testing error handling'); - } catch (err) { - process.report.triggerReport(err); - } -} else { - const helper = require('../common/report.js'); - const spawn = require('child_process').spawn; - const tmpdir = require('../common/tmpdir'); - tmpdir.refresh(); - const child = spawn(process.execPath, ['--experimental-report', - __filename, 'child'], - { cwd: tmpdir.path }); - child.on('exit', common.mustCall((code) => { - const report_msg = 'No reports found'; - const process_msg = 'Process exited unexpectedly'; - assert.strictEqual(code, 0, process_msg); - const reports = helper.findReports(child.pid, tmpdir.path); - assert.strictEqual(reports.length, 1, report_msg); - const report = reports[0]; - helper.validate(report); - })); -} diff --git a/test/node-report/test-api-trigger-with-filename.js b/test/node-report/test-api-trigger-with-filename.js deleted file mode 100644 index 8603c0a7c31b68..00000000000000 --- a/test/node-report/test-api-trigger-with-filename.js +++ /dev/null @@ -1,29 +0,0 @@ -'use strict'; - -// Tests when a report is triggered with a given filename. -const common = require('../common'); -common.skipIfReportDisabled(); -const filename = 'myreport.json'; -if (process.argv[2] === 'child') { - process.report.triggerReport(filename); -} else { - const helper = require('../common/report.js'); - const spawn = require('child_process').spawn; - const assert = require('assert'); - const { join } = require('path'); - const tmpdir = require('../common/tmpdir'); - tmpdir.refresh(); - - const child = spawn(process.execPath, - ['--experimental-report', __filename, 'child'], - { cwd: tmpdir.path }); - child.on('exit', common.mustCall((code) => { - const process_msg = 'Process exited unexpectedly'; - assert.strictEqual(code, 0, process_msg + ':' + code); - const reports = helper.findReports(child.pid, tmpdir.path); - assert.strictEqual(reports.length, 0, - `Found unexpected report ${reports[0]}`); - const report = join(tmpdir.path, filename); - helper.validate(report); - })); -} diff --git a/test/node-report/test-api-trigger-with-options.js b/test/node-report/test-api-trigger-with-options.js deleted file mode 100644 index a236fa1305bf46..00000000000000 --- a/test/node-report/test-api-trigger-with-options.js +++ /dev/null @@ -1,30 +0,0 @@ -'use strict'; - -// Tests when a report is triggered with options set. -const common = require('../common'); -common.skipIfReportDisabled(); -const filename = 'myreport.json'; -if (process.argv[2] === 'child') { - process.report.setOptions({ filename: filename }); - process.report.triggerReport(); -} else { - const helper = require('../common/report.js'); - const spawn = require('child_process').spawn; - const assert = require('assert'); - const { join } = require('path'); - const tmpdir = require('../common/tmpdir'); - tmpdir.refresh(); - - const child = spawn(process.execPath, - ['--experimental-report', __filename, 'child'], - { cwd: tmpdir.path }); - child.on('exit', common.mustCall((code) => { - const process_msg = 'Process exited unexpectedly'; - assert.strictEqual(code, 0, process_msg + ':' + code); - const reports = helper.findReports(child.pid, tmpdir.path); - assert.strictEqual(reports.length, 0, - `Found unexpected report ${reports[0]}`); - const report = join(tmpdir.path, filename); - helper.validate(report); - })); -} From 72cf8a3ed6400a06ed8e6dd6a07d19ecf1327695 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 23 Feb 2019 09:27:36 -0500 Subject: [PATCH 3/3] test: increase triggerReport() coverage PR-URL: https://github.com/nodejs/node/pull/26268 Reviewed-By: Richard Lau Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- test/node-report/test-api-nohooks.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/node-report/test-api-nohooks.js b/test/node-report/test-api-nohooks.js index 6e7195d6ab84f7..4ac8f1a9a528d6 100644 --- a/test/node-report/test-api-nohooks.js +++ b/test/node-report/test-api-nohooks.js @@ -67,3 +67,17 @@ function validate() { helper.validate(filename); fs.unlinkSync(filename); } + +// Test with an invalid file argument. +[null, 1, Symbol(), function() {}].forEach((file) => { + common.expectsError(() => { + process.report.triggerReport(file); + }, { code: 'ERR_INVALID_ARG_TYPE' }); +}); + +// Test with an invalid error argument. +[null, 1, Symbol(), function() {}, 'foo'].forEach((error) => { + common.expectsError(() => { + process.report.triggerReport('file', error); + }, { code: 'ERR_INVALID_ARG_TYPE' }); +});