From defefba45cf21882568b82f167afde1a234c1cbe Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 4 Nov 2016 14:04:00 -0700 Subject: [PATCH 1/2] test,benchmark: provide duration/interval to timers There are several places in the code base where setTimeout() or setInterval() are called with just a callback and no duration/interval. The timers module will use a value of `1` in that situation. I find an unspecified duration or interval confusing. I always spend a moment wondering if it is a mistake. Did the original author simply forget to provide a value? Did they intend to use setImmediate() or even process.nextTick() instead of setTimeout()? And so on. This change provides a duration or interval of `1` to all calls in the codebase where it is missing. `parallel/test-timers.js` still tests the situation where `setTimeout()` and `setInterval()` are called with `undefined` and other non-numeric values for the duration/interval. --- test/message/timeout_throw.js | 2 +- test/parallel/test-handle-wrap-close-abort.js | 2 +- test/parallel/test-stream-end-paused.js | 2 +- test/parallel/test-stream-readable-event.js | 6 +++--- test/parallel/test-stream2-large-read-stall.js | 2 +- test/parallel/test-stream2-readable-non-empty-end.js | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/test/message/timeout_throw.js b/test/message/timeout_throw.js index 14291fabd12565..4008f306fe58be 100644 --- a/test/message/timeout_throw.js +++ b/test/message/timeout_throw.js @@ -4,4 +4,4 @@ require('../common'); setTimeout(function() { // eslint-disable-next-line no-undef undefined_reference_error_maker; -}); +}, 1); diff --git a/test/parallel/test-handle-wrap-close-abort.js b/test/parallel/test-handle-wrap-close-abort.js index 5355e65df60821..e9f69195ad29cd 100644 --- a/test/parallel/test-handle-wrap-close-abort.js +++ b/test/parallel/test-handle-wrap-close-abort.js @@ -13,4 +13,4 @@ setTimeout(function() { setTimeout(function() { throw new Error('setTimeout'); }, 1); -}); +}, 1); diff --git a/test/parallel/test-stream-end-paused.js b/test/parallel/test-stream-end-paused.js index 3c46d49ea65d65..4b585fdb999890 100644 --- a/test/parallel/test-stream-end-paused.js +++ b/test/parallel/test-stream-end-paused.js @@ -21,7 +21,7 @@ stream.pause(); setTimeout(common.mustCall(function() { stream.on('end', common.mustCall(function() {})); stream.resume(); -})); +}), 1); process.on('exit', function() { assert(calledRead); diff --git a/test/parallel/test-stream-readable-event.js b/test/parallel/test-stream-readable-event.js index a20fc2ee732d0f..a8536bdcbab861 100644 --- a/test/parallel/test-stream-readable-event.js +++ b/test/parallel/test-stream-readable-event.js @@ -20,7 +20,7 @@ const Readable = require('stream').Readable; // we're testing what we think we are assert(!r._readableState.reading); r.on('readable', common.mustCall(function() {})); - }); + }, 1); } { @@ -40,7 +40,7 @@ const Readable = require('stream').Readable; // assert we're testing what we think we are assert(r._readableState.reading); r.on('readable', common.mustCall(function() {})); - }); + }, 1); } { @@ -60,5 +60,5 @@ const Readable = require('stream').Readable; // assert we're testing what we think we are assert(!r._readableState.reading); r.on('readable', common.mustCall(function() {})); - }); + }, 1); } diff --git a/test/parallel/test-stream2-large-read-stall.js b/test/parallel/test-stream2-large-read-stall.js index 2a58bb213cc4bd..60e4a2873a0945 100644 --- a/test/parallel/test-stream2-large-read-stall.js +++ b/test/parallel/test-stream2-large-read-stall.js @@ -46,7 +46,7 @@ function push() { console.error(' push #%d', pushes); if (r.push(Buffer.allocUnsafe(PUSHSIZE))) - setTimeout(push); + setTimeout(push, 1); } process.on('exit', function() { diff --git a/test/parallel/test-stream2-readable-non-empty-end.js b/test/parallel/test-stream2-readable-non-empty-end.js index 9800dfdad15cac..0a4eb131c312ab 100644 --- a/test/parallel/test-stream2-readable-non-empty-end.js +++ b/test/parallel/test-stream2-readable-non-empty-end.js @@ -16,7 +16,7 @@ test._read = function(size) { var chunk = chunks[n++]; setTimeout(function() { test.push(chunk === undefined ? null : chunk); - }); + }, 1); }; test.on('end', thrower); @@ -31,7 +31,7 @@ test.on('readable', function() { if (res) { bytesread += res.length; console.error('br=%d len=%d', bytesread, len); - setTimeout(next); + setTimeout(next, 1); } test.read(0); }); From 8a4536d3035df430123c00f40c8d57e325e2b05b Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 4 Nov 2016 14:08:50 -0700 Subject: [PATCH 2/2] tools: add lint rule to enforce timer arguments Add a custom ESLint rule to require that setTimeout() and setInterval() get called with at least two arguments. This prevents omitting the duration or interval. --- .eslintrc | 1 + tools/eslint-rules/timer-arguments.js | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 tools/eslint-rules/timer-arguments.js diff --git a/.eslintrc b/.eslintrc index 54b9049df8baff..cf1f36c86bcbdc 100644 --- a/.eslintrc +++ b/.eslintrc @@ -128,6 +128,7 @@ rules: assert-fail-single-argument: 2 assert-throws-arguments: [2, { requireTwo: false }] new-with-error: [2, Error, RangeError, TypeError, SyntaxError, ReferenceError] + timer-arguments: 2 # Global scoped method and vars globals: diff --git a/tools/eslint-rules/timer-arguments.js b/tools/eslint-rules/timer-arguments.js new file mode 100644 index 00000000000000..4dd7816ff82ff2 --- /dev/null +++ b/tools/eslint-rules/timer-arguments.js @@ -0,0 +1,25 @@ +/** + * @fileoverview Require at least two arguments when calling setTimeout() or + * setInterval(). + * @author Rich Trott + */ +'use strict'; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +function isTimer(name) { + return ['setTimeout', 'setInterval'].includes(name); +} + +module.exports = function(context) { + return { + 'CallExpression': function(node) { + const name = node.callee.name; + if (isTimer(name) && node.arguments.length < 2) { + context.report(node, `${name} must have at least 2 arguments`); + } + } + }; +};