-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Exposing jest.runToFrame() from sinon/fake_timers #14598
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
docs/JestObjectAPI.md
Outdated
@@ -989,6 +989,16 @@ This function is not available when using legacy fake timers implementation. | |||
|
|||
::: | |||
|
|||
### `jest.runToFrame()` |
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.
Should I add these changes to 29.7
docs or just the base docs like I have now?
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.
Just base 👍
Update: never mind, I found where this check should happen (jest runtime). I have updated the PR @SimenB you mentioned that
I can add that, but I did not see any other cases of that so I have not for now. I can add that if you still want it |
It looks like the build failures are caused by unrelated issues |
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.
Perfect, thanks!
In addition to my API question, could you add a changelog entry? 🙂
docs/JestObjectAPI.md
Outdated
@@ -989,6 +989,16 @@ This function is not available when using legacy fake timers implementation. | |||
|
|||
::: | |||
|
|||
### `jest.runToFrame()` |
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.
Should we call it runToNextFrame
? runToFrame
sounds to me like it'd take which frame (or a number of frames) as an argument
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 mirrored the sinon api without too much thought 😊
runToNextFrame()
feels nice and fits with runOnlyPendingTimers()
.
Another idea: advanceTimersToNextFrame()
- fits in with `advanceTimersToNextTimer()
Which do you lean towards? (Or something else?)
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.
yeah, advanceTimersToNextFrame
sounds perfect to align with existing functions 👍
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.
Cc @SimenB
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.
hmm? I believe I answered the question? 😅
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.
Sorry, for some reason I didn't see your reply
Yeah, the windows tests has a bit of flake, unfortunately |
@@ -349,6 +349,14 @@ export interface Jest { | |||
* It is recommended to use `jest.mock()` instead. The `jest.mock()` API's second | |||
* argument is a module factory instead of the expected exported module object. | |||
*/ | |||
/** |
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.
The above TSDoc belongs to 'setMock'. Would be good to move it down (;
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.
Thanks! Fixed
Hi @SimenB, I think I have addressed all the open feedback. Please take a look and let me know what you think |
|
||
In applications, often you want to schedule work inside of an animation frame (with `requestAnimationFrame`). We expose a convenience method `jest.advanceTimersToNextFrame()` to advance all timers enough milliseconds to execute all actively scheduled animation frames. | ||
|
||
For mock timing purposes, animation frames are executed every `16ms` (mapping to roughly `60` frames per second) after the clock starts. When you schedule a callback in an animation frame (with `requestAnimationFrame(callback)`), the `callback` will be called when the clock has advanced `16ms`. `jest.advanceTimersToNextFrame()` will advance the clock just enough to get to the next `16ms` increment. If the clock has already advanced `6ms` since a animation frame `callback` was scheduled, then the clock will be advanced by `10ms`. |
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.
A added some more detail here to hopefully make things clearer for consumers
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.
perfect, thanks!
Here is a workaround for anybody who needs it before // ensure you are using "modern" fake timers
// 1. before doing anything, grab the start time `setStartSystemTime()`
// 2. step through frames by using `advanceTimersToNextFrame()`
let startTime: number | null = null;
/** Record the initial (mocked) system start time
*
* This is no longer needed once `jest.advanceTimersToNextFrame()` is available
* https://github.com/jestjs/jest/pull/14598
*/
export function setStartSystemTime() {
startTime = Date.now();
}
/** Step forward a single animation frame
*
* This is no longer needed once `jest.advanceTimersToNextFrame()` is available
* https://github.com/jestjs/jest/pull/14598
*/
export function advanceTimersToNextFrame() {
if(startTime == null) {
throw new Error('Must call `setStartSystemTime` before using `advanceTimersToNextFrame()`');
}
// Stealing logic from sinon fake timers
// https://github.com/sinonjs/fake-timers/blob/fc312b9ce96a4ea2c7e60bb0cccd2c604b75cdbd/src/fake-timers-src.js#L1102-L1105
const timePassedInFrame = (Date.now() - startTime) % 16;
const timeToNextFrame = 16 - timePassedInFrame;
jest.advanceTimersByTime(timeToNextFrame);
} |
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. |
Closes #14593