Test Setup: Add more-complete mocks of common timer functions #35368
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The existing mock of `requestAnimationFrame` omits a parameter when calling the given callback. This patch adds that missing parameter. Other code in the project also depends on `requestIdleCallback` but this wasn't mocked at all, leading to test failures in a project that integrates Gutenberg. This patch adds a reasonable mock of that function as well. These mocks have been rearranged as well so that their names match the expected names by using the more verbose `= function request...` syntax instead of directly setting their values to the `setTimeout` functions.
dmsnell
requested review from
ajitbohra,
gziolo,
nerrad and
ntwb
as code owners
tw875768
approved these changes
Oct 5, 2021
|
Size Change: +161 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
gziolo
added
[Tool] Jest preset
gziolo
approved these changes
Oct 6, 2021
There was a problem hiding this comment.
Thank you for your contribution. That's definitely a better mock implementation 👍🏻
We could add a note in the CHANGELOG for the package:
https://github.com/WordPress/gutenberg/blob/trunk/packages/jest-preset-default/CHANGELOG.md
| () => | ||
| callback( { | ||
| didTimeout: false, | ||
| timeRemaining: () => Math.max( 0, 50 - ( Date.now() - start ) ), |
There was a problem hiding this comment.
Just confirming I follow the code, it returns a mocked value in the range 0-50.
There was a problem hiding this comment.
now that I reflect on this I'm not sure why that's there. I might have copied it from somewhere.
we should return some value if timeRemaining is called. for the sake of the tests I don't think it will matter because the important part is that we return a non-negative number of ms remaining in the current idle period. for test purposes, this should be small if indeed anything were to rely on it, as you would not expect to get long windows of idle time in the browser
There was a problem hiding this comment.
Right, I don't think it's something that you would want to use in the test for verification.
| callback | ||
| ) { | ||
| // eslint-disable-next-line no-restricted-syntax | ||
| const randomDelay = Math.round( ( Math.random() * 1_000 ) / 60 ); |
There was a problem hiding this comment.
Unrelated Aside: I'm wondering if the time period for animation frame becomes 1000 / 120 when the display has 120Hz like Apple devices with Pro Motion 😄
There was a problem hiding this comment.
this also shouldn't matter that much for the tests. by specification yes, we could get 120Hz > 8ms inter-frame times, but for the sake of mocking this function it will appear as if our fake display is 60Hz. code should not rely on the timing of the call but rather the DOMHighResTimeStamp passed into the callback if they want to perform time-based calculations.
There was a problem hiding this comment.
Yes, it's irrelevant here what the number is here. I mostly found it intriguing that you often can read online that the animation frame is 16ms.
|
Thanks @gziolo. I added a note in the CHANGELOG but I'm not sure if I did it right. I added the note under |
gziolo
approved these changes
Oct 7, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
Created PR to watch tests pass, something locally was failing regardingDockerDescription
The existing mock of
requestAnimationFrameomits a parameter whencalling the given callback. This patch adds that missing parameter.
Other code in the project also depends on
requestIdleCallbackbutthis wasn't mocked at all, leading to test failures in a project
that integrates Gutenberg. This patch adds a reasonable mock of that
function as well.
These mocks have been rearranged as well so that their names match
the expected names by using the more verbose
= function request...syntax instead of directly setting their values to the
setTimeoutfunctions.
How has this been tested?
Please audit the code in here. This change should only impact the test
code and the
setup-globals.jstest configuration in thejest-preset-defaultpackage. To fully test import the package in a project that runs tests on
code that imports
@wordpress/priority-queueIn
trunkthe tests should fail withTypeError: Cannot read property 'timeRemaining' of undefinedIn this branch the tests should pass.
Types of changes
There should be no functional or visual changes in this fix.
Only test environments should be affected and with that
only currently-improperly-failing tests should start properly
passing.
Checklist:
*.native.jsfiles for terms that need renaming or removal).