Add FakeClock

[seaneagan]

This is similar to:

http://sinonjs.org/docs/#clock

(I think this could replace CreateTimer and CreateTimerPeriodic, which IMO exposes too much of the internal implementation of whatever is taking it is a dependency.)

[seaneagan]

My initial plans for this are to use it to test a throttle/debounce implementation for quiver.streams.

fyi: I tried to replace advance with just using Future.delayed within the zone, but it doesn't work because there is no way to know that you are the last timer firing at a given instant of the clock, and advancing slightly past the desired time by a millisecond is a poor experience, and may not work for some use cases. Also, there would need to be a way to tell it to stop executing infinitely repeating timers.

[seaneagan]

@justinfagnani @yjbanov @cbracken anyone had a chance to look at this yet? Need to get this added before I can start working on #116.

[yjbanov]

Thanks for the reminder. Busy quarter :)

Looking at it right now.

[yjbanov]

I think you are taking it a bit too far with zones and async. Clock is much simpler in design than http://sinonjs.org/docs/#clock. All it does is tell you current time and time relative to current time. It's fully synchronous. A FakeClock implementation should only let you manipulate current time, and do that synchronously. If you want to advance clock asynchronously you should setup a separate (fake)timer to do that. Setting up zones should also come from outside.

I think the following interface is all it should have:

abstract class FakeClock extends Clock {
  factory FakeClock({DateTime initialTime});
  advance(Duration d);
  set now(DateTime dt);
}

That said, I think there is value in the async and zone utilities you added. I just think they belong in separate libraries. I think what you are trying to achieve can be done via three orthogonal utilities: FakeClock, FakeTimer and FakeZone. They should be usable independently or together. Then you can easily build your own utility that advances clocks using a time and binds things to specific zones.

Anybody else have different thoughts?

[seaneagan]

Thanks Yegor.

I agree with keeping Clock simple, see #92. In this vein, I considered separating the read/write parts of the API, similar to Completer/Future and SreamController/Stream. That would look like:

class ClockAdvancer {
  Future advance(Duration duration);
  void advanceSync(Duration duration);
  Zone get zone;
  Clock get clock;
}

I initially decided that that wasn't necessary since APIs will take a Clock as a dependency, and hopefully users will know not to do things like if(clock is FakeClock) clock.advance(...) etc. in their production code. But nonetheless, it would be cleaner, so I'd be happy to make this change, just let me know.

I do not agree with a separate FakeTimer/CreateTimer/CreateTimerPeriodic API. Callers should not have to know how their callee (and all of its transitive callees) internally use Timers. Instead they should only have to know that the callee depends on the current time, by taking a Clock as a dependency. The same is true for FakeStopwatch, APIs which use stopwatches should just take a Clock as a dependency, and then pass this Clock (or a closure of it's now method) to something like FakeStopwatch.

Timer usage can be very complex. For example with #116 which is my initial use case for this, both periodic and single shot timers are getting created and canceled all the time and in very dynamic ways. Another example, core APIs which internally use Timers such as Future.delayed, Future.timeout, Stream.periodic, and Stream.timeout will never provide a CreateTimer or similar dependency, so these core APIs couldn't be used within that framework.

So let me explain how this all works...

Luckily, the createTimer and createPeriodicTimer ZoneSpecification hooks allow us to externally observe how Timers are being used, so this PR takes advantage of that. All timers created in the zone are stored for later usage with advance (see below).

When the time is advanced synchronously (advanceSync), the timers should not be called. This simulates blocking or expensive calls, such as a sync File system access.

Asynchronous time advancement (advance), is the more common case, and is where Timers come in to play. When calling advance, any stored Timers scheduled to expire within that time advancement frame are executed, in the correct order, at the correct time, and only if they were not already canceled. Each timer is called in its own event loop frame to robustly simulate real timer behavior. For example, microtasks need to get called between Timers. It uses the real (root zone's) Timer.run to schedule the next timer expiration in the next event loop, so we don't have to actually wait the specified amount of time. Calling a timer callback can cause new Timers to be created or canceled, so each time it searches the entire set of stored timers for the next scheduled. When a timer is canceled or expires (if non-periodic), it is removed from the storage mechanism to make later searches quicker and avoid memory leaks. Once there are no more active timers, or the clock has advanced the specified amount, then the Future returned by advance is completed. As you can see the management of these fake timers in coordination with the advancement of the clock, is quite complex, so this PR attempts to encapsulate all this in an easy to use API so users don't have to come up with one-off solutions for each test they write.

Does that help?

[yjbanov]

Thanks for the detailed explanation. That's roughly how I understood your intention, and I agree that it is very useful. There is, however, a different approach to this that Angular.dart adopted:

https://github.com/angular/angular.dart/blob/master/lib/mock/zone.dart

The combination of async, microLeap and clockTick allow you to test a good range of async code. There are a couple of nice properties about this approach:

• It is not married to Clock. It is useful even to people who don't use Clock or any Quiver utilities. Orthogonality is part of Quiver's approach. It is primarily a collection of small independent utilities.
• It keeps your test code linear. There are no callbacks.

Example:

// Let's test this function
void testedFunc(Clock clock, List msgs) {
  msgs.add(clock.now());
  scheduleMicrotask(() {
    msgs.add(clock.now());
    new Future.delayed(new Duration(days: 2), () {
      msgs.add(clock.now());
    });
  });
}

test('should succeed', async(() { // note the [async] wrapper
  var d = new DateTime(2014, 3, 21);
  var c = new Clock(() => d);
  var msgs = [];
  testedFunc(c, msgs);
  print(msgs); // [2014-03-21 00:00:00.000]

  d = new DateTime(2014, 3, 22);
  microLeap();
  print(msgs); // [2014-03-21 00:00:00.000, 2014-03-22 00:00:00.000]

  d = new DateTime(2014, 3, 23);
  clockTick(days: 1);
  print(msgs); // [2014-03-21 00:00:00.000, 2014-03-22 00:00:00.000]
               // because the future is delayed until 2 days after now,
               // going 1 day forwards in time doesn't reach that time yet.

  d = new DateTime(2014, 3, 25);
  clockTick(days: 2);
  print(msgs); // [2014-03-21 00:00:00.000, 2014-03-22 00:00:00.000, 2014-03-25 00:00:00.000]
               // now we reached the point when the delayed Future is
               // scheduled to complete.
}));

I was thinking of porting it to Quiver at some point. Could you have a look and give your thoughts?

[seaneagan]

Thanks for the link, didn't know about it. After looking it over, it appears to be pretty limited and often incorrect for time simulation. Here's what I found:

• It doesn't run timers in the correct order, looks like it just runs them in the order they were created, and runs periodic timers many times in between other timers regardless.
• It doesn't run timers that are added as a result of other microtask or timer callbacks.
• Since the timers are called synchronously (in the same event loop frame) it doesn't allow you to process any other events added to the event queue, such as click events, between timers.
• Since it doesn't integrate with Clock
  • there is no way for the unit under test to access the current time, for example how much a Stopwatch has elapsed, which is often used to determine further timer scheduling.
  • it stores the elapsed time in each timer, which is just weird.
• It doesn't forward zone hook calls to the parent Zone, which means at the very least it would break https://codereview.chromium.org/133313004/ if not for the fact that it doesn't override the handleUncaughtError hook.
• It uses global state and the top-level methods close over that. That is likely to bleed state from one test into the next, although it looks like it tries to clean up the state at the beginning of each async call, not sure about interaction between calls or nested calls. Also, global state is just bad in general.

This PR covers all of this, and has tests for most of it.

I guess sync is for ensuring no side effects occur after the test is complete. microLeap brings you to the end of the micro task queue, but I'm not sure what the use case is. These could be added as instance methods to FakeClock or ClockAdvancer in a later PR.

If we don't want to marry to Clock, could just provide a now method instead:

class ClockAdvancer {
  // ...
  DateTime now();
}
// ...
var clock = new Clock(clockAdvancer.now);

In your example, you had to update d before calling clockTick. If there were any timers or microtasks which fired before the 2 day later one, then they wouldn't see the correct time. Since this is just sample code you know that's not a concern, but in non-trivial scenarios it often is. And as mentioned before, tests shouldn't assume implementation details about how timers are being used.

Thoughts?

[seaneagan]

@yjbanov PTAL

I removed the dependency on Clock as suggested, and renamed to FakeTime.

I also replaced the exposed zone with a simple run method to make test code simpler. Expected usage pattern is:

test('testedFunc', () {
  var time = new FakeTime(initialTime: initialTime);
  return time.run(() {
    testedFunc(now: time.now);
    return time.advance(duration).then((_) => expect(...));
  });
});

[seaneagan]

@yjbanov @justinfagnani Any other changes I can make to get this landed? Thanks.

[seaneagan]

@cbracken @kevmoo maybe one of you can take a look? or someone else ? Thanks!

[justinfagnani]

Sorry Sean, end of quarter madness the last two weeks over here. I've been
talking about this w/ Yegor, and I think we can come back to it on Monday.

On Fri, Apr 4, 2014 at 1:20 PM, Sean Eagan notifications@github.com wrote:

@cbracken https://github.com/cbracken @kevmoohttps://github.com/kevmoomaybe one of you can take a look? or someone else ? Thanks!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/98#issuecomment-39607170
.

[yjbanov]

Meeting @justinfagnani Monday to discuss it.

[seaneagan]

Great! Here's some other stuff to consider: Rename advance / advanceSync to elapse / elapseSync? I'm not sure tick / tickSync makes sense if it's no longer a Clock. Also, could change run to a static method which takes a callback which takes the FakeTime as an argument:

test('testedFunc', () => FakeTime.run(initialTime, (FakeTime time) {
    testedFunc(now: time.now);
    return time.advance(duration).then((_) => expect(...));
}));

But then initialTime becomes required (maybe that's better anyway?) and any future optional args would have to come after the required callback, which would be very hard to read.

[yjbanov]

Better yet:

var clock = new FakeClock(initialTime);  // when we have one

test('testedFunc', withFakeTimer((FakeTimer timer) {
    testedFunc(now: clock.now);
    return time.advance(duration).then((_) => expect(...));
}, beforeAdvance: clock.advance));

This way FakeClock is in charge of absolute time. FakeTimer is in charge of advancing the time.

The signature of withFakeTimer could be:

Function withFakeTimer(testFn(FakeTimer timer), {beforeAdvance(Duration advancedBy)});

The purpose of beforeAdvance is to keep FakeClock (and/or whatever else you might have) in sync with FakeTimer. It would be called just before firing the timer so that when the tested timer callback gets time from Clock it is already advanced.

Note how I covertly renamed FakeTime to FakeTimer :) This is because we can have it focus on Timer-related things and remove now() and advanceSync, the former being supplied by FakeClock and the latter being supplied by beforeAdvance.

[yjbanov]

Oh, and to match the Dart SDK package structure, we should move this under testing/async. We don't want the time package to depend on dart:async.

[yjbanov]

Actually, I take the rename part back. FakeTime is better.

[seaneagan]

PTAL

I love the idea of separating the absolute time from the advancement of time. I don't like having 2 separate interfaces for advancing time, and having to tie those together for the common case. Synchronous advancement of time is less common, so having a separate synchronous FakeClock would be error prone and confusing. And a push model like beforeAdvance, would expose how Timers are internally being used, which should be avoided.

I accomplished the same separation of absolute time by replacing FakeTime.now with FakeTime.elapsed (and renaming advance to elapse).

I also kept run as an instance method to allow for subclassing and constructor parameters, for example, may want to add an optional minimum duration between Timers to prevent infinitely looping timers. But the run callback now takes the FakeTime as a parameter similar to periodic timer callbacks to avoid having to assign the FakeTime instance to a variable. So the updated usage pattern is now:

test('testedFunc', () => new FakeTime().run((time) {
    testedFunc(now: () => initialTime.add(time.elapsed));
    return time.elapse(duration).then((_) => expect(...));
}));

[yjbanov]

The goal of Clock has been to provide a testable set of time-related facilities on top of an absolute time function. They are currently being used in downstream projects. It seems this refactoring will be a major API breaking change. Let's come up with a plan that considers it.

Also, I think the refactoring of Clock is not related to this PR. Let's log a separate issue and discuss it there.

[seaneagan]

@yjbanov Agreed. Previous commit changed FakeTime.elapsed to FakeTime.clock, for that to work, I would just need to be able to add newStopwatch method to Clock (in a later PR). That shouldn't break any implementers of Clock as they should all be extending Clock or more likely just calling the constructors. Is that agreeable?

[seaneagan]

@justinfagnani @yjbanov PTAL elapse is now sync, and elapseSync is renamed to elapseBlocking. Let me know if any other changes are needed for this initial version.

[yjbanov]

I think here we need only check _nextCall <= _elapsingTo, because _elapsingTo is never null and _now <= _elapsingTo holds, making _nextCall <= _now and _elapsingTo != null checks redundant.

[seaneagan]

Agree that it cannot be null, fixed.

Calls to elapseBlocking from timers/microtasks called during elapse, can cause the current fake time to surpass elapsingTo, in which case those timers expiring all the way up to the current time need to be included. Otherwise, the state of the timers and the clock will be out of sync on the return from elapse, and it would be surprising to have a bunch of timers called on the next call to say elapse(Duration.ZERO).

So I now update _elapsingTo in elapseBlocking when _elapsingTo is surpassed by the clock.

[yjbanov]

This version looks great. I only have a couple of cosmetic suggestions for the implementation.

One last bit I'm still not sure about is the name. This class not only fakes time, but more precisely Dart's entire event loop. What do you think of FakeEventLoop? Too mouthful? How about FakeAsync? After all, the event loop is all about async.

I have some ideas for expanding the functionality of this class, but I think for an initial version this is great.

@justinfagnani, @cbracken, could one of you have a look at the code w.r.t. style? I've been staring at this code for too long :)

[seaneagan]

@yjbanov

Yes, the timer throttling TODO is still relevant. Nested timer chains (including periodic timers) which run without advancing the clock (zero duraton) will lead to an infinite loop causing elapse to never return. That could be avoided by clamping the duration to some positive value. Apparently in browser JavaScript this is 4ms once sufficiently nested. I tested in the standalone VM and dartium, and there doesn't appear to be any throttling there, it's only limited by the execution speed. Could make it configurable in the future, but I think the default ought to be Duration.ZERO as it currently is.

Regarding the name, I like FakeTime best. We're only faking timers and microtasks. The event loop also includes a bunch of other async events (http, io, etc.) that can be faked, but not by this utility, and outside any involvement with the event loop. Also, we're currently only faking microtasks in order to make elapse sync, though it would allow us to add elapseMicro in the future, similar to microLeap from angular. It's already in the impl as _drainMicrotasks, just need to identify a use case before adding it. We can think of microtasks as time elapsing as well, even though it doesn't advance the clock (barring usage of elapseBlocking), we're still simulating the passage of some infinitesimal amount of time that it takes to process microtasks.

[seaneagan]

@justinfagnani any chance we can get this merged this week? Thanks!

[seaneagan]

@yjbanov @justinfagnani @cbracken @anyone ? @bueller ?

[yjbanov]

@seaneagan, I'm ready to merge your PR as soon as you rename FakeTime/fake_time to FakeAsync/fake_async.

[seaneagan]

@yjbanov Thanks. Can you please address my comments above about why I prefer FakeTime. As it currently exists, the purpose of this class is to fake the passage of time, not the event loop, and not all things async. As I mentioned, it's currently only faking microtasks to allow elapse to be sync. I would have preferred to leave elapse async, and have the faking of microtasks (once a use is identified) to be a separate utility, but I got over that :). And if we do want to expose that, I think conceptualizing it as elapseMicro works pretty well.

[yjbanov]

@seaneagan, I understand your reasoning. There is a high level of isomorphism between the operation of the event loop and the the passage of time, so I am actually not worried about the descriptiveness of either "time" or "async" (or even "entropy"). The primary reason I'd like this to be called FakeAsync is because we are faking stuff from dart:async library, which includes zones, timers and microtasks. Note also that dart:async does not include any I/O, HTTP or DOM events. So if a unit-test needs to deal with any of them it will have to deal with real asynchrony (probably via expectAsync).

And yes, I think we should expand it to include flushing of microtasks, etc. I have lots of use-cases for this. But not in this PR. It can be added incrementally. In the future, when I/O can be faked, it will likely be through zones too, and we'll be able to expand the functionality further without carrying the "time" bias.

[seaneagan]

@justinfagnani

We could really use a tie-breaking vote here. Can you help us out?

@yjbanov I don't agree that I/O (or HTTP or DOM) will be faked by Zones in the future, and thus not by this class, and more importantly, should not be. You said above:

"Orthogonality is part of Quiver's approach. It is primarily a collection of small independent utilities."

i.e. "do one thing well". For this class, that one thing is faking time, thus FakeTime.

I think the dart:async analogy is stretching. This class does fake some things from dart:async... Timers and microtasks. It uses Zones to do this. It doesn't fake the most familiar parts of dart:async i.e. Futures, Completers, Streams, StreamControllers etc.

[seaneagan]

@yjbanov

ok, FakeAsync it is then. Thanks in advance for getting this merged quickly! Can't wait to finally start using it!

[yjbanov]

Thank you, Sean, for you contribution and patience :)