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

Make now configurable so time can freeze outside of backburner #179

Merged
merged 2 commits into from
Dec 13, 2016

Conversation

pittst3r
Copy link
Contributor

@pittst3r pittst3r commented Dec 6, 2016

The main purpose of this PR is to initiate a discussion around the problem of freezing time while using Backburner. With PR #166 (and #12941 in Ember) we lost the ability to monkey-patch Date.now without stopping Backburner from running. The use case for freezing time is in testing. Being able to peg time to an arbitrary point while testing time-related things is invaluable in preventing non-determinism.

My proposal is to somehow make the concept of "now" configurable and this PR is my first pass at it, mostly a proof-of-concept. It would also be nice to have an explicit public API (as opposed to doing something like Ember.run.backburner._platform = … which makes me feel like I'm being bad–even just removing the underscore would be nice).

Looking forward to hearing y'alls thoughts. Thanks!

@stefanpenner
Copy link
Collaborator

stefanpenner commented Dec 6, 2016

There are many things that depend on a monotonically increasing system clock. Rather then patching all of them, clock stubbing libraries should likely just handle this.

If #166 broke the ability to stub time, because it now looks up Date.now(), it is likely because the time stubbing library was loaded after BB. Would it be simpler to go back to not looking up Date.now dynamically?


Also , it would be very useful to be describe the failure scenarios #166 introduced


Alternatively, an approach I have found quite good, is for my system to just use a shared clock abstraction. Which I control, and replace if needed. This leaves things like BB alone, but makes time aspects of my application work via the shared clock abstraction.

@pittst3r
Copy link
Contributor Author

pittst3r commented Dec 6, 2016

If #166 broke the ability to stub time, because it now looks up Date.now(), it is likely because the time stubbing library was loaded after BB.

That's precisely what's going on. After #166, Date.now gets evaluated after one has the chance to monkey-patch Date. Going back to not looking up Date.now dynamically sounds like a great solution.

I'll go back and update the PR accordingly and see if I can add some tests to paint a fuller picture.

@pittst3r pittst3r force-pushed the time-freezing branch 3 times, most recently from 04b331a to 1e0d68a Compare December 6, 2016 19:45
@pittst3r
Copy link
Contributor Author

pittst3r commented Dec 6, 2016

Okay, this is updated now. What do you think? I do like this better and like that it simply protects backburner from outside interference.

I wasn't able to come up with any failure scenarios other than the one described in the test I wrote. Basically #setTimeout will not tick if Date.now is not captured before the monkey patch.

Alternatively, an approach I have found quite good, is for my system to just use a shared clock abstraction. Which I control, and replace if needed. This leaves things like BB alone, but makes time aspects of my application work via the shared clock abstraction.

Thanks, that's an excellent suggestion. I'll look into doing this.

@toranb
Copy link

toranb commented Dec 13, 2016

@stefanpenner any chance you could give the code @robbiepitts has a look this week? Although I'm in favor of using a clock abstraction as you suggest, this is technically a regression and I'd love to see if we can fix it (so users who had a passing test suite on ember 2.3 don't break as they continue to upgrade ember over time)

@stefanpenner
Copy link
Collaborator

I believe I am good with this change. Would love another r? to sanity check:

@rwjblue r?

@stefanpenner
Copy link
Collaborator

paired with @rwjblue who gives 👍

@stefanpenner stefanpenner merged commit 8f48597 into BackburnerJS:master Dec 13, 2016
@pittst3r pittst3r deleted the time-freezing branch December 13, 2016 19:39
alias-mac added a commit to alias-mac/backburner.js that referenced this pull request Sep 21, 2017
This allow us to use fake timers, such as `sinon.useFakeTimers`, to
check the state of the app during the run loop.

For example: if a popup is supposed to be triggered in X ms from now and
it will show to the user during Y ms, then this will allow us to use
fake timers to pause between X and Y and check the state of the Ember
app at that exact time.

This is related with BackburnerJS#263, BackburnerJS#179 and BackburnerJS#166.

If someone needs to have the backburner clock separate from the global
stubbed clock (as explained in BackburnerJS#179), then `_platform.now` config should
be used to pass the native `Date.now` function.
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.

3 participants