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(timer.refresh): should just change callAt #425

Merged
merged 6 commits into from Mar 3, 2022

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Mar 2, 2022

Purpose (TL;DR) - mandatory

timer.refresh looks completely broken to me (and is definitely at least somewhat broken, see jestjs/jest#12527).

  1. It uses global setTimeout and clearTimeout, instead of the ones defined in the global passed in
  2. it (tries to) remove the old timer and schedule a new one, "changing" its ID. This:
    1. Does not remove the existing one, as that's not an actual real timer
    2. Break identity since we "schedule" (actually, for real, schedules) a new timer instead of refreshing the existing one (try timer.refresh in node - Symbols asyncId and triggerId never changes, only _idleStart. also t.refresh() === t)

@@ -3575,6 +3575,7 @@
"resolved": "https://registry.npmjs.org/fsevents/-/fsevents-1.2.13.tgz",
"integrity": "sha512-oWb1Z6mkHIskLzEJ/XWX0srkpkTQ7vaopMQkyaEIoq0fmtFVxOthb8cCxeT+p3ynTdkk/RZwbgG4brR5BeWECw==",
"dev": true,
"hasInstallScript": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

I just ran npm install

Copy link
Member

Choose a reason for hiding this comment

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

You're probably just on a newer npm version

Copy link
Member Author

Choose a reason for hiding this comment

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

using what ships with current LTS, seems fine 😀 can revert if you want of course

@@ -4862,17 +4862,6 @@ describe("#187 - Support timeout.refresh in node environments", function () {
}
clock.uninstall();
});

it("assigns a new id to the refreshed timer", function () {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is plain wrong, as mentioned in the OP

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #425 (147981e) into main (5ea3a4d) will increase coverage by 1.31%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   94.17%   95.49%   +1.31%     
==========================================
  Files           1        1              
  Lines         618      621       +3     
==========================================
+ Hits          582      593      +11     
+ Misses         36       28       -8     
Flag Coverage Δ
unit 95.49% <100.00%> (+1.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/fake-timers-src.js 95.49% <100.00%> (+1.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc56a4d...147981e. Read the comment docs.

src/fake-timers-src.js Outdated Show resolved Hide resolved
@fatso83
Copy link
Contributor

fatso83 commented Mar 3, 2022

if good, merge

@SimenB SimenB merged commit 75950c0 into sinonjs:main Mar 3, 2022
@SimenB SimenB deleted the refresh-fix branch March 3, 2022 12:59
@SimenB
Copy link
Member Author

SimenB commented Mar 3, 2022

@fatso83 not sure I dare make a release, tho! 😅 What's the steps? Is the changelog automatic?

@benjamingr
Copy link
Member

@SimenB https://github.com/sinonjs/fake-timers/blob/main/RELEASE.md

@SimenB
Copy link
Member Author

SimenB commented Mar 5, 2022

I'm getting Error: Could not find browser revision 818858. Run "PUPPETEER_PRODUCT=firefox npm install" or "PUPPETEER_PRODUCT=firefox yarn install" to download a supported Firefox browser binary. 🙁

@SimenB
Copy link
Member Author

SimenB commented Mar 5, 2022

Ah, needed to use an older version of npm (which then didn't support the lockfile version...). Then the postversion failed, probably because my fork is still named lolex. However, all the browser tests ran successfully, so hopefully I didn't mess anything up! 😀

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants