Conceivable _.throttle bug goes undetected in /test/functions.js

[zheng-kai]

Hello, I'm a beginner to throttle.When I tried to implement the throttle function, I found that the test cases was incomplete. My throttle function will execute func as many times as it was called.But tests don't detect this bug. Here is my implement.

function throttle(func, wait, options) {
  let callbackArgs = []; // 节流可能要延迟执行很多函数，所以要将函数参数保存起来。
  let result = undefined; // 用来保存返回值
  let originalNow = Date.now;
  let prevTime; //用来记录上一次函数调用的时间
  let timer;
  //throttle does not trigger leading call when leading is set to false
  let noLeading = options && options.leading === false; // options.leading may be undefined. default noLeading=false;
  //throttle does not trigger trailing call when trailing is set to false
  let noTrailing = options && options.trailing === false;
  let ans = function () {
    let now = originalNow();
    let needWaiting;
    if (prevTime) {
      needWaiting = now - prevTime < wait; //判断本次调用是否在节流的判定范围内
    } else {
      needWaiting = noLeading;
      if (noLeading) {
        prevTime = now; // 函数不会调用，但需要记录时间点，用来确定判断后来的函数是否被截。
      }
    }
    let a = Array.from(arguments);
    if (needWaiting) {
      //该函数调用需要被节流阀拦截。
      if (noTrailing) {
        // 如果trailing=false，则不会调用后来的函数，所以不需要更新时间戳，直接返回。
        return result;
      }
      callbackArgs.push(a);
      if (noLeading) {
        // 如果leading=false，则不会调用先来的函数，所以清除掉。
        clearTimeout(timer);
      }
      timer = setTimeout(() => {
        if (callbackArgs.length) {
          //按照队列顺序，等待函数执行。
          ans.apply(this, callbackArgs.shift()); // 递归调用，确保每一个加入队列的函数都会被执行。
        }
      }, wait);
      return result;
    } else {
      prevTime = now;
      result = func.apply(this, a);
      return result;
    }
  };
  ans.cancel = function () {
    prevTime = undefined;
    callbackArgs = [];
  };
  return ans;
}

This is a test case that exposed the bug.

var __ = typeof require == 'function' ? require('.') : window._;  // import my implement
let fun = function (v) {
    console.log(v)
}
let t = __.throttle(fun,1000);
t(1)
t(2)
t(3)
/* output console
1
2
3
*/
/* but the correct output should be
1
2
*/

Can modify the first test case about throttle like this

  // original
  QUnit.test('throttle', function(assert) {
    assert.expect(2);
    var done = assert.async();
    var counter = 0;
    var incr = function(){ counter++; };
    var throttledIncr = __.throttle(incr, 32);
    throttledIncr(); throttledIncr();
    assert.strictEqual(counter, 1, 'incr was called immediately');
    _.delay(function(){ assert.strictEqual(counter, 2, 'incr was throttled'); done(); }, 64);
  });
  // modifyed
  QUnit.test('throttle', function(assert) {
    assert.expect(2);
    var done = assert.async();
    var counter = 0;
    var incr = function(){ counter++; };
    var throttledIncr = __.throttle(incr, 32);
    throttledIncr(); throttledIncr();throttledIncr(); // modify this line
    assert.strictEqual(counter, 1, 'incr was called immediately');
    _.delay(function(){ assert.strictEqual(counter, 2, 'incr was throttled'); done(); }, 64);
  });

[jgonggrijp]

Hey @zheng-kai, thank you for sharing the fruits of your research effort here.

Did you mean that your (slightly faulty) implementation passes all _.throttle tests? That is quite impressive by itself already. My compliments.

So there is a bug in your implementation that our tests can't detect. That lack of detection I wouldn't call a bug in itself; it's impossible to cover all conceivable bugs in our unittests. Still, I agree that it would be desirable to add detection for this particular flavor, now that we know about it.

Would you like to submit a pull request and get the credit for it? I would suggest adding your modified version of the first case as an additional test case, rather than it replacing the original first case, because those two versions are testing subtly different things and there is no reason not to have both.

This is a test case that exposed the bug.

var __ = typeof require == 'function' ? require('.') : window._;  // import my implement
let fun = function (v) {
    console.log(v)
}
let t = __.throttle(fun,1000);
t(1)
t(2)
t(3)
/* output console
1
2
3
*/
/* but the correct output should be
1
2
*/

Shouldn't that be

/*
1
3
*/

?

[zheng-kai]

Yes,my implementation passes all _.throttle tests. I'll submit a pull request soon. oh, the correct output should be

1
3

[zheng-kai]

Thanks for your response😆