search

sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
794 stars 103 forks source link

fix(timer.refresh): should just change `callAt` #425

Closed SimenB closed 2 years ago

SimenB commented 2 years ago

Purpose (TL;DR) - mandatory

timer.refresh looks completely broken to me (and is definitely at least somewhat broken, see https://github.com/facebook/jest/issues/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)
codecov[bot] commented 2 years ago

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%) :arrow_up:

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%) :arrow_up:

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.

fatso83 commented 2 years ago

if good, merge

SimenB commented 2 years ago

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

benjamingr commented 2 years ago

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

SimenB commented 2 years ago

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 commented 2 years ago

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