From 8f61ca9f03e7ee5f7d51946d64170e8d081b4d8c Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Wed, 21 Dec 2016 07:44:05 +0530 Subject: [PATCH 1/4] test: fix and improve debugger-client test This test expects the string 'Debugger listening on port' on stderr and since the message has been changed to 'Debugger listening on host:port' this was failing always. Apart from that, this test expects the main script's name to be `src/node.js`, but that has been renamed to `lib/internal/node.js` and then to `lib/internal/bootstrap_node.js`. So, the script name has been updated. Apart from that, using `const` instead of `var` wherever possible. Refer: https://github.com/nodejs/node/issues/10361 PR-URL: https://github.com/nodejs/node/pull/10371 Reviewed-By: Colin Ihrig Reviewed-By: Sam Roberts Reviewed-By: Rich Trott Reviewed-By: James M Snell --- test/debugger/test-debugger-client.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/debugger/test-debugger-client.js b/test/debugger/test-debugger-client.js index 04823113ec32c1..869dabc6ef0c43 100644 --- a/test/debugger/test-debugger-client.js +++ b/test/debugger/test-debugger-client.js @@ -16,7 +16,7 @@ setTimeout(function() { var resCount = 0; var p = new debug.Protocol(); -p.onResponse = function(res) { +p.onResponse = function() { resCount++; }; @@ -89,7 +89,7 @@ function addTest(cb) { addTest(function(client, done) { console.error('requesting version'); client.reqVersion(function(err, v) { - assert.ok(!err); + assert.ifError(err); console.log('version: %s', v); assert.strictEqual(process.versions.v8, v); done(); @@ -99,13 +99,13 @@ addTest(function(client, done) { addTest(function(client, done) { console.error('requesting scripts'); client.reqScripts(function(err) { - assert.ok(!err); + assert.ifError(err); console.error('got %d scripts', Object.keys(client.scripts).length); var foundMainScript = false; for (var k in client.scripts) { var script = client.scripts[k]; - if (script && script.name === 'node.js') { + if (script && script.name === 'bootstrap_node.js') { foundMainScript = true; break; } @@ -119,7 +119,7 @@ addTest(function(client, done) { console.error('eval 2+2'); client.reqEval('2+2', function(err, res) { console.error(res); - assert.ok(!err); + assert.ifError(err); assert.strictEqual(res.text, '4'); assert.strictEqual(res.value, 4); done(); @@ -137,7 +137,7 @@ function doTest(cb, done) { var args = ['--debug=' + debugPort, '-e', script]; nodeProcess = spawn(process.execPath, args); - nodeProcess.stdout.once('data', function(c) { + nodeProcess.stdout.once('data', function() { console.log('>>> new node process: %d', nodeProcess.pid); var failed = true; try { @@ -158,7 +158,7 @@ function doTest(cb, done) { console.error('got stderr data %j', data); nodeProcess.stderr.resume(); b += data; - if (didTryConnect === false && b.match(/Debugger listening on port/)) { + if (didTryConnect === false && b.match(/Debugger listening on /)) { didTryConnect = true; // The timeout is here to expose a race in the bootstrap process. @@ -168,10 +168,10 @@ function doTest(cb, done) { function tryConnect() { // Wait for some data before trying to connect - var c = new debug.Client(); + const c = new debug.Client(); console.error('>>> connecting...'); c.connect(debug.port); - c.on('break', function(brk) { + c.on('break', function() { c.reqContinue(function() {}); }); c.on('ready', function() { @@ -199,7 +199,7 @@ function doTest(cb, done) { function run() { - var t = tests[0]; + const t = tests[0]; if (!t) return; doTest(t, function() { From fd04f9a1bd28216cd15d6e37d9192e185a80c558 Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Mon, 26 Dec 2016 13:52:31 +0530 Subject: [PATCH 2/4] test: refactor test-debugger-remote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. The test doesn't attach an event listener for `exit` events and removes them before killing. The intention is to fail the tests if the processes exit normally. This patch attaches the `exit` event handlers. 2. Replace `var`s with `let`s and `const`s. 3. Replace `==` based assertion with `strictEqual` assertion. 4. Use `common.PORT` instead of `5959`. 5. The test used to expect only one string "connecting to localhost:5959 ... ok", but the debugger actually emits another string, "break in test/fixtures/empty.js:2". This patch asserts if both of them are received in the same order. Refer: https://github.com/nodejs/node/issues/10361 PR-URL: https://github.com/nodejs/node/pull/10455 Reviewed-By: James M Snell Reviewed-By: Michaël Zasso --- test/debugger/test-debugger-remote.js | 52 ++++++++++++++++----------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/test/debugger/test-debugger-remote.js b/test/debugger/test-debugger-remote.js index f5232dce9c8df4..fb79fb83ee733f 100644 --- a/test/debugger/test-debugger-remote.js +++ b/test/debugger/test-debugger-remote.js @@ -1,25 +1,31 @@ 'use strict'; -var common = require('../common'); -var assert = require('assert'); -var spawn = require('child_process').spawn; +const common = require('../common'); +const assert = require('assert'); +const spawn = require('child_process').spawn; +const path = require('path'); -var buffer = ''; -var scriptToDebug = common.fixturesDir + '/empty.js'; - -function fail() { - assert(0); // `--debug-brk script.js` should not quit -} +const PORT = common.PORT; +const scriptToDebug = path.join(common.fixturesDir, 'empty.js'); // running with debug agent -var child = spawn(process.execPath, ['--debug-brk=5959', scriptToDebug]); - -console.error(process.execPath, '--debug-brk=5959', scriptToDebug); +const child = spawn(process.execPath, [`--debug-brk=${PORT}`, scriptToDebug]); // connect to debug agent -var interfacer = spawn(process.execPath, ['debug', 'localhost:5959']); - -console.error(process.execPath, 'debug', 'localhost:5959'); +const interfacer = spawn(process.execPath, ['debug', `localhost:${PORT}`]); interfacer.stdout.setEncoding('utf-8'); + +// fail the test if either of the processes exit normally +const debugBreakExit = common.fail.bind(null, 'child should not exit normally'); +const debugExit = common.fail.bind(null, 'interfacer should not exit normally'); +child.on('exit', debugBreakExit); +interfacer.on('exit', debugExit); + +let buffer = ''; +const expected = [ + `\bconnecting to localhost:${PORT} ... ok`, + '\bbreak in test/fixtures/empty.js:2' +]; +const actual = []; interfacer.stdout.on('data', function(data) { data = (buffer + data).split('\n'); buffer = data.pop(); @@ -30,22 +36,26 @@ interfacer.stdout.on('data', function(data) { interfacer.on('line', function(line) { line = line.replace(/^(debug> *)+/, ''); - console.log(line); - var expected = '\bconnecting to localhost:5959 ... ok'; - assert.ok(expected == line, 'Got unexpected line: ' + line); + if (expected.includes(line)) { + actual.push(line); + } }); // allow time to start up the debugger setTimeout(function() { - child.removeListener('exit', fail); + // remove the exit handlers before killing the processes + child.removeListener('exit', debugBreakExit); + interfacer.removeListener('exit', debugExit); + child.kill(); - interfacer.removeListener('exit', fail); interfacer.kill(); -}, 2000); +}, common.platformTimeout(2000)); process.on('exit', function() { + // additional checks to ensure that both the processes were actually killed assert(child.killed); assert(interfacer.killed); + assert.deepStrictEqual(actual, expected); }); interfacer.stderr.pipe(process.stderr); From 94cbec6b6cb531a87d7878762b206e10dddba2a8 Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Wed, 4 Jan 2017 00:54:27 +0530 Subject: [PATCH 3/4] test: fix process.title expectation `process.title` would work properly only in FreeBSD, OSX, and Linux as per test/parallel/test-setproctitle.js. This patch makes sure that the test expects an empty string in other platforms. This patch helps fix the SmartOS failures in https://ci.nodejs.org/job/node-test-commit/6962/ for https://github.com/nodejs/node/pull/10456 PR-URL: https://github.com/nodejs/node/pull/10597 Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: Rich Trott Reviewed-By: Evan Lucas --- test/debugger/test-debugger-repl.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/debugger/test-debugger-repl.js b/test/debugger/test-debugger-repl.js index 8a87d40d163af7..ab0468448bef4d 100644 --- a/test/debugger/test-debugger-repl.js +++ b/test/debugger/test-debugger-repl.js @@ -1,5 +1,5 @@ 'use strict'; -require('../common'); +var common = require('../common'); var repl = require('./helper-debugger-repl.js'); repl.startDebugger('breakpoints.js'); @@ -57,7 +57,7 @@ addTest('c', [ // Execute addTest('exec process.title', [ - /node/ + common.isFreeBSD || common.isOSX || common.isLinux ? /node/ : '' ]); // Execute From 4ea2f2aa286ddd51c377c615d7a1948746f4752d Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Mon, 16 Jan 2017 00:32:32 +0530 Subject: [PATCH 4/4] test: increase timeout in break-on-uncaught As the failures suggest, this test expects the uncaught exception to be thrown within 100 milliseconds, but on some of the test machines it takes longer than that limit to notify the exception. Thats why the test was failing. This patch polls every 10 ms to see if the exception is received. PR-URL: https://github.com/nodejs/node/pull/10822 Reviewed-By: Rich Trott Reviewed-By: Santiago Gimeno --- test/debugger/test-debug-break-on-uncaught.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/debugger/test-debug-break-on-uncaught.js b/test/debugger/test-debug-break-on-uncaught.js index dc380fa0193fb1..cfe72bb67b4f02 100644 --- a/test/debugger/test-debug-break-on-uncaught.js +++ b/test/debugger/test-debug-break-on-uncaught.js @@ -73,6 +73,7 @@ function runScenario(scriptName, throwsOnLine, next) { client.connect(port); } + let interval; function runTest(client) { client.req( { @@ -91,19 +92,22 @@ function runScenario(scriptName, throwsOnLine, next) { client.reqContinue(function(error) { assert.ifError(error); - setTimeout(assertHasPaused.bind(null, client), 100); + interval = setInterval(assertHasPaused.bind(null, client), 10); }); } ); } function assertHasPaused(client) { - assert(exceptions.length, 'no exceptions thrown, race condition in test?'); - assert.equal(exceptions.length, 1, 'debugger did not pause on exception'); - assert.equal(exceptions[0].uncaught, true); - assert.equal(exceptions[0].script.name, testScript); - assert.equal(exceptions[0].sourceLine + 1, throwsOnLine); + if (!exceptions.length) return; + + assert.strictEqual(exceptions.length, 1, + 'debugger did not pause on exception'); + assert.strictEqual(exceptions[0].uncaught, true); + assert.strictEqual(exceptions[0].script.name, testScript); + assert.strictEqual(exceptions[0].sourceLine + 1, throwsOnLine); asserted = true; client.reqContinue(assert.ifError); + clearInterval(interval); } }