Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not store a direct reference to Date.now #1478

Closed
wants to merge 1 commit into from

Conversation

lfac-pt
Copy link

@lfac-pt lfac-pt commented Feb 14, 2014

The adition of _.now made it much harder to use fake timers when testing (e.g., http://sinonjs.org/docs/#clock), because we need to stub Date.now before loading Underscore. This is not practical at all and doesn't allow for using fake timers only in some tests.

This patch adds a wrapper function around Date.now.

I understand that this may incur a small performance penalty, but I think that being able to test code that uses _.now (directly or indirectly) is more important.

The adition of `_.now` made it much harder to use fake timers when testing (e.g., http://sinonjs.org/docs/#clock), because we need to stub `Date.now` before loading Underscore. This is not practical at all and doesn't allow for using fake timers only in some tests.

This patch adds a wrapper function around `Date.now`.

I understand that this may incur a small performance penalty, but I think that being able to test code that uses `_.now` (directly or indirectly) is more important.
@jashkenas
Copy link
Owner

Thanks, but we shouldn't be changing Underscore code to make it easier to use a particular mocking library. That's would be backwards.

@lfac-pt
Copy link
Author

lfac-pt commented Feb 14, 2014

I understand your argument, but this is not for a particular library. I was just giving an example. Storing a direct reference won't allow any clock mocking library to work easily.

@@ -1135,7 +1135,7 @@
};

// A (possibly faster) way to get the current timestamp as an integer.
_.now = Date.now || function() { return new Date().getTime(); };
_.now = function () { return Date.now(); } || function() { return new Date().getTime(); };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't quite right, since the LHS || operand will now always evaluate true. We'd lose the feature detection, in other words. We'd need something like this, instead:

_.now = Date.now ? function() { return Date.now(); } :
                   function() { return new Date().getTime(); };

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops! You are right of course. Anyway, it probably won't matter since the PR is closed (although I can fix that if it gets re-opened).

@davidchambers
Copy link
Contributor

The addition of _.now made it much harder to use fake timers when testing […] because we need to stub Date.now before loading Underscore.

Would another solution be to assign your function to _.now as well as Date.now?

_.now = Date.now = ...;

@lfac-pt
Copy link
Author

lfac-pt commented Feb 14, 2014

I fixed the problem on my side by adding:

now : function () {
    return Date.now();
}

to my project mixins (we do not support IE8).

I made the PR because I think it is likely that other people with these kinds of tests will have the same problem and that Underscore shouldn't (by default) get in the way of testing.

Well, at least the issue is documented now!

@eschwartz
Copy link

To fix compatibility issues with SinonJS, I created a wrapper around the sinon.useFakeTimers method which stubs out _.now:

define('clock', ['sinon', 'underscore'], function(sinon, _) {
  var now_orig = _.now;
  var clock;

  return {
    useFakeTimers: function() {
      // using date.getTime() instead of Date.now
      // allows the sinon fake timers to do their thing
      _.now = function () { return new Date().getTime(); };
      clock = sinon.useFakeTimers();
    },
    restore: function() {
      clock.restore();

      // restore stubbed out _.now
      _.now = now_orig;
    },
    tick: function(int) {
      clock.tick(int);
    }
  };
});

Then in your tests, you can use your sinon timers (almost) like normal:

// I'm using jasmine, but this should work similarly for any testing framework.
define(['sut', 'clock'], function(SUT, clock) {

  beforeEach(function() {
    clock.useFakeTimers();
  });

  afterEach(function() {
    clock.restore();
  });

  it('should do something after 100ms', function() {
    //...
    clock.tick(100);
    expect(SUT).toHaveDone(something);
  });
});

Now I can update to v1.6.0! 😄

I hope this is helpful to those who are still having issues.

@lo1tuma
Copy link

lo1tuma commented Mar 10, 2014

This issue is really annoying. There is no clean way to use a fake clock.

@michaelficarra
Copy link
Collaborator

That problem is inherent to JavaScript closures, no?

@lfac-pt
Copy link
Author

lfac-pt commented Mar 10, 2014

Hum, how is this issue related with closures?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants