kolodny / exercises

Some basic javascript coding challenges and interview questions
4.23k stars 672 forks source link

Test suite for `throttle` appears to be incorrect #3

Closed marthaberner closed 9 years ago

marthaberner commented 9 years ago

The throttle test suite has the following tests:

  it('gets called with context', function(done) {
    var ctx;
    var throttled = throttle(function() {
      ctx = this;
    }, 10);
    throttled.call(11);
    throttled.call(22);
    setTimeout(function() {
      assert.equal(ctx, 22);
      done();
    }, 15)
  });

  it('gets called with arguments', function(done) {
    var args;
    var throttled = throttle(function() {
      args = [].slice.call(arguments);
    }, 10);
    throttled(11, 22, 33);
    throttled(22, 33, 44);
    setTimeout(function() {
      assert.deepEqual(args, [22, 33, 44]);
      done();
    }, 15);
  });

In both cases, it appears that the test suite is expecting the throttled function to execute twice within the wait period. Is this test suite correct?

Shouldn't that last test's assertion be assert.deepEqual(args, [11, 22, 33]);?

kolodny commented 9 years ago

Nope, in both cases it swallows up the first execution of the function and only runs the second after the required time passes

kolodny commented 9 years ago

Actually it appears that you are correct. It seems I mixed up throttle and denounce. I'll fix that when I get a chance

Ephem commented 9 years ago

If this throttle-function is supposed to behave like the Underscore one it seems as if it should actually be called twice:

By default, throttle will execute the function as soon as you call it for the first time, and, if you call it again any number of times during the wait period, as soon as that period is over.

Not the clearest sentence, but if I understand it correctly this would mean that it's actually the Readme and the first tests that should be changed?

thedufer commented 9 years ago

This doesn't look fixed. The first test case for throttle expects 3 calls in a row to result in a single execution, but _.throttle would call it twice - once on the leading edge, once on the trailing edge. It may be that you intended it to mimic _.throttle with either the leading: false or trailing: false options set, in which case the README should be updated as such.

kolodny commented 9 years ago

I renamed it to debounce, I'll add a throttle exercise later