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

Conversation

eranshabi
Copy link
Contributor

Summary

In our projects we check getTimerCount() to know that all our timers/immediates are cleaned between tests.
I noticed that the number is getting larger after each test.
It seems that getTimerCount() counts cancelled immediates, this PR fixes this.

Test plan

added a test at jestFakeTimers.test.ts

@SimenB
Copy link
Member

SimenB commented Jul 30, 2019

Should cancelled immediates get removed so this._immediates is correct without the filter?

@jeysal
Copy link
Contributor

jeysal commented Jul 30, 2019

Looking through the usages, I didn't see a reason why there kept around yeah.
@eranshabi wanna try that for a cleaner solution? If all the tests and the new one stay green I think we can be pretty confident it works.

Copy link
Contributor

@lh0x00 lh0x00 left a comment

Choose a reason for hiding this comment

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

do you using Map instead of Array for this case and handle like timers ?

private _immediates!: Map<string, Tick>;

you can easy set, delete and clear but not change reference!

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This LGTM! Could you update the changelog as well, please? 🙂

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Left one comment, other than that LGTM (and changelog as Simen said)

packages/jest-fake-timers/src/jestFakeTimers.ts Outdated Show resolved Hide resolved
// 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

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Mostly LGTM now, just gotta make CI green :)

packages/jest-fake-timers/src/jestFakeTimers.ts Outdated Show resolved Hide resolved
@eranshabi
Copy link
Contributor Author

The CI test that fails is not using fake timers, so not sure why it's red. Will investigate...

@SimenB
Copy link
Member

SimenB commented Aug 16, 2019

CI is failing, though 😢

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants