This repository has been archived by the owner. It is now read-only.

Timers explode when date has been stubbed #3710

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
9 participants
@austinbv

Timers in node all throw an exception when date has been stubbed, mocked, or modified and does not respond like a Date.

To allow for testing and to make times more self-relient the timers module now holds it's own copy of Date rather that referring to the global all over.

Use cases

  • Libraries like Timecop.js can be turned into a full npm modules
  • tests can have timeouts that do not freeze when time is modified
@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Jul 14, 2012

Shouldn't this be global.Date? (In node modules this === exports.)

Shouldn't this be global.Date? (In node modules this === exports.)

@austinbv

This comment has been minimized.

Show comment Hide comment
@austinbv

austinbv Jul 17, 2012

That should fix it

That should fix it

@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 2, 2012

Member

I don't know about this PR. The problem you're describing sounds like something that should be fixed in those test frameworks, not node core.

Member

bnoordhuis commented Aug 2, 2012

I don't know about this PR. The problem you're describing sounds like something that should be fixed in those test frameworks, not node core.

@austinbv

This comment has been minimized.

Show comment Hide comment
@austinbv

austinbv Aug 2, 2012

Here are a couple reasons I feel like this is a change for the better.

  1. This is how browsers work: in no browser environment I have encountered will you override Date and break core functions like set timeout. Providing the same interface will allow for better portability of code.
  2. The framework should be strong: I feel that a framework as fundamental as Node.js should not allow core functionality come to a screeching halt when something unrelated is overridden.
  3. Testing DateTime relient code is a pain: Without this small change Node would require you to create your own wrapper class to correctly test and stub date relient functions and maintain core functionality.
  4. The change has no downsides: It improve the stability of setTimeout and setInterval by removing the dependency on Date at run time this is a huge win, You are locking in what the module requires at compile time a user to modify the code to their hearts content.
  5. The change is small and testable. The fact that the change is testable will ensure that node does not lose the reliability of timers because of Date.
  6. Continues to help the testing and module community grow: I recently ported Timecop.js to be an npm module as well as have no Ruby dependency in the development enviroment. Both of these things are not possible without Timers being reliable not runtime dependent on Date.
  7. This pattern is happening elsewhere: https://github.com/visionmedia/mocha/blob/master/mocha.js#L1141
    Node should hold the same standard of reliability.
  8. Timers are not Dates: the fact that a Timer relies on Date is non consequential. A Timer should only care about the passage of time. Yes, the passage of time is calculated by the Date in this case, but if it could be, or was calculated another way, would that be a bad thing. No, because now Timers truly reflect their intention without exposing implementation details.
  9. To your point that the tests themselves should handle this. There is no clean way to handle stubbing of Date without creating a new interface for the Date object and then stubbing that. But that requires a new level of complexity for an application just to maintain a solid test suite. I feel strongly that this should be the responsibility of the framework, to provide the user with a stable core that is hard to untangle.

austinbv commented Aug 2, 2012

Here are a couple reasons I feel like this is a change for the better.

  1. This is how browsers work: in no browser environment I have encountered will you override Date and break core functions like set timeout. Providing the same interface will allow for better portability of code.
  2. The framework should be strong: I feel that a framework as fundamental as Node.js should not allow core functionality come to a screeching halt when something unrelated is overridden.
  3. Testing DateTime relient code is a pain: Without this small change Node would require you to create your own wrapper class to correctly test and stub date relient functions and maintain core functionality.
  4. The change has no downsides: It improve the stability of setTimeout and setInterval by removing the dependency on Date at run time this is a huge win, You are locking in what the module requires at compile time a user to modify the code to their hearts content.
  5. The change is small and testable. The fact that the change is testable will ensure that node does not lose the reliability of timers because of Date.
  6. Continues to help the testing and module community grow: I recently ported Timecop.js to be an npm module as well as have no Ruby dependency in the development enviroment. Both of these things are not possible without Timers being reliable not runtime dependent on Date.
  7. This pattern is happening elsewhere: https://github.com/visionmedia/mocha/blob/master/mocha.js#L1141
    Node should hold the same standard of reliability.
  8. Timers are not Dates: the fact that a Timer relies on Date is non consequential. A Timer should only care about the passage of time. Yes, the passage of time is calculated by the Date in this case, but if it could be, or was calculated another way, would that be a bad thing. No, because now Timers truly reflect their intention without exposing implementation details.
  9. To your point that the tests themselves should handle this. There is no clean way to handle stubbing of Date without creating a new interface for the Date object and then stubbing that. But that requires a new level of complexity for an application just to maintain a solid test suite. I feel strongly that this should be the responsibility of the framework, to provide the user with a stable core that is hard to untangle.
@magwo

This comment has been minimized.

Show comment Hide comment
@magwo

magwo Aug 16, 2012

I personally feel that "time travel" and so on, as described by Timecop, are concepts that are taking you in the wrong direction with regards to testing and testability. Code that wants to be testable, and relies on dates, timespans and such, should get all its time-related variables as parameters to constructors and functions.

Normally, creating a non-trivial object in a constructor or function - especially when it depends on global state like Date.. is a very bad idea IMO. It simply makes the code hard to test.

Whenever a library instantiates its own non-trivial objects in constructors and functions, it's usually only a matter of time until users of that library run into problems because they are unable to control the existence, properties and behavior of those objects.

And I can see downsides to making Date unmockable for timers - not only does it prevent the overriding of the behavior of timers (which is a bad idea but still), it also surprises the programmer using timers and Timecop. It's a very unexpected behavior, to be resilient against monkey-patching, I think.

magwo commented Aug 16, 2012

I personally feel that "time travel" and so on, as described by Timecop, are concepts that are taking you in the wrong direction with regards to testing and testability. Code that wants to be testable, and relies on dates, timespans and such, should get all its time-related variables as parameters to constructors and functions.

Normally, creating a non-trivial object in a constructor or function - especially when it depends on global state like Date.. is a very bad idea IMO. It simply makes the code hard to test.

Whenever a library instantiates its own non-trivial objects in constructors and functions, it's usually only a matter of time until users of that library run into problems because they are unable to control the existence, properties and behavior of those objects.

And I can see downsides to making Date unmockable for timers - not only does it prevent the overriding of the behavior of timers (which is a bad idea but still), it also surprises the programmer using timers and Timecop. It's a very unexpected behavior, to be resilient against monkey-patching, I think.

@magwo

This comment has been minimized.

Show comment Hide comment
@magwo

magwo Aug 20, 2012

Oh and I would add that setTimeout is a function that, for testing purposes, normally should also be injected into code and be mocked by an immediateTimeout kind of function in tests.

magwo commented Aug 20, 2012

Oh and I would add that setTimeout is a function that, for testing purposes, normally should also be injected into code and be mocked by an immediateTimeout kind of function in tests.

@austinbv

This comment has been minimized.

Show comment Hide comment
@austinbv

austinbv Aug 24, 2012

I do agree with you that these things should happen a lot of the time but I am not a fan of a framework as broad as node forcing a specific architecture especially when it is not consistant with browser JS. It may be better practice it may just work better for you, I still think it is rather silly to brush of a change for stability and constancy just because your opinion of how software should be architected.

I do agree with you that these things should happen a lot of the time but I am not a fan of a framework as broad as node forcing a specific architecture especially when it is not consistant with browser JS. It may be better practice it may just work better for you, I still think it is rather silly to brush of a change for stability and constancy just because your opinion of how software should be architected.

@magwo

This comment has been minimized.

Show comment Hide comment
@magwo

magwo Aug 24, 2012

These are just my opinions, I'm only pitching in and giving my view. I am in no way involved with development of Node. I think what makes me against this change is that it causes unexpected behaviour in Node code - as a monkey-patched Date implementation will be sort of application-wide, but not quite. It would be weird I think. Is there not a better solution?

magwo commented Aug 24, 2012

These are just my opinions, I'm only pitching in and giving my view. I am in no way involved with development of Node. I think what makes me against this change is that it causes unexpected behaviour in Node code - as a monkey-patched Date implementation will be sort of application-wide, but not quite. It would be weird I think. Is there not a better solution?

@jamesarosen

This comment has been minimized.

Show comment Hide comment
@jamesarosen

jamesarosen Jan 22, 2013

@bnoordhuis I'm quite happy to fix whatever needs fixing in Timecop to make it work on node. As far as I can tell, though, there's no way for me to override this behavior from that library. (I could try to redefine setTimeout et al, but other libraries will already have a reference to the default implementation.)

@bnoordhuis I'm quite happy to fix whatever needs fixing in Timecop to make it work on node. As far as I can tell, though, there's no way for me to override this behavior from that library. (I could try to redefine setTimeout et al, but other libraries will already have a reference to the default implementation.)

@TooTallNate

This comment has been minimized.

Show comment Hide comment
@TooTallNate

TooTallNate Jan 22, 2013

I'm rather on the fence with this (leaning towards a -1 though)... on one hand, there's no harm in being resilient against the environment we're running in. But on the other hand anyone who overwrites the global Date object is asking for trouble, and I also feel that this solution is somewhat "hacky".

Additionally, there's probably other places in node-core where overwriting one of the built-in natives would cause problems, so this doesn't really seem like something we should be worrying about (or if we are, then to make sure to catch all the places where that can happen, which also sounds difficult).

I'm rather on the fence with this (leaning towards a -1 though)... on one hand, there's no harm in being resilient against the environment we're running in. But on the other hand anyone who overwrites the global Date object is asking for trouble, and I also feel that this solution is somewhat "hacky".

Additionally, there's probably other places in node-core where overwriting one of the built-in natives would cause problems, so this doesn't really seem like something we should be worrying about (or if we are, then to make sure to catch all the places where that can happen, which also sounds difficult).

@jamesarosen

This comment has been minimized.

Show comment Hide comment
@jamesarosen

jamesarosen Jan 22, 2013

@magwo said, "Code that wants to be testable, and relies on dates, timespans and such, should get all its time-related variables as parameters to constructors and functions."

That seems like you're saying tests should always determine the API. Let's say you're building a scheduling library, and you want to add an inNHours function. In a non-test-driven environment, you might write

Scheduler.prototype.inNHours = function(n, fn) {
  var at = +(new Date()) + ( n * 1000 * 60 * 60 );
  this.schedule(at, fn);
};

You want to test that behavior in an integration test, but you don't want to wait 3 hours for the tests to run. According to your rule, the solution would be to change the API to

Schedule.prototype.inNHours = function(n, fn, from) {
  from = from || new Date(),
  var at = +from + ( n * 1000 * 60 * 60 );
  this.schedule(at, fn);
};

This adds complexity to the API and the implementation to make your tests happy. In general, I'm for driving the design with tests (or with some other well-specified client) since it makes for better APIs. Maybe I should trust TDD here. After all, who am I to say that clients of this hypothetical Schedule wouldn't want the third argument? I usually like (small) dependency injection like this, so maybe I'm just hesitant because I wrote the library and want it to live on.

@magwo said, "Code that wants to be testable, and relies on dates, timespans and such, should get all its time-related variables as parameters to constructors and functions."

That seems like you're saying tests should always determine the API. Let's say you're building a scheduling library, and you want to add an inNHours function. In a non-test-driven environment, you might write

Scheduler.prototype.inNHours = function(n, fn) {
  var at = +(new Date()) + ( n * 1000 * 60 * 60 );
  this.schedule(at, fn);
};

You want to test that behavior in an integration test, but you don't want to wait 3 hours for the tests to run. According to your rule, the solution would be to change the API to

Schedule.prototype.inNHours = function(n, fn, from) {
  from = from || new Date(),
  var at = +from + ( n * 1000 * 60 * 60 );
  this.schedule(at, fn);
};

This adds complexity to the API and the implementation to make your tests happy. In general, I'm for driving the design with tests (or with some other well-specified client) since it makes for better APIs. Maybe I should trust TDD here. After all, who am I to say that clients of this hypothetical Schedule wouldn't want the third argument? I usually like (small) dependency injection like this, so maybe I'm just hesitant because I wrote the library and want it to live on.

@jamesarosen

This comment has been minimized.

Show comment Hide comment
@jamesarosen

jamesarosen Jan 22, 2013

@TooTallNate It's certainly possible that Timecop, which has proven valuable in the Ruby community, simply doesn't fit in Node. The languages are substantially different. Perhaps most notably, it's common in JavaScript to save off a reference to an external object (say, Date). This is much less common in Ruby. Perhaps this one small (but important!) detail makes all the difference.

@TooTallNate It's certainly possible that Timecop, which has proven valuable in the Ruby community, simply doesn't fit in Node. The languages are substantially different. Perhaps most notably, it's common in JavaScript to save off a reference to an external object (say, Date). This is much less common in Ruby. Perhaps this one small (but important!) detail makes all the difference.

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Jan 22, 2013

How does this patch affect benchmarks? If it makes {set,clear}{Timeout,Interval,Immediate} faster, I'd take it. Otherwise, no.

isaacs commented Jan 22, 2013

How does this patch affect benchmarks? If it makes {set,clear}{Timeout,Interval,Immediate} faster, I'd take it. Otherwise, no.

@austinbv

This comment has been minimized.

Show comment Hide comment
@austinbv

austinbv Jan 23, 2013

@jamesarosen I have written the patches for timecop but never pushed them to you because of this oddity in node. 👎 . I agree there is a difference betten the ruby and node community but I still think "being resilient against the environment we're running in" is totally true.

As for the solution being hacky... I am not sure how, this is the point of closure, if date were to be passed in just as everyone is arguing the same problem would be solved, rather than relying on a global to exist.

@jamesarosen I have written the patches for timecop but never pushed them to you because of this oddity in node. 👎 . I agree there is a difference betten the ruby and node community but I still think "being resilient against the environment we're running in" is totally true.

As for the solution being hacky... I am not sure how, this is the point of closure, if date were to be passed in just as everyone is arguing the same problem would be solved, rather than relying on a global to exist.

@trevnorris

This comment has been minimized.

Show comment Hide comment
@trevnorris

trevnorris Feb 7, 2013

Reminder to self. I'll throw up the benchmark numbers in the next week or so. Please remind me if I forget. =)

Reminder to self. I'll throw up the benchmark numbers in the next week or so. Please remind me if I forget. =)

@Nodejs-Jenkins

This comment has been minimized.

Show comment Hide comment
@Nodejs-Jenkins

Nodejs-Jenkins Mar 13, 2013

Can one of the admins verify this patch?

Can one of the admins verify this patch?

@tjfontaine

This comment has been minimized.

Show comment Hide comment
@tjfontaine

tjfontaine Feb 18, 2014

We no longer use Date in timers.js so this shouldn't be a problem anymore

We no longer use Date in timers.js so this shouldn't be a problem anymore

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.