[scheduler] 1/n Improve tests for 'schedule' module #12880
Merged
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
**what is the change?:** Renamed some methods, and made a method to advance a frame in the test environment. **why make this change?:** We often need to simulate a frame passing with some amount of idle time or lack of idle time, and the new method makes it easier to write that out. **test plan:** Run the updated tests. Also temporarily tried breaking the scheduler and verified that the tests will fail. **issue:** See internal task T29442940
gaearon
reviewed
May 22, 2018
| @@ -10,15 +10,25 @@ | |||
| 'use strict'; | |||
|
|
|||
| let ReactScheduler; | |||
| type FrameTimeoutConfigType = { | |||
There was a problem hiding this comment.
AFAIK we don't run Flow on tests.
There was a problem hiding this comment.
:( I noticed that. We could either consider it as inline documentation for now, or I can replace it with comments in order to avoid a false sense of security.
flarnie
changed the title
flarnie
added a commit
to flarnie/react
that referenced
this pull request
May 23, 2018
NOTE: This PR depends on facebook#12880 and facebook#12884 Please review those first, and after they land Flarnie will rebase on top of them. --- **what is the change?:** See title **why make this change?:** This seems to make the code simpler, and potentially saves space of having an array and object around holding references to the callbacks. **test plan:** Run existing tests
This was referenced May 23, 2018
acdlite
approved these changes
May 24, 2018
|
|
flarnie
added a commit
to flarnie/react
that referenced
this pull request
May 24, 2018
NOTE: This PR depends on facebook#12880 and facebook#12884 Please review those first, and after they land Flarnie will rebase on top of them. --- **what is the change?:** See title **why make this change?:** This seems to make the code simpler, and potentially saves space of having an array and object around holding references to the callbacks. **test plan:** Run existing tests
flarnie
added a commit
to flarnie/react
that referenced
this pull request
May 24, 2018
NOTE: This PR depends on facebook#12880 and facebook#12884 Please review those first, and after they land Flarnie will rebase on top of them. --- **what is the change?:** See title **why make this change?:** This seems to make the code simpler, and potentially saves space of having an array and object around holding references to the callbacks. **test plan:** Run existing tests
flarnie
added a commit
to flarnie/react
that referenced
this pull request
May 26, 2018
NOTE: This PR depends on facebook#12880 and facebook#12884 Please review those first, and after they land Flarnie will rebase on top of them. --- **what is the change?:** See title **why make this change?:** This seems to make the code simpler, and potentially saves space of having an array and object around holding references to the callbacks. **test plan:** Run existing tests
flarnie
added a commit
that referenced
this pull request
May 26, 2018
…ck storage (#12893) * [schedule] Use linked list instead of queue and map for storing cbs NOTE: This PR depends on #12880 and #12884 Please review those first, and after they land Flarnie will rebase on top of them. --- **what is the change?:** See title **why make this change?:** This seems to make the code simpler, and potentially saves space of having an array and object around holding references to the callbacks. **test plan:** Run existing tests * minor style improvements * refactor conditionals in cancelScheduledWork for increased clarity * Remove 'canUseDOM' condition and fix some flow issues w/callbackID type **what is the change?:** - Removed conditional which fell back to 'setTimeout' when the environment doesn't have DOM. This appears to be an old polyfill used for test environments and we don't use it any more. - Fixed type definitions around the callbackID to be more accurate in the scheduler itself, and more loose in the React code. **why make this change?:** To get Flow passing, simplify the scheduler code, make things accurate. **test plan:** Run tests and flow. * Rewrite 'cancelScheduledWork' so that Flow accepts it **what is the change?:** Adding verification that 'previousCallbackConfig' and 'nextCallbackConfig' are not null before accessing properties on them. Slightly concerned because this implementation relies on these properties being untouched and correct on the config which is passed to 'cancelScheduledWork' but I guess we already rely heavily on that for this whole approach. :\ **why make this change?:** To get Flow passing. Not sure why it passed earlier and in CI, but now it's not. **test plan:** `yarn flow dom` and other flow tests, lint, tests, etc. * ran prettier * Put back the fallback implementation of scheduler for node environment **what is the change?:** We had tried removing the fallback implementation of `scheduler` but tests reminded us that this is important for supporting isomorphic uses of React. Long term we will move this out of the `schedule` module but for now let's keep things simple. **why make this change?:** Keep things working! **test plan:** Ran tests and flow * Shorten properties stored in objects by sheduler **what is the change?:** `previousScheduledCallback` -> `prev` `nextScheduledCallback` -> `next` **why make this change?:** We want this package to be smaller, and less letters means less code means smaller! **test plan:** ran existing tests * further remove extra lines in scheduler
NMinhNguyen
pushed a commit
to enzymejs/react-shallow-renderer
that referenced
this pull request
Jan 29, 2020
…ck storage (#12893) * [schedule] Use linked list instead of queue and map for storing cbs NOTE: This PR depends on facebook/react#12880 and facebook/react#12884 Please review those first, and after they land Flarnie will rebase on top of them. --- **what is the change?:** See title **why make this change?:** This seems to make the code simpler, and potentially saves space of having an array and object around holding references to the callbacks. **test plan:** Run existing tests * minor style improvements * refactor conditionals in cancelScheduledWork for increased clarity * Remove 'canUseDOM' condition and fix some flow issues w/callbackID type **what is the change?:** - Removed conditional which fell back to 'setTimeout' when the environment doesn't have DOM. This appears to be an old polyfill used for test environments and we don't use it any more. - Fixed type definitions around the callbackID to be more accurate in the scheduler itself, and more loose in the React code. **why make this change?:** To get Flow passing, simplify the scheduler code, make things accurate. **test plan:** Run tests and flow. * Rewrite 'cancelScheduledWork' so that Flow accepts it **what is the change?:** Adding verification that 'previousCallbackConfig' and 'nextCallbackConfig' are not null before accessing properties on them. Slightly concerned because this implementation relies on these properties being untouched and correct on the config which is passed to 'cancelScheduledWork' but I guess we already rely heavily on that for this whole approach. :\ **why make this change?:** To get Flow passing. Not sure why it passed earlier and in CI, but now it's not. **test plan:** `yarn flow dom` and other flow tests, lint, tests, etc. * ran prettier * Put back the fallback implementation of scheduler for node environment **what is the change?:** We had tried removing the fallback implementation of `scheduler` but tests reminded us that this is important for supporting isomorphic uses of React. Long term we will move this out of the `schedule` module but for now let's keep things simple. **why make this change?:** Keep things working! **test plan:** Ran tests and flow * Shorten properties stored in objects by sheduler **what is the change?:** `previousScheduledCallback` -> `prev` `nextScheduledCallback` -> `next` **why make this change?:** We want this package to be smaller, and less letters means less code means smaller! **test plan:** ran existing tests * further remove extra lines in scheduler
renawolford6
added a commit
to renawolford6/react-shallow-renderer
that referenced
this pull request
Nov 10, 2022
…ck storage (#12893) * [schedule] Use linked list instead of queue and map for storing cbs NOTE: This PR depends on facebook/react#12880 and facebook/react#12884 Please review those first, and after they land Flarnie will rebase on top of them. --- **what is the change?:** See title **why make this change?:** This seems to make the code simpler, and potentially saves space of having an array and object around holding references to the callbacks. **test plan:** Run existing tests * minor style improvements * refactor conditionals in cancelScheduledWork for increased clarity * Remove 'canUseDOM' condition and fix some flow issues w/callbackID type **what is the change?:** - Removed conditional which fell back to 'setTimeout' when the environment doesn't have DOM. This appears to be an old polyfill used for test environments and we don't use it any more. - Fixed type definitions around the callbackID to be more accurate in the scheduler itself, and more loose in the React code. **why make this change?:** To get Flow passing, simplify the scheduler code, make things accurate. **test plan:** Run tests and flow. * Rewrite 'cancelScheduledWork' so that Flow accepts it **what is the change?:** Adding verification that 'previousCallbackConfig' and 'nextCallbackConfig' are not null before accessing properties on them. Slightly concerned because this implementation relies on these properties being untouched and correct on the config which is passed to 'cancelScheduledWork' but I guess we already rely heavily on that for this whole approach. :\ **why make this change?:** To get Flow passing. Not sure why it passed earlier and in CI, but now it's not. **test plan:** `yarn flow dom` and other flow tests, lint, tests, etc. * ran prettier * Put back the fallback implementation of scheduler for node environment **what is the change?:** We had tried removing the fallback implementation of `scheduler` but tests reminded us that this is important for supporting isomorphic uses of React. Long term we will move this out of the `schedule` module but for now let's keep things simple. **why make this change?:** Keep things working! **test plan:** Ran tests and flow * Shorten properties stored in objects by sheduler **what is the change?:** `previousScheduledCallback` -> `prev` `nextScheduledCallback` -> `next` **why make this change?:** We want this package to be smaller, and less letters means less code means smaller! **test plan:** ran existing tests * further remove extra lines in scheduler
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.
Another bite-sized PR with some test improvements. 🍩
what is the change?:
Renamed some methods, and made a method to advance a frame in the test
environment.
why make this change?:
We often need to simulate a frame passing with some amount of idle time
or lack of idle time, and the new method makes it easier to write that
out.
test plan:
Run the updated tests.
Also temporarily tried breaking the scheduler and verified that the
tests will fail.
issue:
See internal task T29442940