-
Notifications
You must be signed in to change notification settings - Fork 108
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
Conversation
@@ -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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 () { |
There was a problem hiding this comment.
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 Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
if good, merge |
@fatso83 not sure I dare make a release, tho! 😅 What's the steps? Is the changelog automatic? |
I'm getting |
Purpose (TL;DR) - mandatory
timer.refresh
looks completely broken to me (and is definitely at least somewhat broken, see jestjs/jest#12527).setTimeout
andclearTimeout
, instead of the ones defined in theglobal
passed intimer.refresh
in node -Symbol
sasyncId
andtriggerId
never changes, only_idleStart
. alsot.refresh() === t
)