From c94f98af37f876ec124d36f39f0c5d85b313ada3 Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Sat, 29 Apr 2017 00:41:18 +0300 Subject: [PATCH 1/2] test: make common.noop a getter Make `common.noop` a getter that returns a new function object each time the propery is read, not the same one. The old behavior could possibly lead to subtle and hard to catch bugs in some cases. Refs: https://github.com/nodejs/node/pull/12711#discussion_r114008771 --- test/README.md | 5 ++++- test/common.js | 16 +++++++------- test/parallel/test-common.js | 5 +++++ ...test-event-emitter-remove-all-listeners.js | 21 +++++++++++-------- 4 files changed, 30 insertions(+), 17 deletions(-) diff --git a/test/README.md b/test/README.md index 6b073c757269ef..6fefeee4b26455 100644 --- a/test/README.md +++ b/test/README.md @@ -348,7 +348,10 @@ Returns `true` if the exit code `exitCode` and/or signal name `signal` represent ### noop -A non-op `Function` that can be used for a variety of scenarios. +A getter that returns a non-op `Function` which can be used for a variety of +scenarios. A new function object is always created, so the result of +`common.noop` must be saved in a variable if it is intended to be used in +assertions. For instance, diff --git a/test/common.js b/test/common.js index 2d61fb0dc58012..a10b7936737ae7 100644 --- a/test/common.js +++ b/test/common.js @@ -35,9 +35,11 @@ const execSync = require('child_process').execSync; const testRoot = process.env.NODE_TEST_DIR ? fs.realpathSync(process.env.NODE_TEST_DIR) : __dirname; -const noop = () => {}; +Object.defineProperty(exports, 'noop', { + enumerable: true, + get: () => () => {} +}); -exports.noop = noop; exports.fixturesDir = path.join(__dirname, 'fixtures'); exports.tmpDirName = 'tmp'; // PORT should match the definition in test/testpy/__init__.py. @@ -434,9 +436,9 @@ function runCallChecks(exitCode) { exports.mustCall = function(fn, expected) { if (typeof fn === 'number') { expected = fn; - fn = noop; + fn = exports.noop; } else if (fn === undefined) { - fn = noop; + fn = exports.noop; } if (expected === undefined) @@ -530,9 +532,9 @@ util.inherits(ArrayStream, stream.Stream); exports.ArrayStream = ArrayStream; ArrayStream.prototype.readable = true; ArrayStream.prototype.writable = true; -ArrayStream.prototype.pause = noop; -ArrayStream.prototype.resume = noop; -ArrayStream.prototype.write = noop; +ArrayStream.prototype.pause = exports.noop; +ArrayStream.prototype.resume = exports.noop; +ArrayStream.prototype.write = exports.noop; // Returns true if the exit code "exitCode" and/or signal name "signal" // represent the exit code and/or signal name of a node process that aborted, diff --git a/test/parallel/test-common.js b/test/parallel/test-common.js index 201fa40d08ac25..ae38b73d56a34b 100644 --- a/test/parallel/test-common.js +++ b/test/parallel/test-common.js @@ -40,6 +40,11 @@ assert.throws(function() { }, /^TypeError: Invalid expected value: \/foo\/$/); +// common.noop() tests +assert.strictEqual(common.noop(), undefined); +assert.notStrictEqual(common.noop, common.noop); + + // assert.fail() tests assert.throws( () => { assert.fail('fhqwhgads'); }, diff --git a/test/parallel/test-event-emitter-remove-all-listeners.js b/test/parallel/test-event-emitter-remove-all-listeners.js index a4c3f76f6c7281..23269f5db71bd4 100644 --- a/test/parallel/test-event-emitter-remove-all-listeners.js +++ b/test/parallel/test-event-emitter-remove-all-listeners.js @@ -24,7 +24,6 @@ const common = require('../common'); const assert = require('assert'); const events = require('events'); - function expect(expected) { const actual = []; process.on('exit', function() { @@ -38,24 +37,28 @@ function expect(expected) { { const ee = new events.EventEmitter(); - ee.on('foo', common.noop); - ee.on('bar', common.noop); - ee.on('baz', common.noop); - ee.on('baz', common.noop); + const fooListener = common.noop; + const barListener = common.noop; + const bazListener1 = common.noop; + const bazListener2 = common.noop; + ee.on('foo', fooListener); + ee.on('bar', barListener); + ee.on('baz', bazListener1); + ee.on('baz', bazListener2); const fooListeners = ee.listeners('foo'); const barListeners = ee.listeners('bar'); const bazListeners = ee.listeners('baz'); ee.on('removeListener', expect(['bar', 'baz', 'baz'])); ee.removeAllListeners('bar'); ee.removeAllListeners('baz'); - assert.deepStrictEqual(ee.listeners('foo'), [common.noop]); + assert.deepStrictEqual(ee.listeners('foo'), [fooListener]); assert.deepStrictEqual(ee.listeners('bar'), []); assert.deepStrictEqual(ee.listeners('baz'), []); // After calling removeAllListeners(), // the old listeners array should stay unchanged. - assert.deepStrictEqual(fooListeners, [common.noop]); - assert.deepStrictEqual(barListeners, [common.noop]); - assert.deepStrictEqual(bazListeners, [common.noop, common.noop]); + assert.deepStrictEqual(fooListeners, [fooListener]); + assert.deepStrictEqual(barListeners, [barListener]); + assert.deepStrictEqual(bazListeners, [bazListener1, bazListener2]); // After calling removeAllListeners(), // new listeners arrays is different from the old. assert.notStrictEqual(ee.listeners('bar'), barListeners); From 48ec07392e1267caa383d7699cc3c46d4ef94646 Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Sat, 29 Apr 2017 01:05:50 +0300 Subject: [PATCH 2/2] fixup! test: make common.noop a getter --- test/parallel/test-event-emitter-remove-all-listeners.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/parallel/test-event-emitter-remove-all-listeners.js b/test/parallel/test-event-emitter-remove-all-listeners.js index 23269f5db71bd4..15c20cc662c711 100644 --- a/test/parallel/test-event-emitter-remove-all-listeners.js +++ b/test/parallel/test-event-emitter-remove-all-listeners.js @@ -24,6 +24,7 @@ const common = require('../common'); const assert = require('assert'); const events = require('events'); + function expect(expected) { const actual = []; process.on('exit', function() {