tools,test: provide duration/interval to timers, require it with lint rule #9472

Trott · 2016-11-04T21:13:22Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tools test timers

Description of change

First commit

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.

Second commit

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.

not-an-aardvark

I have a slight nitpick with the error message, but otherwise this LGTM.

not-an-aardvark · 2016-11-04T21:42:38Z

tools/eslint-rules/timer-arguments.js

+    'CallExpression': function(node) {
+      const name = node.callee.name;
+      if (isTimer(name) && node.arguments.length < 2) {
+        context.report(node, `${name} must have exactly 2 arguments`);


This error message seems a bit misleading to me, since setTimeout and setInterval can be called with more than 2 arguments. Maybe it would be better to use must have at least 2 arguments instead.

Yes, you are right, I updated the rule to accommodate more arguments but did not update the message. Fixed.

not-an-aardvark · 2016-11-04T21:43:54Z

tools/eslint-rules/timer-arguments.js

+module.exports = function(context) {
+  return {
+    'CallExpression': function(node) {
+      const name = node.callee.name;


I'm not sure if it matters, but name will be undefined for calls like foo.bar().

I think that's fine but would be happy to add a check for the situation if others feel differently.

Fishrock123 · 2016-11-04T22:07:15Z

I think this is unnecessary and we should probably not bother.

mscdex · 2016-11-04T22:09:55Z

Unless these are specifically testing setTimeout()/setInterval(), they could probably be replaced with setImmediate().

not-an-aardvark

LGTM

Refs: nodejs#90 See also: nodejs/node#9637 nodejs/node#9472 nodejs/node#9648

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.

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.

Trott · 2017-01-06T23:25:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5741/

Trott · 2017-01-06T23:51:08Z

Landed in 6263c00 and 4e5cf85.

There are 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. An unspecified duration or interval can be confusing. Did the original author forget to provide a value? Did they intend to use setImmediate() or 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. PR-URL: nodejs#9472 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>