Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2960103002:
Improve add to homescreen data fetcher unit tests. (Closed)
DescriptionImprove add to homescreen data fetcher unit tests.
Existing tests for this component don't work. The service worker
registration is for a different browser context, so the tests never
actuallly verify WebAPK-compatibility properly.
This CL revamps the tests by mocking out InstallableManager instead of
WebContents. This allows precise control over the data returned to the
data fetcher so we can verify more scenarios more accurately.
BUG=721881
Review-Url: https://codereview.chromium.org/2960103002
Cr-Commit-Position: refs/heads/master@{#485505}
Committed: https://chromium.googlesource.com/chromium/src/+/14dca19610d4b387af4f896d756f2cd9af5abf23
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #
Total comments: 28
Patch Set 3 : Comments #Patch Set 4 : Consolidation, testing calling GetData callback after time out #
Total comments: 45
Patch Set 5 : Comments #
Total comments: 10
Patch Set 6 : Comments #
Total comments: 11
Patch Set 7 : Comments #
Total comments: 7
Patch Set 8 : Comments #
Total comments: 4
Patch Set 9 : Comments #
Total comments: 6
Patch Set 10 : Rebase #Patch Set 11 : Address nits #Patch Set 12 : Re-rebase #Patch Set 13 : Self nits #
Messages
Total messages: 62 (42 generated)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dominickn@chromium.org changed reviewers: + pkotwicz@chromium.org
Hey Peter, WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2960103002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:158: params.check_installable ? is_installable_ : false}); For the sake of clarity, you should only return a primary icon and primary icon URL if InstallableParams::fetch_primary_icon is true https://codereview.chromium.org/2960103002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:322: Given that there is only a single caller to BuildNoIconManifest() I think that this would be clearer: content::Manifest manifest(BuildDefaultManifest()); manifest.icons.clear(); https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:154: } Can this block be simplified to: if (params.check_installable && !is_installable_) code = NO_MATCHING_SERVICE_WORKER; https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:329: A lot of the tests are very similar. Can we a function which runs the tests? Perhaps: void TestProcess( InstallableManager* installable_manager, const WebApplicationInfo& application_info, bool expected_webapk_compatible, const std::string& expected_title, const SkBitmap& expected_primary_icon) {} https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:334: EXPECT_TRUE(fetcher->primary_icon().drawsNothing()); This is incorrect. AddToHomescreenDataFetcher should have a non-empty generated icon. This is not the case because the test harness stubs out FinalizeLauncherIconInBackground() I think that we should change FinalizeLauncherIconInBackground() to return a solid color icon if the icon it is passed is empty and use gfx::test::AreImagesEqual() to test equality https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:335: EXPECT_TRUE(fetcher->shortcut_info().best_primary_icon_url.is_empty()); I don't think that it is worth checking the primary icon URL. Checking the primary icon URL makes the tests longer and does not provide significant gain. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:340: EXPECT_TRUE(fetcher->shortcut_info().best_badge_icon_url.is_empty()); I don't think that checking the value of AddToHomescreenDataFetcher::badge_icon() is useful I don't think that it is worth checking in every test that badge icons are only requested when AddToHomescreenDataFetcher::check_webapk_compatibility_ == true https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:345: TEST_P(AddToHomescreenDataFetcherTestCommon, NonInstallableManifest) { This test case has a very creative name given that it tests the case that the manifest is "installable" https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:399: // site. This test seems like it solely tests that the badge icon is fetched when AddToHomescreenDataFetcher::check_webapk_compatibility_ == true. Can you please change the "test description comment" to indicate this? You can probably also delete all of the non-badge-icon EXPECT() calls https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:405: SetWebApkInstallable(manifest); The SetWebApkInstallable() function name is misleading. A badge icon is not required for a Web Manifest to be considered WebAPK-able. Maybe inline the SetWebApkInstallable() implementation in the test case? https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:434: ManifestShortNameClobbersWebApplicationName) { This test case seems to be a duplicate of the creatively named AddToHomescreenDataFetcherTestCommon.NonInstallableManifest Can this test case be simplified to test only the differences between this test and AddToHomescreenDataFetcherTestCommon.NonInstallableManifest https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:475: GURL(kDefaultIconUrl)); Checking for the primary icon seems duplicative of the AddToHomescreenDataFetcherTestCommon.NonInstallableManifest test https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:478: } You probably need only one of the two sub test cases. I don't have a strong opinion as to which of the two to keep https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:563: { I think you can probably delete the first test case. My opinion is that the second test case passing guarantees that the first test case would also pass
High level comments: 1. these tests are cheap and fast, so they should check as much as possible. 2. these tests are trying to avoid making assumptions about the underlying implementation 3. the data fetcher logic is subtle enough, with enough variants in inputs (manifest with/without icons, installable, badge, name, short_name) that having things being explicitly spelled out, and each path tested independently is of significant value. If these tests had existed previously the the service worker observer changes would have been much easier. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:154: } On 2017/06/28 18:17:02, pkotwicz wrote: > Can this block be simplified to: > > if (params.check_installable && !is_installable_) > code = NO_MATCHING_SERVICE_WORKER; We need to call IsManifestValidForWebApp to ensure we pick up the MANIFEST_MISSING_SHORT_NAME_OR_NAME error code for the test exercising the null name and null short_name. I could manually hack that in but it seemed better just to defer to what InstallableManager does so that if it changes there this test will break. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:329: On 2017/06/28 18:17:02, pkotwicz wrote: > A lot of the tests are very similar. Can we a function which runs the tests? > > Perhaps: > > void TestProcess( > InstallableManager* installable_manager, > const WebApplicationInfo& application_info, > bool expected_webapk_compatible, > const std::string& expected_title, > const SkBitmap& expected_primary_icon) {} I deliberately separated everything out so that test failures would be more clear. I'm actually starting to lean against having too much in magical test runners because it's much harder to trace and debug why a test is failing. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:334: EXPECT_TRUE(fetcher->primary_icon().drawsNothing()); On 2017/06/28 18:17:02, pkotwicz wrote: > This is incorrect. AddToHomescreenDataFetcher should have a non-empty generated > icon. This is not the case because the test harness stubs out > FinalizeLauncherIconInBackground() No, this is correct for AddToHomescreenDataFetcher. The icon is generated in Java, which is not a part of this test. Instead, we simply echo back out the icon that we got: what we are testing is that we are not using any icons from the manifest and get an empty icon back. Testing icon generation needs to be done in the Java-side instrumentation test > I think that we should change FinalizeLauncherIconInBackground() to return a > solid color icon if the icon it is passed is empty and use > gfx::test::AreImagesEqual() to test equality That is idempotent to what is happening here, which is testing that we pass in nothing and we get nothing back. There's not much point pretending to generate an icon here when we can just check that we get nothing back. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:335: EXPECT_TRUE(fetcher->shortcut_info().best_primary_icon_url.is_empty()); On 2017/06/28 18:17:02, pkotwicz wrote: > I don't think that it is worth checking the primary icon URL. Checking the > primary icon URL makes the tests longer and does not provide significant gain. It's an easy check for correctness, so I don't see any reason to leave it out. Part of the problem with these tests before is that they didn't check enough stuff - we have very subtle logic about when the primary_icon and icon_urls are saved, so we should sanity check that it comes out correctly. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:340: EXPECT_TRUE(fetcher->shortcut_info().best_badge_icon_url.is_empty()); On 2017/06/28 18:17:02, pkotwicz wrote: > I don't think that checking the value of > AddToHomescreenDataFetcher::badge_icon() is useful > > I don't think that it is worth checking in every test that badge icons are only > requested when AddToHomescreenDataFetcher::check_webapk_compatibility_ == true See my previous comment. The more exhaustive the checks here, the better in my opinion due to how complicated the async work is. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:345: TEST_P(AddToHomescreenDataFetcherTestCommon, NonInstallableManifest) { On 2017/06/28 18:17:02, pkotwicz wrote: > This test case has a very creative name given that it tests the case that the > manifest is "installable" Done. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:399: // site. On 2017/06/28 18:17:02, pkotwicz wrote: > This test seems like it solely tests that the badge icon is fetched when > AddToHomescreenDataFetcher::check_webapk_compatibility_ == true. Can you please > change the "test description comment" to indicate this? > > You can probably also delete all of the non-badge-icon EXPECT() calls Fixed comment, but I don't really see much reason to remove the other calls. For instance, in the future we may make the WebAPK code path totally different to the ones exercised above. The point of these unit tests is that they should always pass no matter what we do with the underlying implementation. Removing EXPECT calls indicates an assumption about the underlying implementation (that other tests exercise that code path). https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:405: SetWebApkInstallable(manifest); On 2017/06/28 18:17:02, pkotwicz wrote: > The SetWebApkInstallable() function name is misleading. A badge icon is not > required for a Web Manifest to be considered WebAPK-able. Maybe inline the > SetWebApkInstallable() implementation in the test case? Renamed method to SetInstallableAndBadgeIcon https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:434: ManifestShortNameClobbersWebApplicationName) { On 2017/06/28 18:17:02, pkotwicz wrote: > This test case seems to be a duplicate of the creatively named > AddToHomescreenDataFetcherTestCommon.NonInstallableManifest > > Can this test case be simplified to test only the differences between this test > and AddToHomescreenDataFetcherTestCommon.NonInstallableManifest The difference is that manifest.name is explicitly set to null here, unlike the less creatively named test. Like I mentioned earlier, I don't feel like there's much point in removing the various EXPECTs here. The test is still very cheap to run, and is exercising a code path that has seen a bunch of contention lately. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:475: GURL(kDefaultIconUrl)); On 2017/06/28 18:17:02, pkotwicz wrote: > Checking for the primary icon seems duplicative of the > AddToHomescreenDataFetcherTestCommon.NonInstallableManifest test See earlier comment about not making assumptions about the implementation https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:478: } On 2017/06/28 18:17:03, pkotwicz wrote: > You probably need only one of the two sub test cases. I don't have a strong > opinion as to which of the two to keep Soon enough we'll flip check_webapk_installable to true always, but up until then I think it's fine to check both. https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:563: { On 2017/06/28 18:17:02, pkotwicz wrote: > I think you can probably delete the first test case. My opinion is that the > second test case passing guarantees that the first test case would also pass Again, that is making assumptions about the implementation. I wanted to be very detailed about testing all possible cases: - no manifest - manifest with just icons and name - fully installable - badge icon - missing name
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some random thoughts: - In an ideal world, a given test would have just one EXPECT() statement. (I realize that this is an extreme viewpoint) - I try to avoid multi part tests - I want to identify permutations which are redundant and thus don't have to be tested. I fear we will otherwise get an explosion of test cases which in turn make the code hard to modify https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:154: } The code in add_to_homescreen_data_fetcher.cc does not check what the error code is. If it did, the current implementation would be problematic because InstallableManager::GetData() will never return content::NO_MATCHING_SERVICE_WORKER because we set InstallableParams::wait_for_worker https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:329: I agree that it is harder to debug failures when you have a "magical test runner". However: - "magical test runners" sometimes make "what you are testing" clearer. In the case of these tests, I have to put in a lot of effort to figure out what the purpose of each test is and how each test is different from the next - test failures should be rare. - you can have EXPECT() and FAIL() calls in "magical test runners" I think that you are trying to write "black box tests". "black box tests" are in my opinion more amenable to "magical test runners" than "white box tests" Also, I think that add_to_homescreen_data_fetcher.cc is complicated enough to warrant "white box tests" https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:335: EXPECT_TRUE(fetcher->shortcut_info().best_primary_icon_url.is_empty()); There are a bunch of things that the tests are still not testing. - We should test that AddToHomescreenDataFetcher::Observer::OnDidDetermineWebApkCompatibility() is called exactly once. We used to have a bug where it was called multiple times - We should test AddToHomescreenDataFetcher::OnDidGetManifestAndIcons() being called after AddToHomescreenDataFetcher::OnDataTimedout() - We should test that the user does not get an empty home screen icon when the Web Manifest has no icons https://codereview.chromium.org/2960103002/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:340: EXPECT_TRUE(fetcher->shortcut_info().best_badge_icon_url.is_empty()); You could make this argument for all of the attributes of ShortcutInfo. From looking at the implementation of AddToHomescreenDataFetcher I don't see anything special about the badge icon handling.
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated as per our conversation, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Mostly comment-related comments https://codereview.chromium.org/2960103002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:322: Ping on this comment https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:154: if (!IsManifestValidForWebApp(manifest_)) Can you make IsManifestValidForWebApp() a static method which returns an error code in InstallableManager? I think that it would make things less awkward. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:165: &badge_icon_, params.check_installable ? is_installable_ : false}; For the sake of clarity, you should only return a primary icon and primary icon URL if InstallableParams::fetch_primary_icon is true The benefit of this is that you can merge SetManifest() and SetManifestAndIcons() and SetInstallableAndBadgeIcon() I am envisioning: void SetManifest(const GURL& url, const content::Manifest& manifest) { manifest_url_ = url; manifest_ = manifest; if (!manifest_.icons.empty()) { primary_icon_url_ = manifest_.icons[0].src; primary_icon_ = gfx::test::CreateBitmap(kIconSizePx, kIconSizePx)); badge_icon_url_ = manifest_.icons[0].src; badge_icon_ = gfx::test::CreateBitmap(kIconSizePx, kIconSizePx)); } } https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:169: (params.check_installable && should_installable_time_out_)) { Can this be simplified to: if (should_manifest_time_out_ || params.check_installable) {} or alternatively if (should_manifest_time_out_ || (params.check_installable && params.wait_for_worker)) {} https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:215: GURL manifest_url_; It seems like this is always kDefaultManifestUrl. Can we remove the |manifest_url_| variable? https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:221: bool is_installable_ = false; Should this variable be renamed to |has_service_worker_| https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:277: } Maybe we can make RunFetcher() call a helper function void CheckObserversNotified(ObserverWaiter& observer, int expected_count) { EXPECT_EQ(expected_count, observer.determined_webapk_compatibility_count()); EXPECT_EQ(expected_count, observer.title_available_count()); EXPECT_EQ(expected_count, observer.on_data_available_count()); } - The benefit of having CheckObserversNotified() is that you can run it after: waiter.ResetCounts() RunDeferredCallback() - I think that having accessors for the number of times that OnDidDetermineWebApkCompatibility() and OnUserTitleAvailable() and OnDataAvailable() are called is clearer than having EXPECT() calls in ObserverWaiter https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:312: void RunDeferredCallback() { installable_manager_->RunCallback(); } We should call base::RunLoop().RunUntilIdle() after calling the callback in case the OnData() callback calls CreateLauncherIcon() https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:360: // Test a manifest with no icons. This should only use the metadata title and Nit: "only use" -> "use" https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:362: // be replaced by a favicon or a generated icon). Thank you for the awesome explanatory comment! Things are much clearer now https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:385: // Check with a site that has a manifest and icons, but no service worker. If the manifest-fetch times out, I don't think that the nature of the manifest matters Maybe change the comment to: "Check with a site with no service worker." https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:395: // Ensure GetData can call the data fetcher callback after the time out. Nit: "GetData" -> "GetData()" Maybe add: "Test that the GetData() call does not call any of the observer's methods a second time." https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:417: TEST_F(AddToHomescreenDataFetcherTest, ServiceWorkerCheckTimesOut) { Because this is the only TEST_F() test, you can probably make this one TEST_P() as well and early return if !check_webapk_compatibility() https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:422: // compatibility. Can you make it clear that this test tests the case of a page which has a service worker but where AddToHomescreenDataFetcher returns prior to the asynchronous service worker check completing. We can optionally have a separate test case which tests a page with an installable manifest but no service worker The cases are different because the error code sent as a result of RunDeferredCallback() is different https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:463: // compatibility. Maybe change the comment to: "Check that the badge icon is requested only when AddToHomescreenDataFetcher checks for WebAPK compatibility." This test case should be in its own test. Perhaps: TEST_P(AddToHomescreenDataFetcherTestCommon, BadgeIconOnlyRequestedWhenCheckingForWebApkCompatibility) {} https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:509: // checking WebAPK compatibility. Maybe change the comment to: "Check page with no service worker." This way the comment applies regardless of whether AddToHomescreenDataFetcher checks for WebAPK-compatibility https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:528: RunDeferredCallback(); Maybe change the comment to: if (check_webapk_compatibility() { // InstallableManager::GetData() should have timed out waiting for a service // worker to be registered. Test that the GetData() call does not call any // of the observer's methods a second time. RunDeferredCallback(); } https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:535: // Check the case where the service worker check doesn't time out. Maybe change the comment to: "Check page with service worker." https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:557: // from the manifest. Nit: Please put each '-' on a separate line https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:566: SetShouldInstallableTimeOut(true); This case cannot occur in practice because IsManifestValidForWebApp() sets the MANIFEST_MISSING_NAME_OR_SHORT_NAME error code. The InstallableManager should not time out
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Thanks for the detailed review! https://codereview.chromium.org/2960103002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:322: On 2017/06/29 19:22:05, pkotwicz wrote: > Ping on this comment Somehow missed this. Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:154: if (!IsManifestValidForWebApp(manifest_)) On 2017/06/29 19:22:05, pkotwicz wrote: > Can you make IsManifestValidForWebApp() a static method which returns an error > code in InstallableManager? I think that it would make things less awkward. I'll do that in a follow up because it will mean changing a bunch of InstallableManager's tests as well. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:165: &badge_icon_, params.check_installable ? is_installable_ : false}; On 2017/06/29 19:22:06, pkotwicz wrote: > For the sake of clarity, you should only return a primary icon and primary icon > URL if InstallableParams::fetch_primary_icon is true > > The benefit of this is that you can merge SetManifest() and > SetManifestAndIcons() and SetInstallableAndBadgeIcon() > > I am envisioning: > void SetManifest(const GURL& url, const content::Manifest& manifest) { > manifest_url_ = url; > manifest_ = manifest; > if (!manifest_.icons.empty()) { > primary_icon_url_ = manifest_.icons[0].src; > primary_icon_ = gfx::test::CreateBitmap(kIconSizePx, kIconSizePx)); > badge_icon_url_ = manifest_.icons[0].src; > badge_icon_ = gfx::test::CreateBitmap(kIconSizePx, kIconSizePx)); > } > } Good suggestion, done https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:169: (params.check_installable && should_installable_time_out_)) { On 2017/06/29 19:22:05, pkotwicz wrote: > Can this be simplified to: > > if (should_manifest_time_out_ || params.check_installable) {} > > or alternatively > > if (should_manifest_time_out_ || > (params.check_installable && params.wait_for_worker)) {} Not really, because we want to test the first GetData() call (manifest + icon) and the second (service worker) timing out / not timing out separately. - we can't use wait_for_worker because that's always true - similarly check_installable is always true if we're checking for Webapk-compatibility. We need a separate mechanism so that we can test checking compatibility but it times out. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:215: GURL manifest_url_; On 2017/06/29 19:22:05, pkotwicz wrote: > It seems like this is always kDefaultManifestUrl. Can we remove the > |manifest_url_| variable? Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:221: bool is_installable_ = false; On 2017/06/29 19:22:05, pkotwicz wrote: > Should this variable be renamed to |has_service_worker_| I want to keep it is_installable_ to match the name of the parameter passed back from GetData(). It also checks if the manifest has display: standalone, which is another separate thing we could test. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:277: } On 2017/06/29 19:22:05, pkotwicz wrote: > Maybe we can make RunFetcher() call a helper function > > void CheckObserversNotified(ObserverWaiter& observer, int expected_count) { > EXPECT_EQ(expected_count, observer.determined_webapk_compatibility_count()); > EXPECT_EQ(expected_count, observer.title_available_count()); > EXPECT_EQ(expected_count, observer.on_data_available_count()); > } > > - The benefit of having CheckObserversNotified() is that you can run it after: > waiter.ResetCounts() > RunDeferredCallback() > > - I think that having accessors for the number of times that > OnDidDetermineWebApkCompatibility() and OnUserTitleAvailable() and > OnDataAvailable() are called is clearer than having EXPECT() calls in > ObserverWaiter I don't see a strong enough benefit here - just using scopes and refreshing the state entirely with a new object seems just as clear. We don't have any scenario where it's correct to call the observer methods more than once, but keeping counts implies that it's okay to call them more than once. The EXPECT is very explicit that for each observer's lifetime, it should get one call. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:312: void RunDeferredCallback() { installable_manager_->RunCallback(); } On 2017/06/29 19:22:06, pkotwicz wrote: > We should call > base::RunLoop().RunUntilIdle() > after calling the callback in case the OnData() callback calls > CreateLauncherIcon() Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:360: // Test a manifest with no icons. This should only use the metadata title and On 2017/06/29 19:22:06, pkotwicz wrote: > Nit: "only use" -> "use" Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:362: // be replaced by a favicon or a generated icon). On 2017/06/29 19:22:06, pkotwicz wrote: > Thank you for the awesome explanatory comment! Things are much clearer now :) https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:385: // Check with a site that has a manifest and icons, but no service worker. On 2017/06/29 19:22:05, pkotwicz wrote: > If the manifest-fetch times out, I don't think that the nature of the manifest > matters > > Maybe change the comment to: > "Check with a site with no service worker." Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:395: // Ensure GetData can call the data fetcher callback after the time out. On 2017/06/29 19:22:06, pkotwicz wrote: > Nit: "GetData" -> "GetData()" > > Maybe add: "Test that the GetData() call does not call any of the observer's > methods a second time." Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:417: TEST_F(AddToHomescreenDataFetcherTest, ServiceWorkerCheckTimesOut) { On 2017/06/29 19:22:05, pkotwicz wrote: > Because this is the only TEST_F() test, you can probably make this one TEST_P() > as well and early return if !check_webapk_compatibility() That leaves a do-nothing test, which is a bit misleading. I'd prefer to keep it a TEST_F if that's okay. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:422: // compatibility. On 2017/06/29 19:22:06, pkotwicz wrote: > Can you make it clear that this test tests the case of a page which has a > service worker but where AddToHomescreenDataFetcher returns prior to the > asynchronous service worker check completing. > > We can optionally have a separate test case which tests a page with an > installable manifest but no service worker > > The cases are different because the error code sent as a result of > RunDeferredCallback() is different Very subtle difference. Added a new test for it. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:463: // compatibility. On 2017/06/29 19:22:06, pkotwicz wrote: > Maybe change the comment to: > "Check that the badge icon is requested only when AddToHomescreenDataFetcher > checks for WebAPK compatibility." > > This test case should be in its own test. Perhaps: > TEST_P(AddToHomescreenDataFetcherTestCommon, > BadgeIconOnlyRequestedWhenCheckingForWebApkCompatibility) {} Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:509: // checking WebAPK compatibility. On 2017/06/29 19:22:05, pkotwicz wrote: > Maybe change the comment to: "Check page with no service worker." This way the > comment applies regardless of whether AddToHomescreenDataFetcher checks for > WebAPK-compatibility Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:528: RunDeferredCallback(); On 2017/06/29 19:22:06, pkotwicz wrote: > Maybe change the comment to: > if (check_webapk_compatibility() { > // InstallableManager::GetData() should have timed out waiting for a service > // worker to be registered. Test that the GetData() call does not call any > // of the observer's methods a second time. > RunDeferredCallback(); > } Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:535: // Check the case where the service worker check doesn't time out. On 2017/06/29 19:22:06, pkotwicz wrote: > Maybe change the comment to: "Check page with service worker." Done. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:557: // from the manifest. On 2017/06/29 19:22:05, pkotwicz wrote: > Nit: Please put each '-' on a separate line Ugh that was a clang-format fail. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:566: SetShouldInstallableTimeOut(true); On 2017/06/29 19:22:06, pkotwicz wrote: > This case cannot occur in practice because IsManifestValidForWebApp() sets the > MANIFEST_MISSING_NAME_OR_SHORT_NAME error code. > > The InstallableManager should not time out Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:154: if (!IsManifestValidForWebApp(manifest_)) Fair enough https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:169: (params.check_installable && should_installable_time_out_)) { You are right, my suggestion has problems. We should only time out if: - we are checking for installability - the page does not have a service worker - there is no other error I guess that would make it if (should_manifest_time_out_ || (params.check_installable && code == NO_MATCHING_SERVICE_WORKER)) { ... } I thinking that lines 153-158 could look like this: if (params.check_installable) { if (!IsManifestValidForWebApp(manifest_)) { ... } else if (service_worker_error_code_ != NO_ERROR_DETECTED) { code = service_worker_error_code_; } } SetServiceWorkerErrorCode() would replace SetInstallable() and SetShouldInstallableTimeOut() https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:417: TEST_F(AddToHomescreenDataFetcherTest, ServiceWorkerCheckTimesOut) { Fair enough https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:389: ObserverWaiter waiter; The setup here is identical to the setup on line 371 https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:408: // it's taken too long) .This should use the short_name and icon from the Nit: ' .T' -> '. T' https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:434: // NOT_OFFLINE_CAPABLE as above. After giving the matter some thought, I don't think that we should test the case of the InstallableManager::GetData() callback being called after AddToHomescreenDataFetcher times out. I think that testing this case is too confusing I have written https://codereview.chromium.org/2968693003/ in order to make testing the InstallableManager::GetData() callback being called after AddToHomescreenDataFetcher times out unnecessary Doesn't this case test: - a page has a service worker - the service worker does not have a fetch handler - the service worker check times out because it takes too long If a page truly does not have a service worker, the second OnData() callback will never be called https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:484: ObserverWaiter waiter; The setup in this test seems identical to the setup in InstallableManifest. Can lines 471-474 replace lines 493-497 (and lines 471-474 be deleted) https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:504: // Manifest::name that Manifest::short_name is used as the name. Can we flip this test case around to have an empty Manifest::short_name but a non-empty Manifest::name (So that the title is different than in the case where both are present)
https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:169: (params.check_installable && should_installable_time_out_)) { The initial manifest fetch is always made, and it could time out too (and that's a separate code path so it should be tested). Since we're no longer calling the callback on a delayed basis, I think using booleans to indicate if the installable check should time out is sufficient again (I don't really think it's necessary to check the distinction between NO_MATCHING_SERVICE_WORKER or NOT_OFFLINE_CAPABLE in here). https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:389: ObserverWaiter waiter; On 2017/06/30 23:54:52, pkotwicz wrote: > The setup here is identical to the setup on line 371 Done. https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:408: // it's taken too long) .This should use the short_name and icon from the On 2017/06/30 23:54:52, pkotwicz wrote: > Nit: ' .T' -> '. T' Done. https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:434: // NOT_OFFLINE_CAPABLE as above. This test case is checking the case where we have a service worker with no fetch handler, but the check is too slow and times out (as opposed to the first case where we are actually installable, just slow). https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:484: ObserverWaiter waiter; On 2017/06/30 23:54:52, pkotwicz wrote: > The setup in this test seems identical to the setup in InstallableManifest. > > Can lines 471-474 replace lines 493-497 (and lines 471-474 be deleted) Done. https://codereview.chromium.org/2960103002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:504: // Manifest::name that Manifest::short_name is used as the name. On 2017/06/30 23:54:52, pkotwicz wrote: > Can we flip this test case around to have an empty Manifest::short_name but a > non-empty Manifest::name (So that the title is different than in the case where > both are present) Done.
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:169: (params.check_installable && should_installable_time_out_)) { Because SetShouldInstallableTimeOut() takes precedence over SetInstallable(), I think that we can safely remove all of the calls to SetInstallable(). All of the current calls to SetInstallable(false) occur occur when SetShouldInstallableTimeOut(true) https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:317: // Test a manifest with no icons. This should use the metadata title and have The AddToHomescreenDataFetcher uses kDefaultManifestShortName not the metadata title. https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:358: // Check with a service worker present. Lines 346-354 are identical to lines 359-367 https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:398: SetShouldInstallableTimeOut(true); Remove call to SetInstallable(false) because SetShouldInstallableTimeOut(true) takes precedence over SetInstallable(false) in TestInstallableManager. In my opinion, setting SetInstallable(false) and SetShouldInstallableTimeOut(true) amounts to testing the test harness Removing SetInstallable(false) makes lines 378-391 and lines 397-410 identical https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:472: SetManifest(manifest); Nit: Can you move the manifest construction out of the nested scope because the same manifest is used by this test and the next two? https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:474: SetInstallable(false); Nit: Remove call to SetInstallable(false) because SetShouldInstallableTimeOut(true) takes precedence over SetInstallable(false) in TestInstallableManager
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:317: // Test a manifest with no icons. This should use the metadata title and have On 2017/07/05 21:37:43, pkotwicz wrote: > The AddToHomescreenDataFetcher uses kDefaultManifestShortName not the metadata > title. Done. https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:358: // Check with a service worker present. On 2017/07/05 21:37:43, pkotwicz wrote: > Lines 346-354 are identical to lines 359-367 Done. https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:398: SetShouldInstallableTimeOut(true); On 2017/07/05 21:37:43, pkotwicz wrote: > Remove call to SetInstallable(false) because > SetShouldInstallableTimeOut(true) takes precedence over SetInstallable(false) in > TestInstallableManager. In my opinion, setting SetInstallable(false) and > SetShouldInstallableTimeOut(true) amounts to testing the test harness > > Removing SetInstallable(false) makes lines 378-391 and lines 397-410 identical Done. https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:472: SetManifest(manifest); On 2017/07/05 21:37:43, pkotwicz wrote: > Nit: Can you move the manifest construction out of the nested scope because the > same manifest is used by this test and the next two? Done. https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:474: SetInstallable(false); On 2017/07/05 21:37:43, pkotwicz wrote: > Nit: Remove call to SetInstallable(false) because > SetShouldInstallableTimeOut(true) takes precedence over SetInstallable(false) in > TestInstallableManager Done. https://codereview.chromium.org/2960103002/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:474: SetInstallable(false); On 2017/07/05 21:37:43, pkotwicz wrote: > Nit: Remove call to SetInstallable(false) because > SetShouldInstallableTimeOut(true) takes precedence over SetInstallable(false) in > TestInstallableManager Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dominickn@chromium.org
I think that you missed a few comments from the previous patch set https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:169: (params.check_installable && should_installable_time_out_)) { Ping on this comment https://codereview.chromium.org/2960103002/diff/120001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:455: // out because its too slow. As per the comment in the previous patch set, isn't this test case identical to the one above. https://codereview.chromium.org/2960103002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:457: SetInstallable(true); Isn't the call to SetInstallable(true) a no-op? In particular from line 150 - 154 if (params.check_installable) { if (!is_installable_) code = NO_MATCHING_SERVICE_WORKER; } https://codereview.chromium.org/2960103002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:504: SetShouldInstallableTimeOut(false); Nit: The SetShouldInstallableTimeOut() is unnecessary because "false" is the default value
Also you will need to merge with https://codereview.chromium.org/2968693003/ which landed an hour ago. Merging mostly involves deleting fetcher->set_weak_observer(nullptr); calls
It is very difficult to follow up on comments on previous patchsets when there is this much churn happening. Some previous patchsets have 40+ comments. If possible, please try to keep comments on the latest patchset, and as detailed as possible. I know your CL already landed. I'm not going to rebase until we're done with all of this back and forth because reviewing a CL with a rebase in the middle is very difficult. https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:169: (params.check_installable && should_installable_time_out_)) { On 2017/07/06 02:03:16, pkotwicz wrote: > Ping on this comment SetInstallable defaults to false and is explicitly set to true in a couple of tests. Removing it doesn't make sense for the NOT_OFFLINE_CAPABLE case which will deterministically return. https://codereview.chromium.org/2960103002/diff/120001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:455: // out because its too slow. On 2017/07/06 02:03:16, pkotwicz wrote: > As per the comment in the previous patch set, isn't this test case identical to > the one above. Done. https://codereview.chromium.org/2960103002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:457: SetInstallable(true); On 2017/07/06 02:03:16, pkotwicz wrote: > Isn't the call to SetInstallable(true) a no-op? > > In particular from line 150 - 154 > > if (params.check_installable) { > if (!is_installable_) > code = NO_MATCHING_SERVICE_WORKER; > } Done. https://codereview.chromium.org/2960103002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:504: SetShouldInstallableTimeOut(false); On 2017/07/06 02:03:16, pkotwicz wrote: > Nit: The SetShouldInstallableTimeOut() is unnecessary because "false" is the > default value Done. https://codereview.chromium.org/2960103002/diff/120001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:504: SetShouldInstallableTimeOut(false); On 2017/07/06 02:03:16, pkotwicz wrote: > Nit: The SetShouldInstallableTimeOut() is unnecessary because "false" is the > default value Done.
https://codereview.chromium.org/2960103002/diff/140001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/140001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:144: (params.check_installable && should_installable_time_out_)) { >>> Because SetShouldInstallableTimeOut() takes precedence over SetInstallable(), I >>> think that we can safely remove all of the calls to SetInstallable(). All of the >>> current calls to SetInstallable(false) occur occur when >>> SetShouldInstallableTimeOut(true) >> SetInstallable defaults to false and is explicitly set to true in a couple of >> tests. Removing it doesn't make sense for the NOT_OFFLINE_CAPABLE case which >>will deterministically return. The default value of |is_installable_| depends on whether the Web Manifest has icons or not. See the implementation of TestInstallableManager::SetManifest() In order to test the NOT_OFFLINE_CAPABLE case, you need to: - Call SetManifest() with an installable Web Manifest - Call SetInstallable(false) https://codereview.chromium.org/2960103002/diff/140001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:438: // Check a site with no offline-capable service worker. The comment for this test case is confusing. I am confused whether this test case is testing the case of: A) A page without a service worker OR B) A page with a service worker but no fetch handler If the page has a service worker but the service worker does not have a fetch handler, that is equivalent to: SetShouldInstallableTimeOut(false) SetInstallable(false)
Thanks for picking these up. https://codereview.chromium.org/2960103002/diff/140001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/140001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:144: (params.check_installable && should_installable_time_out_)) { On 2017/07/06 20:49:30, pkotwicz wrote: > >>> Because SetShouldInstallableTimeOut() takes precedence over > SetInstallable(), I > >>> think that we can safely remove all of the calls to SetInstallable(). All of > the > >>> current calls to SetInstallable(false) occur occur when > >>> SetShouldInstallableTimeOut(true) > > >> SetInstallable defaults to false and is explicitly set to true in a couple of > >> tests. Removing it doesn't make sense for the NOT_OFFLINE_CAPABLE case which > >>will deterministically return. > > The default value of |is_installable_| depends on whether the Web Manifest has > icons or not. See the implementation of TestInstallableManager::SetManifest() > > In order to test the NOT_OFFLINE_CAPABLE case, you need to: > - Call SetManifest() with an installable Web Manifest > - Call SetInstallable(false) Good catch, done. https://codereview.chromium.org/2960103002/diff/140001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:438: // Check a site with no offline-capable service worker. On 2017/07/06 20:49:30, pkotwicz wrote: > The comment for this test case is confusing. I am confused whether this test > case is testing the case of: > A) A page without a service worker > OR > B) A page with a service worker but no fetch handler > > If the page has a service worker but the service worker does not have a fetch > handler, that is equivalent to: > > SetShouldInstallableTimeOut(false) > SetInstallable(false) Fixed. The intention is to exercise the three code paths: a) we get the manifest, and the worker is not offline capable b) we get the manifest, and we time out waiting for the worker c) we get the manifest and we get the worker.
LGTM with optional comment Thank you for bearing with me! https://codereview.chromium.org/2960103002/diff/160001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/160001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:154: } Optional: It might be clearer if you added the following before the if(params.check_installable) {} block InstallableStatusCode code = NO_ERROR_DETECTED; if (params.fetch_valid_primary_icon && !primary_icon_) { code = NO_ACCEPTABLE_ICON } This enables you to get rid of the |TestInstallableManager::code_| member variable and to no longer assign to |TestInstallableManager::is_installable_| in SetManifest() The reason that I am making this suggestion is because I think that assigning to |TestInstallableManager::is_installable_| in SetManifest() is confusing https://codereview.chromium.org/2960103002/diff/160001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:345: // Check a site with no offline-capable service worker. Nit: Move the comment above the SetInstallable() call https://codereview.chromium.org/2960103002/diff/160001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:389: // There should always be a primary icon. The badge icon should only be Nit: Remove the sentence about "The badge icon should only be present ..." It duplicates the comment on line 395
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for sticking through this! Rebased + nits addressed in the last two patchsets. https://codereview.chromium.org/2960103002/diff/160001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc (right): https://codereview.chromium.org/2960103002/diff/160001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:154: } On 2017/07/10 21:14:40, pkotwicz wrote: > Optional: It might be clearer if you added the following before the > if(params.check_installable) {} block > > InstallableStatusCode code = NO_ERROR_DETECTED; > if (params.fetch_valid_primary_icon && !primary_icon_) { > code = NO_ACCEPTABLE_ICON > } > > This enables you to get rid of the |TestInstallableManager::code_| member > variable and to no longer assign to |TestInstallableManager::is_installable_| in > SetManifest() > > The reason that I am making this suggestion is because I think that assigning to > |TestInstallableManager::is_installable_| in SetManifest() is confusing Good suggestion, done. https://codereview.chromium.org/2960103002/diff/160001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:345: // Check a site with no offline-capable service worker. On 2017/07/10 21:14:40, pkotwicz wrote: > Nit: Move the comment above the SetInstallable() call Done. https://codereview.chromium.org/2960103002/diff/160001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:389: // There should always be a primary icon. The badge icon should only be On 2017/07/10 21:14:40, pkotwicz wrote: > Nit: Remove the sentence about "The badge icon should only be present ..." It > duplicates the comment on line 395 Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2960103002/#ps240001 (title: "Self nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1499741719246340,
"parent_rev": "251d69bbe3292b89dd7d895fa4fb99a9d1ec5a89", "commit_rev":
"14dca19610d4b387af4f896d756f2cd9af5abf23"}
Message was sent while issue was closed.
Description was changed from ========== Improve add to homescreen data fetcher unit tests. Existing tests for this component don't work. The service worker registration is for a different browser context, so the tests never actuallly verify WebAPK-compatibility properly. This CL revamps the tests by mocking out InstallableManager instead of WebContents. This allows precise control over the data returned to the data fetcher so we can verify more scenarios more accurately. BUG=721881 ========== to ========== Improve add to homescreen data fetcher unit tests. Existing tests for this component don't work. The service worker registration is for a different browser context, so the tests never actuallly verify WebAPK-compatibility properly. This CL revamps the tests by mocking out InstallableManager instead of WebContents. This allows precise control over the data returned to the data fetcher so we can verify more scenarios more accurately. BUG=721881 Review-Url: https://codereview.chromium.org/2960103002 Cr-Commit-Position: refs/heads/master@{#485505} Committed: https://chromium.googlesource.com/chromium/src/+/14dca19610d4b387af4f896d756f... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/14dca19610d4b387af4f896d756f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
