test: remove common.noop · nodejs/node@e879a56 · GitHub
Skip to content

Commit e879a56

Browse files
Trottaddaleax
Trott
authored and
addaleax
committed
test: remove common.noop
This change removes `common.noop` from the Node.js internal testing common module. Over the last few weeks, I've grown to dislike the `common.noop` abstraction. First, new (and experienced) contributors are unaware of it and so it results in a large number of low-value nits on PRs. It also increases the number of things newcomers and infrequent contributors have to be aware of to be effective on the project. Second, it is confusing. Is it a singleton/property or a getter? Which should be expected? This can lead to subtle and hard-to-find bugs. (To my knowledge, none have landed on master. But I also think it's only a matter of time.) Third, the abstraction is low-value in my opinion. What does it really get us? A case could me made that it is without value at all. Lastly, and this is minor, but the abstraction is wordier than not using the abstraction. `common.noop` doesn't save anything over `() => {}`. So, I propose removing it. PR-URL: #12822 Backport-PR-URL: #14174 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>
1 parent 76ba1b5 commit e879a56

67 files changed

Lines changed: 135 additions & 149 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎test/common/README.md‎

Lines changed: 4 additions & 17 deletions

‎test/common/index.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ const testRoot = process.env.NODE_TEST_DIR ?
3737

3838
const noop = () => {};
3939

40-
exports.noop = noop;
4140
exports.fixturesDir = path.join(__dirname, '..', 'fixtures');
4241
exports.tmpDirName = 'tmp';
4342
// PORT should match the definition in test/testpy/__init__.py.

‎test/message/unhandled_promise_trace_warnings.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// Flags: --trace-warnings
22
'use strict';
3-
const common = require('../common');
3+
require('../common');
44
const p = Promise.reject(new Error('This was rejected'));
5-
setImmediate(() => p.catch(common.noop));
5+
setImmediate(() => p.catch(() => {}));

‎test/parallel/test-assert.js‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -571,29 +571,30 @@ a.throws(makeBlock(a.deepEqual, args, []));
571571

572572
// check messages from assert.throws()
573573
{
574+
const noop = () => {};
574575
assert.throws(
575-
() => { a.throws(common.noop); },
576+
() => { a.throws((noop)); },
576577
common.expectsError({
577578
code: 'ERR_ASSERTION',
578579
message: /^Missing expected exception\.$/
579580
}));
580581

581582
assert.throws(
582-
() => { a.throws(common.noop, TypeError); },
583+
() => { a.throws(noop, TypeError); },
583584
common.expectsError({
584585
code: 'ERR_ASSERTION',
585586
message: /^Missing expected exception \(TypeError\)\.$/
586587
}));
587588

588589
assert.throws(
589-
() => { a.throws(common.noop, 'fhqwhgads'); },
590+
() => { a.throws(noop, 'fhqwhgads'); },
590591
common.expectsError({
591592
code: 'ERR_ASSERTION',
592593
message: /^Missing expected exception: fhqwhgads$/
593594
}));
594595

595596
assert.throws(
596-
() => { a.throws(common.noop, TypeError, 'fhqwhgads'); },
597+
() => { a.throws(noop, TypeError, 'fhqwhgads'); },
597598
common.expectsError({
598599
code: 'ERR_ASSERTION',
599600
message: /^Missing expected exception \(TypeError\): fhqwhgads$/

‎test/parallel/test-buffer-includes.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
'use strict';
2-
const common = require('../common');
2+
require('../common');
33
const assert = require('assert');
44

55
const b = Buffer.from('abcdef');
@@ -274,7 +274,7 @@ for (let lengthIndex = 0; lengthIndex < lengths.length; lengthIndex++) {
274274
const expectedError =
275275
/^TypeError: "val" argument must be string, number, Buffer or Uint8Array$/;
276276
assert.throws(() => {
277-
b.includes(common.noop);
277+
b.includes(() => {});
278278
}, expectedError);
279279
assert.throws(() => {
280280
b.includes({});

‎test/parallel/test-child-process-bad-stdio.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ const assert = require('assert');
55
const cp = require('child_process');
66

77
if (process.argv[2] === 'child') {
8-
setTimeout(common.noop, common.platformTimeout(100));
8+
setTimeout(() => {}, common.platformTimeout(100));
99
return;
1010
}
1111

‎test/parallel/test-child-process-spawnsync-kill-signal.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const assert = require('assert');
44
const cp = require('child_process');
55

66
if (process.argv[2] === 'child') {
7-
setInterval(common.noop, 1000);
7+
setInterval(() => {}, 1000);
88
} else {
99
const { SIGKILL } = process.binding('constants').os.signals;
1010

‎test/parallel/test-cluster-rr-domain-listen.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
// USE OR OTHER DEALINGS IN THE SOFTWARE.
2121

2222
'use strict';
23-
const common = require('../common');
23+
require('../common');
2424
const cluster = require('cluster');
2525
const domain = require('domain');
2626

@@ -29,10 +29,10 @@ const domain = require('domain');
2929

3030
if (cluster.isWorker) {
3131
const d = domain.create();
32-
d.run(common.noop);
32+
d.run(() => {});
3333

3434
const http = require('http');
35-
http.Server(common.noop).listen(0, '127.0.0.1');
35+
http.Server(() => {}).listen(0, '127.0.0.1');
3636

3737
} else if (cluster.isMaster) {
3838

‎test/parallel/test-cluster-worker-wait-server-close.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ if (cluster.isWorker) {
1111
const server = net.createServer(function(socket) {
1212
// Wait for any data, then close connection
1313
socket.write('.');
14-
socket.on('data', common.noop);
14+
socket.on('data', () => {});
1515
}).listen(0, common.localhostIPv4);
1616

1717
server.once('close', function() {
@@ -20,7 +20,7 @@ if (cluster.isWorker) {
2020

2121
// Although not typical, the worker process can exit before the disconnect
2222
// event fires. Use this to keep the process open until the event has fired.
23-
const keepOpen = setInterval(common.noop, 9999);
23+
const keepOpen = setInterval(() => {}, 9999);
2424

2525
// Check worker events and properties
2626
process.once('disconnect', function() {

‎test/parallel/test-common.js‎

Lines changed: 1 addition & 1 deletion

0 commit comments

Comments
 (0)