Adding more schedule tests #12858
Merged
Adding more schedule tests #12858
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
flarnie
force-pushed
the
addingMoreScheduleTests
branch
from
3b2c738
to
c5fbdc4
Compare
flarnie
changed the title
|
Edit: I rebased and now this only has the changes which need reviewed. If you look at the second commit, that's my changes. I want a better API for simulating the change in time during a frame, will work on that in a follow-up diff. |
**what is the change?:** Test coverage checking that callbacks are called when they time out. This test surfaced a bug and this commit includes the fix. I want to refine this approach, but basically we can simulate time outs by controlling the return value of 'now()' and the argument passed to the rAF callback. Next we will write a browser fixture to further test this against real browser APIs. **why make this change?:** Better tests will keep this module working smoothly while we continue refactoring and improving it. **test plan:** Run the new tests, see that it fails without the bug fix.
flarnie
force-pushed
the
addingMoreScheduleTests
branch
from
c5fbdc4
to
fc5a0ea
Compare
flarnie
commented
May 18, 2018
| runRAFCallbacks(); // runs rAF callback | ||
| // push time ahead a bit so that we have no idle time | ||
| startOfLatestFrame += 16; | ||
| drainPostMessageQueue(); // runs postMessage callback, idleTick |
There was a problem hiding this comment.
I plan to improve the API for writing these tests; instead of drainPostMessageQueue and runRAFCallbacks it should be more generic and descriptive, like so:
const shorterTimeout = 2;
const longerTimeout = 100;
scheduleWork(callbackA);
scheduleWork(callbackB, {timeout: longerTimeout});
scheduleWork(callbackC, {timeout: shorterTimeout});
// generic API for advancing a frame and configuring how much idle time there will be
advanceByOneFrame({timeRemaining: -1 * (longerTimeout * 2)});
// idle time was negative, so there was none and we passed the timeout threshhold for the shorterTimeout
expect(callbackLog).toEqual(['C']);
// ... etc.
|
|
|
Going to continue working on top of this soon, hopefully any issues can be done in a follow-up PR since this is just adding tests. 😇 |
acdlite
approved these changes
May 22, 2018
|
|
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.
Tests and fixes for 'timing out' behavior
what is the change?:
Test coverage checking that callbacks are called when they time out.
This test surfaced a bug and this commit includes the fix.
I want to refine this approach, but basically we can simulate time outs
by controlling the return value of 'now()' and the argument passed to
the rAF callback.
Next we will write a browser fixture to further test this against real
browser APIs.
why make this change?:
Better tests will keep this module working smoothly while we continue
refactoring and improving it.
test plan:
Run the new tests, see that it fails without the bug fix.