Fix FCM rxjs Incompatibility #3221
Merged
Conversation
This file contains hidden or 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
Context: the tests were switched off a couple of years ago due to flakiness. It has not been maintained since then. During the time, Some Selenium API methods used were deprecated and removed; the Firebase projects used are no longer managed or owned by the FCM team. Consequently, the test became unfunctional. In the effort of providing safety for the upcoming FCM releases, this PR is created to fix, deflake, refactor and improve the old tests.
This PR did the following:
- Enabled comprehensive IT for chrome (ver.80). Now we are covering send&foreground recevie for FCM messages (messages with {notification} payload, {data} payload and {notification, data} payload), delete/update token, use default/customized ServiceWorker.
- Defalaked test. The IT is now reasonably stable without retry. Previously we are retrying 1 or 3 times.
- Optimized test. Previously we create a webDriver for each test, which is slow and annoying. Because a window is created and brought to focus and killed frequently, it makes working on other tasks and testing nearly impossible (Probably using a headless browser would work but I haven't found a satisfying solution to have the app in the state of foreground and background which is a requirement for FCM functions). With the way the tests are organized, the IT only spin up a new web driver when necessary. Some data on performance: (old) 'test-send' take 74 seconds (only measured 'test-send' because the other test suites were not functional at the time); (now) 'test-send', 'test-deleteToken', 'test-updateToken', 'test-useDefaultServiceWorker', 'test-useValidManifest' takes in total 33s (10 run average).
- General refactoring. Including refactors on expect blocks, createWebDriver, use const for constants usage, etc. The code should be much easier to understand and maintain.
Future work:
- Enable test on firefox once I get the notification permission working.
- Run the IC against some milestone chrome/firefox version (if it makes sense) to ensure backward compatibility. We should try to avoid #2712 . :)
Context: FCM IT were turned off a couple years ago due to flakiness. It became mostly unfunctional as repo structure change overtime. The goal is to fix and enable IT for more confident developement flow and safer releases. This CL does the following: - Fix the IT to be functional again. The IT is derteminated from my experiements (no longer flaky). Therefore, The CL removed the retry mechasim (previously retry 3 times) which makes running IT cheaper and more enjoyable. - Include IT for test:change for FCM package: the entire IT test suites is resoanblly fast (from my exeperience 1-3 miutes to complete. As it grows larger, maybe it makes run tests in parallel in Saucelab) Futhure work: - Enable testing for firefox
js doesn't allow underscore for int
why dot.env(FCM_SECRETS) are not included in env?
Because headless chrome doesn't have the concept of foreground and background
Binary Size ReportAffected SDKs
Test Logs |
Feiyang1
previously requested changes
Jun 16, 2020
…into fix-fcm-observer Sync to head to pick up changeset related chnages
🦋 Changeset is good to goLatest commit: 4aca0f2 We got this. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Feiyang1
reviewed
Jun 26, 2020
zwu52
requested review from
alikn,
andirayo,
avolkovi,
egilmorez,
erikeldridge,
sam-gc,
scottcrossen,
yuchenshi and
zijianjoy
as code owners
Feiyang1
approved these changes
Jul 6, 2020
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes #3142
Adding a field variable to hold rxjs observer or subscriber. It enables onMessage(nextFn) and onMessage(observer) to both execute if both are set.