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

getTimerCount will not include cancelled immediates #8764

Merged
merged 9 commits into from
Aug 18, 2019
Merged
Show file tree
Hide file tree
Changes from 4 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
17 changes: 17 additions & 0 deletions packages/jest-fake-timers/src/__tests__/jestFakeTimers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1303,5 +1303,22 @@ describe('FakeTimers', () => {

expect(timers.getTimerCount()).toEqual(3);
});

it('not includes cancelled immediates', () => {
const timers = new FakeTimers({
config,
global,
moduleMocker,
timerConfig,
});

timers.useFakeTimers();

global.setImmediate(() => {});
expect(timers.getTimerCount()).toEqual(1);
timers.clearAllTimers();

expect(timers.getTimerCount()).toEqual(0);
});
});
});
34 changes: 16 additions & 18 deletions packages/jest-fake-timers/src/jestFakeTimers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ const setGlobal = (
};

export default class FakeTimers<TimerRef> {
private _cancelledImmediates!: Record<string, boolean>;
private _cancelledTicks!: Record<string, boolean>;
private _config: StackTraceConfig;
private _disposed?: boolean;
Expand Down Expand Up @@ -105,9 +104,7 @@ export default class FakeTimers<TimerRef> {
}

clearAllTimers() {
this._immediates.forEach(immediate =>
this._fakeClearImmediate(immediate.uuid),
);
this._immediates = [];
this._timers.clear();
}

Expand All @@ -118,7 +115,6 @@ export default class FakeTimers<TimerRef> {

reset() {
this._cancelledTicks = {};
this._cancelledImmediates = {};
this._now = 0;
this._ticks = [];
this._immediates = [];
Expand Down Expand Up @@ -177,11 +173,8 @@ export default class FakeTimers<TimerRef> {
}

private _runImmediate(immediate: Tick) {
if (!this._cancelledImmediates.hasOwnProperty(immediate.uuid)) {
// Callback may throw, so update the map prior calling.
this._cancelledImmediates[immediate.uuid] = true;
immediate.callback();
}
immediate.callback();
jeysal marked this conversation as resolved.
Show resolved Hide resolved
this._fakeClearImmediate(immediate.uuid);
}

runAllTimers() {
Expand Down Expand Up @@ -402,7 +395,9 @@ export default class FakeTimers<TimerRef> {
}

private _fakeClearImmediate(uuid: TimerID) {
this._cancelledImmediates[uuid] = true;
this._immediates = this._immediates.filter(
immediate => immediate.uuid !== uuid,
);
}

private _fakeNextTick(callback: Callback, ...args: Array<any>) {
Expand Down Expand Up @@ -432,19 +427,22 @@ export default class FakeTimers<TimerRef> {
return null;
}

const uuid = this._uuidCounter++;
const uuid = String(this._uuidCounter++);

this._immediates.push({
callback: () => callback.apply(null, args),
uuid: String(uuid),
uuid,
});

const cancelledImmediates = this._cancelledImmediates;
const immediates = this._immediates;
this._timerAPIs.setImmediate(() => {
if (!cancelledImmediates.hasOwnProperty(uuid)) {
// Callback may throw, so update the map prior calling.
cancelledImmediates[String(uuid)] = true;
callback.apply(null, args);
if (immediates.find(x => x.uuid === uuid)) {
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please refer to this comment: #8764 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I mean, why do we have find at all?

Copy link
Contributor Author

@eranshabi eranshabi Aug 15, 2019

Choose a reason for hiding this comment

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

so we don't run immediates more than once.
If it was cleared (not in the immediates array) then it shouldn't run even if runAllImmediates is called

Copy link
Member

Choose a reason for hiding this comment

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

Right, makes sense

try {
callback.apply(null, args);
} catch (error) {
eranshabi marked this conversation as resolved.
Show resolved Hide resolved
} finally {
this._fakeClearImmediate(uuid);
}
}
});

Expand Down