[scheduler] 2/n Adding 'schedule' fixture #12884

flarnie · 2018-05-22T20:08:05Z

what is the change?:
We need to test the schedule module against real live browser APIs. As
a quick solution we're writing a fixture for using in manual testing.
Later we plan on adding automated browser testing, using this or a
similar fixture as the test page.

why make this change?:
To further solidify test coverage for schedule before making further
improvements/refactors to the module.

test plan:
open fixtures/schedule/index.html and inspect the results. It should
be clear that things pass.

We also temporarily broke the scheduler and verified that this fixture
demonstrates the problems.

Example of the edit: latest fixture page:

And an example of making it fail intentionally:

issue:
Internal task T29442940

flarnie · 2018-05-22T20:21:51Z

As a nice-to-have, thinking of adding a green/red dashed/solid border around tests depending on whether the output is correct or not.

**what is the change?:** We need to test the `schedule` module against real live browser APIs. As a quick solution we're writing a fixture for using in manual testing. Later we plan on adding automated browser testing, using this or a similar fixture as the test page. **why make this change?:** To further solidify test coverage for `schedule` before making further improvements/refactors to the module. **test plan:** `open fixtures/schedule/index.html` and inspect the results. It should be clear that things pass. We also temporarily broke the scheduler and verified that this fixture demonstrates the problems. **issue:** Internal task T29442940

**what is the change?:** Added red/green solid/dashed border for test results when using the schedule fixture. We also tweaked the timing of the last test because it was on the line in terms of whether it passed or failed. **why make this change?:** To make it faster to use the fixture - it takes more time to read through the results line by line and check that they match what is expected. **test plan:** Looked at the fixture, and also tried modifying a test to show what it looks like when something fails.

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

acdlite

Neat!

flarnie · 2018-05-24T21:11:19Z

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

…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

…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

flarnie requested review from sebmarkbage, gaearon and acdlite May 22, 2018 20:08

facebook-github-bot added the CLA Signed label May 22, 2018

flarnie force-pushed the addFixtureToTestScheduleModule branch from ba54da6 to 938f363 Compare May 23, 2018 17:12

flarnie changed the title ~~Adding 'schedule' fixture~~ [scheduler] 2/n Adding 'schedule' fixture May 23, 2018

This was referenced May 23, 2018

[scheduler] 3/n Use a linked list instead of map and queue for callback storage #12893

Merged

[scheduler] 4/n Allow splitting out schedule in fb-www, prepare to fix polyfill issue internally #12900

Merged

acdlite approved these changes May 24, 2018

View reviewed changes

flarnie merged commit 76e0707 into facebook:master May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[scheduler] 2/n Adding 'schedule' fixture #12884

[scheduler] 2/n Adding 'schedule' fixture #12884

flarnie commented May 22, 2018 •

edited

Loading

flarnie commented May 22, 2018

acdlite left a comment

flarnie commented May 24, 2018

[scheduler] 2/n Adding 'schedule' fixture #12884

[scheduler] 2/n Adding 'schedule' fixture #12884

Conversation

flarnie commented May 22, 2018 • edited Loading

flarnie commented May 22, 2018

acdlite left a comment

Choose a reason for hiding this comment

flarnie commented May 24, 2018

flarnie commented May 22, 2018 •

edited

Loading