-
Notifications
You must be signed in to change notification settings - Fork 409
Ensure FakeAsyncTestZoneSpec tick always doTick #1099
Conversation
lib/zone-spec/fake-async-test.ts
Outdated
this._currentTime = finalTime; | ||
if (doTick) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the code is added here, the doTick
may be triggered twice.
in your test case, when setTimeout
callback is triggered, the doTick
will be called twice.
consider the following case.
it('should run doTick callback even if no work ran', () => {
fakeAsyncTestZone.run(() => {
let log: number[] = [];
function doTick(elapsed: number) {
log.push(elapsed);
}
setTimeout(() => {}, 10);
testZoneSpec.tick(6, doTick);
testZoneSpec.tick(6, doTick);
testZoneSpec.tick(6, doTick);
});
});
the log will be [6, 4, 2, 6]
which I think will be correct.
But if the setTimeout delay is 12
.
the log will be [6. 6. 0, 6]
. I think we should not trigger doTick
with this 0
.
So we should add some logic to avoid this, such as
tick(millis: number = 0, doTick?: (elapsed: number) => void): void {
let finalTime = this._currentTime + millis;
let lastCurrentTime = 0;
if (this._schedulerQueue.length === 0 && doTick) {
doTick(millis);
return;
}
let queueConsumed = false;
while (this._schedulerQueue.length > 0) {
let current = this._schedulerQueue[0];
if (finalTime < current.endTime) {
// Done processing the queue since it's sorted by endTime.
break;
} else {
// Time to run scheduled function. Remove it from the head of queue.
queueConsumed = true;
let current = this._schedulerQueue.shift();
lastCurrentTime = this._currentTime;
this._currentTime = current.endTime;
if (doTick) {
doTick(this._currentTime - lastCurrentTime);
}
let retval = current.func.apply(global, current.args);
if (!retval) {
// Uncaught exception in the current scheduled function. Stop processing the queue.
break;
}
}
}
lastCurrentTime = this._currentTime;
this._currentTime = finalTime;
// if some task in queue have been invoked and the invoked task endTime equals
// finalTime, we should not doTick with a zero again.
if (!(queueConsumed && this._currentTime === lastCurrentTime) && doTick) {
doTick(this._currentTime - lastCurrentTime);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doTick
will already be called with 0 today when there are multiple functions scheduled for execution at the same end time since doTick
will always be called every time a scheduled function is invoked. Generally it's very unclear what the behavior of this callback should be since I couldn't find any existing tests for it.
I'm also not sure what queueConsumed
in your snipit is for. Regardless of what scheduled work ran or not this function should always make sure it ticks any remaining time. And if you believe that doTick
should not be invoked with 0 then the this._currentTime === lastCurrentTime
check should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doTick will already be called with 0 today when there are multiple functions scheduled for execution at the same end time since doTick will always be called every time a scheduled function is invoked.
This is a little different, because in this case, doTick is triggered because the scheduled function is invoked. But consider the doTick
name, it should mean fakeTimer ticked
, so if several scheduled task endTime are the same, we should only doTick
once.
queueConsumed
just means if the doTick
is triggered once by the scheduled function, and the finalTime equals to currentTime, we should not doTick
again, but if doTick
is not triggered by schedule function, even finalTime equals to currentTime, we still need to call doTick
.
Generally it's very unclear what the behavior of this callback should be since I couldn't find any existing tests for it.
Yeah, this is old code, I am not sure when we should call doTick
either, I will clarify the requirement and let's discuss again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mitchellwills
The doTick
callback is from here. #866
The purpose is to advance Jasmine.mockDate
, and in current newest zone.js
, jasmine.mockDate
has been advanced automatically, so we don't need to do that already.
So I think you may have another use case to use doTick
, could you share your use case?
And I think the requirement should be,
doTick should be called when fake timer advanced.
So
- multiple scheduled macroTasks with the same end time should not trigger multiple
doTick
. 0
should be triggered, but still only once.
The reason that we should only doTick
once, because consider the following case.
setTimeout(function() {
}, 0);
setTimeout(function() {
}, 0);
tick(0, millis => console.log);
the result should be
0 // triggered by the 1st timeout
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My use case is also advancing the clock when integrating with some tooling we have.
My view on this was that either there are very clear semantics on when doTick fires or it should be considered an implementation detail in which case it doesn't matter if doTick is called as long as the durations add up to the correct amount and it's best to just keep the code as simple as possible. Are there any other use cases aside from advancing the clock.
A few options:
- Fires after each macro task
- Only called with a non-zero value
- Once for each macro task time advance and then the duration from the last macro task to the end time, possibly 0 (I think this is what you suggested)
- It's an implementation detail and the only guarantee is that it will be called to increment time the correct amount
Some other edge cases
setTimeout(function() {}, 10);
setTimeout(function() {}, 10);
tick(10, millis => console.log);
// should this print 10 or 10, 0
setTimeout(function() {}, 0);
setTimeout(function() {}, 0);
tick(10, millis => console.log);
// should this print 0, 10 or 10
My personal opinion would be whatever leads to the simplest code unless there are specific semantics that would be advantageous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only called with a non-zero value
I am not sure about this one, I think doTick
should fire when setTimeout(() => {}, 0); tick()
.
But if you mean Fires after each macro task
is a stronger rule, I think it will be ok.
About the sample code you provided,
setTimeout(function() {}, 10);
setTimeout(function() {}, 10);
tick(10, millis => console.log);
// should this print 10 or 10, 0
// I prefer 10 here.
setTimeout(function() {}, 0);
setTimeout(function() {}, 0);
tick(10, millis => console.log);
// should this print 0, 10 or 10
// I prefer 0, 10 here
So the rule will be
- macroTask will fire
doTick
regardless of the duration is zero or not. - If not match the rule 1, no macroTasks triggered, then we should also trigger
doTick
regardless of the duration is zero or not. - If rule 1 is matched, some macroTask trigger
doTick
already, and after the trigger, the last triggered macroTasks' endTime equals finalTime, we should notdoTick
again with a zero.
// rule 1
setTimeout(function() {}, 0);
tick(0, (millis) => console.log);
// should print 0
// rule 2 and rule3
setTimeout(function(){}, 10);
tick(0, (millis) => console.log);
// should print 0
tick(5, (millis) => console.log);
// should print 5
tick(10, (millis) => console.log);
// should print 10 (not 10, 0)
what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be fine; however, I haven't seen any justification for the additional complexity.
If there comes a time where having a specific behavior makes sense then it could be added, but until then is there any reason to overconstrain the behavior (more complex to maintain, harder to change in the future, etc)? As long at the function fulfills advancing a clock the correct amount of time it doesn't seem like there is any need to define the behavior further than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your point. I believe currently doTick
was only used inside google
for their tests, so your current PR logic (may cause multiple doTick
call) may cause some breaking changes in their test cases, I think we need @vikerman to verify it.
So my complex version just want to reduce the risk to impact on google
test cases, anyway, I think this one need to be reviewed by @vikerman .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
@vikerman I did run a global presubmit with the google code base. There were only one failure, but I don't think they were because of the number of calls to doTick.
@vikerman , could you help to review this one (especially about when to trigger |
@mitchellwills, some other PR was merged, could you rebase this one? thanks. |
In the case where there is pending work in the scheduler queue, but the duration of the tick did not causes it to run the doTick callback would not be called (or would not be called with intervals summing to the total time ellapsed).
Rebased. |
In the case where there is pending work in the scheduler queue, but the duration
of the tick did not causes it to run the doTick callback would not be called (or
would not be called with intervals summing to the total time ellapsed).