New issue
Advanced search Search tips

Issue 697228 link

Starred by 2 users

Issue metadata

Status: Untriaged
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: ----



Sign in to add a comment

Investigate unit tests for AddToHomescreenDataFetcher

Project Member Reported by zpeng@chromium.org, Feb 28 2017

Issue description

At 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.

 

Comment 1 by zpeng@chromium.org, Feb 28 2017

Owner: zpeng@chromium.org

Comment 2 by zpeng@chromium.org, Mar 15 2017

Owner: ----
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.

Project Member

Comment 3 by bugdroid1@chromium.org, 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

Project Member

Comment 4 by bugdroid1@chromium.org, 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