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

fix: getTimerCount not taking immediates and ticks into account #8139

Merged
merged 6 commits into from
Mar 18, 2019

Conversation

xixixao
Copy link
Contributor

@xixixao xixixao commented Mar 17, 2019

Summary

I assume this is how the getTimerCount method was supposed to work. Otherwise it's pretty useless.

I included moving to Map, so that the function doesn't incur an O(n) size check, because I'm running this in a pretty tight loop.

Test plan

Added unit test.

@@ -374,8 +374,8 @@ export default class FakeTimers<TimerRef> {
private _fakeClearTimer(timerRef: TimerRef) {
const uuid = this._timerConfig.refToId(timerRef);

if (uuid && this._timers.hasOwnProperty(uuid)) {
delete this._timers[String(uuid)];
if (uuid && this._timers.has(uuid)) {
Copy link
Member

Choose a reason for hiding this comment

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

do we need the has check? I'd guess delete just no-ops if the key is not there

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.

I agree this seems more correct.

Mind fixing CI and updating the changelog?

@thymikee thymikee changed the title Fix get timers fix: getTimerCount not taking immediates and ticks into account Mar 18, 2019
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

I've pushed some changes to fix the tests and the new map implementation

@thymikee thymikee requested a review from SimenB March 18, 2019 13:54
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.

Still missing changelog, code LGTM

@@ -65,7 +65,7 @@ export default class FakeTimers<TimerRef> {
private _now!: number;
private _ticks!: Array<Tick>;
private _timerAPIs: TimerAPI;
private _timers!: {[key: string]: Timer};
private _timers!: Map<string, Timer>;
Copy link
Member

Choose a reason for hiding this comment

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

Technically breaking, but meh. It's internal

@SimenB SimenB merged commit 8675290 into jestjs:master Mar 18, 2019
@xixixao
Copy link
Contributor Author

xixixao commented Mar 18, 2019

Thanks you guys rock!

thymikee added a commit to Brantron/jest that referenced this pull request Mar 19, 2019
* upstream/master: (391 commits)
  more precise circus asyncError types (jestjs#8150)
  Add typeahead watch plugin (jestjs#6449)
  fix: getTimerCount not taking immediates and ticks into account (jestjs#8139)
  website: add an additional filter predicate to backers (jestjs#7286)
  [🔥] Revised README (jestjs#8076)
  [jest-each] Fix test function type (jestjs#8145)
  chore: improve bug template labels for easier maintenance (jestjs#8141)
  Add documentation related to auto-mocking (jestjs#8099)
  Add support for bigint to pretty-format (jestjs#8138)
  Revert "Add fuzzing based tests in Jest (jestjs#8012)"
  chore: remove console.log
  chore: Improve description of optional arguments in ExpectAPI.md (jestjs#8126)
  Add fuzzing based tests in Jest (jestjs#8012)
  Move @types/node to the root package.json (jestjs#8129)
  chore: use property initializer syntax (jestjs#8117)
  chore: delete flow types from the repo (jestjs#8061)
  Move changelog entry to the proper version (jestjs#8115)
  Release 24.5.0
  Expose throwOnModuleCollision (jestjs#8113)
  add matchers to expect type (jestjs#8093)
  ...
@SimenB
Copy link
Member

SimenB commented Mar 21, 2019

@xixixao do you think you could PR this to Lolex as well? In #5171 I'm trying to move to Lolex, and sinonjs/fake-timers#215 did not include ticks, so the new test fails 🙂

@SimenB
Copy link
Member

SimenB commented Apr 13, 2019

@xixixao ping (if you don't have time, that's of course perfectly fine)

@xixixao
Copy link
Contributor Author

xixixao commented Apr 13, 2019 via email

@SimenB
Copy link
Member

SimenB commented Apr 21, 2019

Thanks! I opened up sinonjs/fake-timers#238 so we don't forget 🙂

@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.

4 participants