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.

Sign up for GitHub

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

Jump to bottom

getTimerCount will not include cancelled immediates #8764

Merged
merged 9 commits into from
Aug 18, 2019
Merged

getTimerCount will not include cancelled immediates #8764

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.

lh0x00
lh0x00 suggested changes Aug 2, 2019
View reviewed changes
Copy link
Contributor

@lh0x00 lh0x00 left a comment •
edited
Loading

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!

SimenB
SimenB approved these changes Aug 10, 2019
View reviewed changes
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? 🙂

jeysal
jeysal reviewed Aug 11, 2019
View reviewed changes
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
SimenB
SimenB reviewed Aug 15, 2019
View reviewed changes
packages/jest-fake-timers/src/jestFakeTimers.ts Outdated
// 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 •
edited
Loading

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

jeysal
jeysal reviewed Aug 15, 2019
View reviewed changes
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 😢

@SimenB SimenB merged commit d69f8d3 into jestjs:master Aug 18, 2019
This was referenced Jan 28, 2020
Open
Open
This was referenced Feb 4, 2020
Open
Open
Open
Open
@Leko Leko mentioned this pull request Feb 18, 2020
Open
This was referenced Feb 18, 2020
Open
Open
@Leko Leko mentioned this pull request Feb 25, 2020
Open
This was referenced Mar 3, 2020
Open
Open
This was referenced Mar 10, 2020
Open
Open
Open
Open
@snyk-bot snyk-bot mentioned this pull request Mar 21, 2020
Closed
This was referenced Mar 24, 2020
Open
Open
This was referenced Apr 16, 2020
Open
Open
Merged
Closed
Merged
This was referenced Apr 30, 2020
Closed
Closed
@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.
Reviewers

@jeysal jeysal jeysal left review comments

@lh0x00 lh0x00 lh0x00 requested changes

@SimenB SimenB SimenB approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eranshabi @SimenB @jeysal @lh0x00 @facebook-github-bot