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

lolex can now attach itself to the system timers and automatically ad… #102

Merged
merged 7 commits into from
Jul 14, 2017
Merged

Conversation

acud
Copy link
Contributor

@acud acud commented Jun 28, 2017

in regards to issue #85: these changes will allow lolex to attach itself to the system timers when the clock is being installed.

when installing - lolex will set up a normal 'setInterval' on the real system timer (the one that's being stored at _setInterval) at a default value of 20ms, since every time the interval gets triggered the event loop will get blocked - I thought that 50hz is about enough to get good resolution but also not to clog the event loop at the same time (0.5-1 second is too much IMHO, 1ms too fast). every time the setInterval gets triggered - it will tick(20) thus resulting in timers getting invoked.

when uninstalling - lolex will remove the setInterval that was created.

i implemented the possibility to toggle this feature on and off through exports.shouldAdvanceTime - since I'm not sure that adding another argument to the constructor would be a wise choice for this use case - too much magic reordering arguments at the ctor and I would like to propagate this into sinon (which uses install(now || 0, methods)).

…vance

mocked time at a certain 'real' interval
@fatso83
Copy link
Contributor

fatso83 commented Jun 28, 2017

Thank you for this contribution! At first I thought this is really two pull requests in one I see, as it both implements async set* methods and real time advance. They are not necessarily the same, so I thought it might make sense to keep the config for the two separate.

For instance, you can have the faked Date.now() delegate to the real function and give the elapsed time since the start without any asynchronicity.

Then I realized that this meant the tick method wouldn't make sense in that scenario, and implicitly neither would timers (as you could never leave the main execution thread), so it would really only make sense together with the async behavior you added ... 😊

I'll leave this here for @mroderick to see as well, but 👍 from me.

src/lolex-src.js Outdated
@@ -394,6 +398,12 @@ function hijackMethod(target, method, clock) {
target[method].clock = clock;
}

exports.shouldAdvanceTime = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

not super in-love with this, but on the other hand I also hate magic constructor argument matching. What's wrong with config objects?

Copy link
Contributor Author

@acud acud Jun 28, 2017

Choose a reason for hiding this comment

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

I'm also not super in-love with this 🤣
is this what you mean?

exports.config = {
    shouldAdvanceTime: false
}

and then toggle it through:

lolex.config = {shouldAdvanceTime: true};
OR:
lolex.config.shouldAdvanceTime = true;

var clock = lolex.install(date);

I didn't want to overload the constructor with another parameter since I didn't dive too deep into what would be the implications of rewriting the logic doing the parameter swapping magic and what would be the possible repercussions for people using this library on contexts different than sinon. Also if adding another parameter to the install function (obviously at the end of the param list - install(target, now, toFake, loopLimit, shouldAdvanceTime)) - now install would be invoked with install(date,null,null,null,true)? not sure if it makes much sense either, and not really readable at first/second/tenth sight

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a function with more than a couple of optional arguments is broken anyhow, especially when the optional ones can precede others like in install. I wonder if it would make sense to change the Lolex API for a version 2 now, before adding yet another config option to something like lolex.install(config). That would scale with new options, such as this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. well that's up to you. I'm not sure if you have any releases planned but we can definitely refactor that part (and possibly others too).
Would be happy to pitch in for this too. Maybe you could point out other parts that need patching too.

Copy link
Contributor

Choose a reason for hiding this comment

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

This has now been done. Care to move the config?

@mroderick
Copy link
Member

@elad-nach would you mind providing a usage example? It would greatly help me wrap my head around how this is expected to work

@acud
Copy link
Contributor Author

acud commented Jun 29, 2017

@mroderick sure.

The thing is that today with the current usage of lolex, if a test depends on a timer executing - there's no way to know when to tick() the timers. if you set a timeout after the clock has been installed - it won't get triggered. if you set a loop to tick() periodically - the timer won't be set because the execution context is inside the for loop, and the code that should be setting the timer does not get executed as of such.

Y need to tick() just after the timer has been set. this is not possible without either stubbing or proxying methods, since you have to use an interrupt style method to pull the program counter out of the main execution context and into a context which ticks() the lolex timer, invoking any pending timers, then returns to execute the code being tested.
issue #85 will be addressed with this fix. this one too

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

This is almost ready for merge. What is missing is documentation on the how and why of this feature. Currently the docs are in the Readme. If you could add that (mostly done in #105 actually!) and move the config as discussed I will merge this today!

src/lolex-src.js Outdated
@@ -394,6 +398,12 @@ function hijackMethod(target, method, clock) {
target[method].clock = clock;
}

exports.shouldAdvanceTime = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This has now been done. Care to move the config?

@fatso83
Copy link
Contributor

fatso83 commented Jul 13, 2017

I would really like to merge this, but other than the pretty minor config details mentioned above, the only real thing missing is documentation on how this works for new users.

@kylefleming has already done a lot of the work on explaining the use-cases and differences between his async stuff and this, so that might help out.

Thanks a lot for the work you put into this!

@acud
Copy link
Contributor Author

acud commented Jul 13, 2017

@fatso83 I'm on it 👍
and speaking of documentation - it is still oriented towards the old install method signature.
would you cover the change regarding the install signature once the major version has been done? or should I update the install signature doc too?

cheers!

acud added 3 commits July 13, 2017 13:41
…vance

mocked time at a certain 'real' interval
…ses where this might be necessary

- updated shouldAdvanceTime functionality to support new install method signature
- moved time delta parameter to config
@fatso83
Copy link
Contributor

fatso83 commented Jul 13, 2017

Hi, Elad. I just noticed the docs were missing, so made #109. Whoever makes the docs is irrelevant to me - I am happy it just gets done by someone :)

@fatso83
Copy link
Contributor

fatso83 commented Jul 13, 2017

P.S. If you could rebase your branch to get rid of the merge commits when you are finished working on it it would be 👍

@benjamingr
Copy link
Member

Hey, looking at the diff I think this landed, right?

@fatso83
Copy link
Contributor

fatso83 commented Jul 13, 2017

@benjamingr not sure what you mean. It's not merged. But the current diff is messed up so don't assume anything based on that. Not sure what has happened, but nothing an interactive rebase won't fix 😋

@benjamingr
Copy link
Member

@fatso83 oh, cool - I got confused by the commit diff. Sorry!

@acud
Copy link
Contributor Author

acud commented Jul 14, 2017

@fatso83, i updated the readme and history. please review the changes.

also, readme line 80 - to hijack timers in another context - lolex-src line 643 - as I understand this translates into the target object in the config. please verify.

if you want me to clean the commits I could just patch a new fork and close this PR, as the initial commits were made before the new branches were merged (or just squash them when you merge the PR).

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

I think this is fine, and I'll just squash the commits. And reg. your question: the context and target is the same.

@@ -6,6 +6,8 @@ v2.0.0 / 2017-07-13
* Add support for performance.now (#106)
* Fix issue with tick(): setSystemClock then throw
* Update old dependencies
* Added support to automatically increment time (#85)
Copy link
Contributor

Choose a reason for hiding this comment

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

that's very considerate, but you can drop this. we create this semi-automatically when crafting a new release using git-extras

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

Successfully merging this pull request may close these issues.

4 participants