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

Add FakeAsync.runNextTimer #85

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
## 1.3.2-wip
## 1.4.0-wip

* Require Dart 3.3
* Add `FakeAsync.runNextTimer`, a single-step analogue
of `FakeAsync.flushTimers`.

## 1.3.1

Expand Down
61 changes: 36 additions & 25 deletions lib/fake_async.dart
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class FakeAsync {
}

_elapsingTo = _elapsed + duration;
_fireTimersWhile((next) => next._nextCall <= _elapsingTo!);
while (runNextTimer(timeout: _elapsingTo! - _elapsed)) {}
Copy link
Member

Choose a reason for hiding this comment

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

I'd make an internal helper _runNextTimer([Duration? until]) that doesn't take a duration relative to now, but takes the actual end time (duration since initialTime).

That's the time (_elapsingTo) that you already have here, and it's the first thing that runNextTimer computes internally anyway.
It avoids repeatedly doing _elapsingTo - _elapsed.

So:

  final elapsingTo = _elapsingTo = _elapsed + duration;
  while (_runNextTimer(elapsingTo)) {}

Then the public function would be:

bool runNextTimer({Duration? timeout]) {
   if (timeout == null) return runNextTimer();
   var timeoutTime = _elapsed + timeout;
   if (_runNextTimer(timeoutTime)) {
     return true;
   } else {
     _elapsed = timeoutTime;
     return false;
   }
}

(I suggest advancing time by at least the [timeout]. If not, it should be renamed to something else, like before. A timeout only applies if time has actually advanced.)

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'd make an internal helper _runNextTimer([Duration? until]) that doesn't take a duration relative to now, but takes the actual end time (duration since initialTime).

Sure, done.

(Though the tests informed me that that final elapsingTo = … version of this call site isn't quite right — the "timers expiring due to elapseBlocking" test shows this loop needs to re-read the _elapsingTo property because it may have changed.)

That's the time (_elapsingTo) that you already have here, and it's the first thing that runNextTimer computes internally anyway.
It avoids repeatedly doing _elapsingTo - _elapsed.

Yeah, those repeated subtractions did feel a bit silly.

This also prompts the thought that perhaps it'd even be good to go a bit further in this direction: perhaps those repeated subtractions are a sign that the actual end time should be what's in the public interface, too.

In particular, just like the two internal callers here, my external use case (gnprice/zulip-flutter@1ad0a6f#diff-03f5c714d04c27b23d2efc576efe97e08095be869e3021d017434bbd88205944R33) also starts by computing the desired end time, and repeatedly subtracts elapsed:

  const timeout = Duration(hours: 1);
  final absoluteTimeout = async.elapsed + timeout;
  while (async.runNextTimer(timeout: absoluteTimeout - async.elapsed)) {

_elapseTo(_elapsingTo!);
_elapsingTo = null;
}
Expand Down Expand Up @@ -211,39 +211,50 @@ class FakeAsync {
{Duration timeout = const Duration(hours: 1),
bool flushPeriodicTimers = true}) {
final absoluteTimeout = _elapsed + timeout;
_fireTimersWhile((timer) {
if (timer._nextCall > absoluteTimeout) {
// TODO(nweiz): Make this a [TimeoutException].
throw StateError('Exceeded timeout $timeout while flushing timers');
for (;;) {
Copy link
Member

Choose a reason for hiding this comment

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

Usually use while (true) { for this pattern in Dart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, changed.

(That's usually what I'd write too; the for (;;) was preserved from the old _fireTimersWhile.)

// With [flushPeriodicTimers] false, continue firing timers only until
// all remaining timers are periodic *and* every periodic timer has had
// a chance to run against the final value of [_elapsed].
if (!flushPeriodicTimers) {
if (_timers
Copy link
Member

Choose a reason for hiding this comment

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

(Just a comment: So much of this code would be easier to read (and more efficient) if _timers was a priority queue. And if it tracked the number of periodic timers on the side, this check would just be _timers.length == _periodicTimerCount. All this repeated iteration only works because it's for testing only, and there aren't that many timers in one test.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

.every((timer) => timer.isPeriodic && timer._nextCall > _elapsed)) {
break;
}
}

if (flushPeriodicTimers) return _timers.isNotEmpty;
if (!runNextTimer(timeout: absoluteTimeout - _elapsed)) {
if (_timers.isEmpty) break;

// Continue firing timers until the only ones left are periodic *and*
// every periodic timer has had a change to run against the final
// value of [_elapsed].
return _timers
.any((timer) => !timer.isPeriodic || timer._nextCall <= _elapsed);
});
// TODO(nweiz): Make this a [TimeoutException].
throw StateError('Exceeded timeout $timeout while flushing timers');
}
}
}

/// Invoke the callback for each timer until [predicate] returns `false` for
/// the next timer that would be fired.
/// Elapses time to run one timer, if any timer exists.
///
/// Microtasks are flushed before and after each timer is fired. Before each
/// timer fires, [_elapsed] is updated to the appropriate duration.
void _fireTimersWhile(bool Function(FakeTimer timer) predicate) {
flushMicrotasks();
for (;;) {
if (_timers.isEmpty) break;
/// Microtasks are flushed before and after the timer runs. Before the
Copy link
Member

Choose a reason for hiding this comment

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

Mention that the current microtask queue is flushed whether or not there is a timer in range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

/// timer runs, [elapsed] is updated to the appropriate value.
///
/// The [timeout] controls how much fake time may elapse. If non-null,
/// then timers further in the future than the given duration will be ignored.
Copy link
Member

Choose a reason for hiding this comment

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

I would still advance the time by timeout before returning false.
Otherwise the name should be different. A timeout triggering means that the time has passed.

I can see that flushTimers doesn't do that, but that's because it throws an error if all (non-periodic) timers are not flushed before the timeout is up, and presumably the test fails then, so advancing the time doesn't matter.
That is, the timeout parameter to runNextTimer is not the same as the one to flushTimers. The latter means "it's an error if we reach it", the former is just "only try to go this far".

If we change the call in flushTimers to the internal _runNextTimer, and it's only the public runNextTimer that advance time on a false result, then we won't change the behavior of flushTimers.

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 would still advance the time by timeout before returning false.
Otherwise the name should be different. A timeout triggering means that the time has passed.
[…]
That is, the timeout parameter to runNextTimer is not the same as the one to flushTimers. The latter means "it's an error if we reach it", the former is just "only try to go this far".

Hmm, yeah, I agree — "timeout" isn't the right word to describe this behavior.

I think the "only try to go this far" behavior is the one I'd prefer to expose, though. In particular it's easy for a caller to use that behavior to implement the "error if we reach this" behavior, the same way flushTimers does; but if the method has already advanced time, then the caller can't undo that.

(The flushTimers implementation uses _timers.isEmpty, but an external caller can say .pendingTimers.isEmpty. That's inefficient because currently pendingTimers allocates a List, but there's no reason it should be inefficient to ask that question; I think the inefficiency there is just a case of what you pointed out at #85 (comment) above.)

So I'd prefer to find a better name for this parameter, rather than change the behavior to match the timeout name. Perhaps within? Or before?

///
/// Returns true if a timer was run, false otherwise.
natebosch marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I'd code-quote true and false. Generally try to code-quote literals when I refer to language-semantic values, like these, void and null, "done" or 42, to distinguish them from plain text containg the same symbols as English words or numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks. Done.

bool runNextTimer({Duration? timeout}) {
final absoluteTimeout = timeout == null ? null : _elapsed + timeout;
Copy link
Member

Choose a reason for hiding this comment

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

That is, I'd start the internal bool _runNextTimer(Duration? absoluteTimeout) {
here instead.


final timer = minBy(_timers, (FakeTimer timer) => timer._nextCall)!;
if (!predicate(timer)) break;
flushMicrotasks();

_elapseTo(timer._nextCall);
timer._fire();
flushMicrotasks();
if (_timers.isEmpty) return false;
final timer = minBy(_timers, (FakeTimer timer) => timer._nextCall)!;
if (absoluteTimeout != null && timer._nextCall > absoluteTimeout) {
return false;
}

_elapseTo(timer._nextCall);
timer._fire();
flushMicrotasks();
return true;
}

/// Creates a new timer controlled by `this` that fires [callback] after
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: fake_async
version: 1.3.2-wip
version: 1.4.0-wip
description: >-
Fake asynchronous events such as timers and microtasks for deterministic
testing.
Expand Down
95 changes: 95 additions & 0 deletions test/fake_async_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,101 @@ void main() {
});
});

group('runNextTimer', () {
test('should run the earliest timer', () {
FakeAsync().run((async) {
var last = 0;
Timer(const Duration(days: 2), () => last = 2);
Timer(const Duration(days: 1), () => last = 1);
Timer(const Duration(days: 3), () => last = 3);
expect(async.runNextTimer(), true);
expect(last, 1);
});
});

test('should return false if no timers exist', () {
FakeAsync().run((async) {
expect(async.runNextTimer(), false);
});
});

test('should run microtasks before choosing timer', () {
FakeAsync().run((async) {
var last = 0;
Timer(const Duration(days: 2), () => last = 2);
scheduleMicrotask(() => Timer(const Duration(days: 1), () => last = 1));
expect(async.runNextTimer(), true);
expect(last, 1);
expect(async.runNextTimer(), true);
expect(last, 2);
});
});

test('should run microtasks before deciding no timers exist', () {
FakeAsync().run((async) {
var last = 0;
scheduleMicrotask(() => Timer(const Duration(days: 1), () => last = 1));
expect(async.runNextTimer(), true);
expect(last, 1);
});
});

test('should run microtasks after timer', () {
FakeAsync().run((async) {
var ran = false;
Timer.run(() => scheduleMicrotask(() => ran = true));
expect(async.runNextTimer(), true);
expect(ran, true);
});
});

test('should update elapsed before running timer', () {
FakeAsync().run((async) {
Duration? time;
Timer(const Duration(days: 1), () => time = async.elapsed);
expect(async.runNextTimer(), true);
expect(time, const Duration(days: 1));
});
});

test('should apply timeout', () {
FakeAsync().run((async) {
var ran = false;
Timer(const Duration(days: 1), () => ran = true);
expect(async.runNextTimer(timeout: const Duration(hours: 1)), false);
expect(ran, false);
});
});

test('should apply timeout as non-strict bound', () {
FakeAsync().run((async) {
var ran = false;
Timer(const Duration(hours: 1), () => ran = true);
expect(async.runNextTimer(timeout: const Duration(hours: 1)), true);
expect(ran, true);
});
});

test('should apply timeout relative to current time', () {
FakeAsync().run((async) {
var ran = false;
Timer(const Duration(hours: 3), () => ran = true);
async.elapse(const Duration(hours: 2));
expect(async.runNextTimer(timeout: const Duration(hours: 2)), true);
expect(ran, true);
});
});

test('should have no timeout by default', () {
FakeAsync().run((async) {
var ran = false;
Timer(const Duration(microseconds: 1 << 52), () => ran = true);
expect(async.runNextTimer(), true);
expect(ran, true);
});
});
});

group('stats', () {
test('should report the number of pending microtasks', () {
FakeAsync().run((async) {
Expand Down