Investigate unit tests for AddToHomescreenDataFetcher |
||
Issue descriptionAt the moment the tests for testing timeout callbacks are slow and disabled. Moreover, the existing tests would quit after either the timeout callback or the success callback returns, failing to capture the scenarios when both are invoked.
,
Mar 15 2017
Known issues for existing tests: 1. Time consuming. Several tests are disabled due to taking too long (waiting for timeout to happen). 2. Quit too early. AddToHomescreenDataFetcher is implemented in a way that both timeout callback and success callback would always be called. However, the tests quit as soon as one of the callbacks is finished. As a result, it failed to detect the crash in crbug.com/692626 . 3. Incorrect test criteria. Instead of checking whether an Observer callback has been called, we want to count how many times an Observer callback has been called. Callbacks such as OnUserTitleAvailable() should have been called exactly once. Peter's idea for mocking InstallableManager: https://codereview.chromium.org/2741283002/ mocks InstallableManager and triggers time out event manually with a TestMockTimeTaskRunner. InstallableManager has its own unit tests and browser tests, so it's safe to mock out InstallableManager and only tests AddToHomescreenDataFetcher here. If we extend the idea to make sure that all callbacks are called {Add content::BrowserThread::GetBlockingPool()->FlushForTesting(); after test_installable_manager()->callback_.Run(builder.Build());}, and to count how many times each Observer callbacks are called instead of checking whether callbacks have been run, it would cover the test cases that we want to test. However, this is flaky. With --gtest-repeat, this idea does not guarantee 100% success rate on Nexus 7 with Android M or Nexus 6P with Android N.
,
Jun 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/685bb7c7671afce49cda469ce95d303413cf3694 commit 685bb7c7671afce49cda469ce95d303413cf3694 Author: pkotwicz <pkotwicz@chromium.org> Date: Fri Jun 16 01:58:01 2017 [Android WebAPK] Do not add padding to WebAPK primary icon For non-intranet sites the WebAPK server ignores the icon sent as part of the proto (but not the hash). This CL stops add_to_homescreen_data_fetcher.cc from adding padding to the primary icon. This makes the icon in the add-to-homescreen banner look the same as the one added to app list / homescreen by Google Play. BUG=697228 Review-Url: https://codereview.chromium.org/2939773004 Cr-Commit-Position: refs/heads/master@{#479912} [modify] https://crrev.com/685bb7c7671afce49cda469ce95d303413cf3694/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
,
Jun 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5cc00e8e97d0dabc14e7dd4284d0244033caffd3 commit 5cc00e8e97d0dabc14e7dd4284d0244033caffd3 Author: dominickn <dominickn@chromium.org> Date: Fri Jun 30 04:27:56 2017 Don't ignore manifest icons for sites that don't have a service worker. https://crrev.com/2942513002 changed the InstallableManager such that it will wait indefinitely for a site to register a service worker. For add to homescreen, this means that sites which have icons in a manifest, but no service worker, InstallableManager::GetData will time out and the icons in the manifest will be ignored. This CL updates AddToHomeScreenDataFetcher to run InstallableManager in two phases, like AppBannerManager does. The first phase fetches the manifest and the most appropriate icons from it. The second phase checks installability, including the presence of a service worker. Note that sites which have icons in a manifest and otherwise meet the PWA eligibility criteria *except* for not having a service worker will now take up to 4 seconds (the timeout period) before the details are populated. BUG=721881,697228 Review-Url: https://codereview.chromium.org/2949993002 Cr-Commit-Position: refs/heads/master@{#483626} [modify] https://crrev.com/5cc00e8e97d0dabc14e7dd4284d0244033caffd3/chrome/browser/android/webapk/webapk_update_data_fetcher.cc [modify] https://crrev.com/5cc00e8e97d0dabc14e7dd4284d0244033caffd3/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc [modify] https://crrev.com/5cc00e8e97d0dabc14e7dd4284d0244033caffd3/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h [modify] https://crrev.com/5cc00e8e97d0dabc14e7dd4284d0244033caffd3/chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc [modify] https://crrev.com/5cc00e8e97d0dabc14e7dd4284d0244033caffd3/chrome/browser/android/webapps/add_to_homescreen_manager.cc [modify] https://crrev.com/5cc00e8e97d0dabc14e7dd4284d0244033caffd3/chrome/browser/banners/app_banner_manager.cc [modify] https://crrev.com/5cc00e8e97d0dabc14e7dd4284d0244033caffd3/chrome/browser/ui/tab_helpers.cc |
||
►
Sign in to add a comment |
||
Comment 1 by zpeng@chromium.org
, Feb 28 2017