Fix ArcAppModelBuilderTest

chrome/browser/ui/app_list/arc/arc_app_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <map>
 #include <string>
@@ skipping 48 matching lines @@
   const gfx::ImageSkia& image() { return image_; }
 
  private:
   size_t update_image_cnt_ = 0;
   std::string app_id_;
   gfx::ImageSkia image_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeAppIconLoaderDelegate);
 };
 
+void WaitForIconReady(ArcAppListPrefs* prefs,
+                      const std::string& app_id,
+                      ui::ScaleFactor scale_factor) {
+  const base::FilePath icon_path = prefs->GetIconPath(app_id, scale_factor);
+  // Process pending tasks. This performs multiple thread hops, so we need
+  // to run it continuously until it is resolved.
+  do {
+    content::BrowserThread::GetBlockingPool()->FlushForTesting();
+    base::RunLoop().RunUntilIdle();
+  } while (!base::PathExists(icon_path));
+}
+
 }  // namespace
 
 class ArcAppModelBuilderTest : public AppListTestBase {
  public:
   ArcAppModelBuilderTest() {}
   ~ArcAppModelBuilderTest() override {
     // Release profile file in order to keep right sequence.
     profile_.reset();
   }
 
   void SetUp() override {
     AppListTestBase::SetUp();
     arc_test_.SetUp(profile_.get());
     CreateBuilder();
+
+    // Validating decoded content does not fit well for unit tests.
+    ArcAppIcon::DisableSafeDecodingForTesting();
   }
 
   void TearDown() override {
     arc_test_.TearDown();
     ResetBuilder();
   }
 
  protected:
   // Creates a new builder, destroying any existing one.
   void CreateBuilder() {
@@ skipping 447 matching lines @@
   item_first->Activate(0);
   app_instance()->WaitForIncomingMethodCall();
   item_last->Activate(0);
   app_instance()->WaitForIncomingMethodCall();
   item_first->Activate(0);
   app_instance()->WaitForIncomingMethodCall();
 
   const ScopedVector<arc::FakeAppInstance::Request>& launch_requests =
       app_instance()->launch_requests();
   ASSERT_EQ(3u, launch_requests.size());
-  EXPECT_EQ(true, launch_requests[0]->IsForApp(app_first));
-  EXPECT_EQ(true, launch_requests[1]->IsForApp(app_last));
-  EXPECT_EQ(true, launch_requests[2]->IsForApp(app_first));
+  EXPECT_TRUE(launch_requests[0]->IsForApp(app_first));
+  EXPECT_TRUE(launch_requests[1]->IsForApp(app_last));
+  EXPECT_TRUE(launch_requests[2]->IsForApp(app_first));
 
   // Test an attempt to launch of a not-ready app.
   bridge_service()->SetStopped();
   item_first = FindArcItem(ArcAppTest::GetAppId(app_first));
   ASSERT_NE(nullptr, item_first);
   size_t launch_request_count_before = app_instance()->launch_requests().size();
   item_first->Activate(0);
   // Number of launch requests must not change.
   EXPECT_EQ(launch_request_count_before,
             app_instance()->launch_requests().size());
@@ skipping 17 matching lines @@
   item_first->Activate(0);
   app_instance()->WaitForIncomingMethodCall();
   item_last->Activate(0);
   app_instance()->WaitForIncomingMethodCall();
   item_first->Activate(0);
   app_instance()->WaitForIncomingMethodCall();
 
   const ScopedVector<mojo::String>& launch_intents =
       app_instance()->launch_intents();
   ASSERT_EQ(3u, launch_intents.size());
-  EXPECT_EQ(true, app_first.intent_uri == *launch_intents[0]);
-  EXPECT_EQ(true, app_last.intent_uri == *launch_intents[1]);
-  EXPECT_EQ(true, app_first.intent_uri == *launch_intents[2]);
+  EXPECT_EQ(app_first.intent_uri, *launch_intents[0]);
+  EXPECT_EQ(app_last.intent_uri, *launch_intents[1]);
+  EXPECT_EQ(app_first.intent_uri, *launch_intents[2]);
 
   // Test an attempt to launch of a not-ready shortcut.
   bridge_service()->SetStopped();
   item_first = FindArcItem(ArcAppTest::GetAppId(app_first));
   ASSERT_NE(nullptr, item_first);
   size_t launch_request_count_before = app_instance()->launch_intents().size();
   item_first->Activate(0);
   // Number of launch requests must not change.
   EXPECT_EQ(launch_request_count_before,
             app_instance()->launch_intents().size());
@@ skipping 12 matching lines @@
   uint32_t expected_mask = 0;
   const std::vector<ui::ScaleFactor>& scale_factors =
       ui::GetSupportedScaleFactors();
   for (auto& scale_factor : scale_factors) {
     expected_mask |= 1 <<  scale_factor;
     for (auto& app : fake_apps()) {
       ArcAppItem* app_item = FindArcItem(ArcAppTest::GetAppId(app));
       ASSERT_NE(nullptr, app_item);
       const float scale = ui::GetScaleForScaleFactor(scale_factor);
       app_item->icon().GetRepresentation(scale);
+
+      // This does not result in an icon being loaded, so WaitForIconReady
+      // cannot be used.
+      content::BrowserThread::GetBlockingPool()->FlushForTesting();
+      base::RunLoop().RunUntilIdle();
     }
   }
 
-  // Process pending tasks.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
   // Normally just one call to RunUntilIdle() suffices to make sure
   // all RequestAppIcon() calls are delivered, but on slower machines
   // (especially when running under Valgrind), they might not get
   // delivered on time. Wait for the remaining tasks individually.
   const size_t expected_size = scale_factors.size() * fake_apps().size();
   while (app_instance()->icon_requests().size() < expected_size) {
     app_instance()->WaitForIncomingMethodCall();
   }
 
   // At this moment we should receive all requests for icon loading.
@@ skipping 14 matching lines @@
   // Validate that we have a request for each icon for each supported scale
   // factor.
   EXPECT_EQ(fake_apps().size(), app_masks.size());
   for (auto& app : fake_apps()) {
     const std::string id = ArcAppTest::GetAppId(app);
     ASSERT_NE(app_masks.find(id), app_masks.end());
     EXPECT_EQ(app_masks[id], expected_mask);
   }
 }
 
-// TODO(crbug.com/624446) - reenable once this test is fixed
-TEST_F(ArcAppModelBuilderTest, DISABLED_RequestShortcutIcons) {
+TEST_F(ArcAppModelBuilderTest, RequestShortcutIcons) {
   // Make sure we are on UI thread.
   ASSERT_TRUE(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   bridge_service()->SetReady();
   const arc::mojom::ShortcutInfo& shortcut = fake_shortcuts()[0];
   app_instance()->SendInstallShortcut(shortcut);
 
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_.get());
   ASSERT_NE(nullptr, prefs);
 
   // Validate that no icon exists at the beginning and request icon for
   // each supported scale factor. This will start asynchronous loading.
   uint32_t expected_mask = 0;
   ArcAppItem* app_item = FindArcItem(ArcAppTest::GetAppId(shortcut));
   ASSERT_NE(nullptr, app_item);
   const std::vector<ui::ScaleFactor>& scale_factors =
       ui::GetSupportedScaleFactors();
   for (auto& scale_factor : scale_factors) {
     expected_mask |= 1 << scale_factor;
     const float scale = ui::GetScaleForScaleFactor(scale_factor);
     const base::FilePath icon_path =
         prefs->GetIconPath(ArcAppTest::GetAppId(shortcut), scale_factor);
-    EXPECT_EQ(true, !base::PathExists(icon_path));
+    EXPECT_FALSE(base::PathExists(icon_path));
 
     app_item->icon().GetRepresentation(scale);
+    WaitForIconReady(prefs, ArcAppTest::GetAppId(shortcut), scale_factor);
   }
 
-  // Process pending tasks.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-
   // At this moment we should receive all requests for icon loading.
   const size_t expected_size = scale_factors.size();
   const ScopedVector<arc::FakeAppInstance::ShortcutIconRequest>& icon_requests =
       app_instance()->shortcut_icon_requests();
   EXPECT_EQ(expected_size, icon_requests.size());
   uint32_t app_mask = 0;
   for (size_t i = 0; i < icon_requests.size(); ++i) {
     const arc::FakeAppInstance::ShortcutIconRequest* icon_request =
         icon_requests[i];
     EXPECT_EQ(shortcut.icon_resource_id, icon_request->icon_resource_id());
 
     // Make sure no double requests.
     EXPECT_NE(app_mask, app_mask | (1 << icon_request->scale_factor()));
     app_mask |= (1 << icon_request->scale_factor());
   }
 
   // Validate that we have a request for each icon for each supported scale
   // factor.
   EXPECT_EQ(app_mask, expected_mask);
 
   // Validate all icon files are installed.
   for (auto& scale_factor : scale_factors) {
     const base::FilePath icon_path =
         prefs->GetIconPath(ArcAppTest::GetAppId(shortcut), scale_factor);
-    EXPECT_EQ(true, base::PathExists(icon_path));
+    EXPECT_TRUE(base::PathExists(icon_path));
   }
 }
 
 TEST_F(ArcAppModelBuilderTest, InstallIcon) {
   // Make sure we are on UI thread.
   ASSERT_TRUE(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   bridge_service()->SetReady();
   app_instance()->RefreshAppList();
   app_instance()->SendRefreshAppList(std::vector<arc::mojom::AppInfo>(
       fake_apps().begin(), fake_apps().begin() + 1));
   const arc::mojom::AppInfo& app = fake_apps()[0];
 
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_.get());
   ASSERT_NE(nullptr, prefs);
 
   const ui::ScaleFactor scale_factor = ui::GetSupportedScaleFactors()[0];
   const float scale = ui::GetScaleForScaleFactor(scale_factor);
   const base::FilePath icon_path = prefs->GetIconPath(ArcAppTest::GetAppId(app),
                                                       scale_factor);
-  EXPECT_EQ(true, !base::PathExists(icon_path));
+  EXPECT_FALSE(base::PathExists(icon_path));
 
   const ArcAppItem* app_item = FindArcItem(ArcAppTest::GetAppId(app));
   EXPECT_NE(nullptr, app_item);
   // This initiates async loading.
   app_item->icon().GetRepresentation(scale);
 
-  // Process pending tasks.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-
-  // Validating decoded content does not fit well for unit tests.
-  ArcAppIcon::DisableSafeDecodingForTesting();
-
   // Now send generated icon for the app.
   std::string png_data;
-  EXPECT_EQ(true, app_instance()->GenerateAndSendIcon(
-                      app, static_cast<arc::mojom::ScaleFactor>(scale_factor),
-                      &png_data));
-
-  // Process pending tasks.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(app_instance()->GenerateAndSendIcon(
+      app, static_cast<arc::mojom::ScaleFactor>(scale_factor), &png_data));
+  WaitForIconReady(prefs, ArcAppTest::GetAppId(app), scale_factor);
 
   // Validate that icons are installed, have right content and icon is
   // refreshed for ARC app item.
-  EXPECT_EQ(true, base::PathExists(icon_path));
+  EXPECT_TRUE(base::PathExists(icon_path));
 
   std::string icon_data;
   // Read the file from disk and compare with reference data.
-  EXPECT_EQ(true, base::ReadFileToString(icon_path, &icon_data));
+  EXPECT_TRUE(base::ReadFileToString(icon_path, &icon_data));
   ASSERT_EQ(icon_data, png_data);
 }
 
 TEST_F(ArcAppModelBuilderTest, RemoveAppCleanUpFolder) {
   // Make sure we are on UI thread.
   ASSERT_TRUE(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   bridge_service()->SetReady();
   app_instance()->RefreshAppList();
   app_instance()->SendRefreshAppList(std::vector<arc::mojom::AppInfo>(
       fake_apps().begin(), fake_apps().begin() + 1));
   const arc::mojom::AppInfo& app = fake_apps()[0];
 
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_.get());
   ASSERT_NE(nullptr, prefs);
 
   const std::string app_id = ArcAppTest::GetAppId(app);
   const base::FilePath app_path = prefs->GetAppPath(app_id);
   const ui::ScaleFactor scale_factor = ui::GetSupportedScaleFactors()[0];
 
   // No app folder by default.
-  EXPECT_EQ(true, !base::PathExists(app_path));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(app_path));
 
   // Request icon, this will create app folder.
   prefs->RequestIcon(app_id, scale_factor);
   // Now send generated icon for the app.
   std::string png_data;
-  EXPECT_EQ(true, app_instance()->GenerateAndSendIcon(
-                      app, static_cast<arc::mojom::ScaleFactor>(scale_factor),
-                      &png_data));
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(true, base::PathExists(app_path));
+  EXPECT_TRUE(app_instance()->GenerateAndSendIcon(
+      app, static_cast<arc::mojom::ScaleFactor>(scale_factor), &png_data));
+  WaitForIconReady(prefs, app_id, scale_factor);
+  EXPECT_TRUE(base::PathExists(app_path));
 
   // Send empty app list. This will delete app and its folder.
   app_instance()->SendRefreshAppList(std::vector<arc::mojom::AppInfo>());
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(true, !base::PathExists(app_path));
+  // Process pending tasks. This performs multiple thread hops, so we need
+  // to run it continuously until it is resolved.
+  do {
+    content::BrowserThread::GetBlockingPool()->FlushForTesting();
+    base::RunLoop().RunUntilIdle();
+  } while (base::PathExists(app_path));

[xiyuan, 2016/07/14 22:59:32]

Could this be WaitForIconReady ?

[Luis Héctor Chávez, 2016/07/14 23:25:45]

No, it needs the opposite condition. I'll add a comment.

+  EXPECT_FALSE(base::PathExists(app_path));
 }
 
 TEST_F(ArcAppModelBuilderTest, LastLaunchTime) {
   // Make sure we are on UI thread.
   ASSERT_TRUE(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   bridge_service()->SetReady();
   app_instance()->RefreshAppList();
   app_instance()->SendRefreshAppList(std::vector<arc::mojom::AppInfo>(
       fake_apps().begin(), fake_apps().begin() + 2));
@@ skipping 26 matching lines @@
   base::Time time_before = base::Time::Now();
   arc::LaunchApp(profile(), id2);
   base::Time time_after = base::Time::Now();
 
   app_info = prefs->GetApp(id2);
   ASSERT_NE(nullptr, app_info.get());
   ASSERT_LE(time_before, app_info->last_launch_time);
   ASSERT_GE(time_after, app_info->last_launch_time);
 }
 
 TEST_F(ArcAppModelBuilderTest, IconLoader) {

[Luis Héctor Chávez, 2016/07/14 22:44:32]

This test is unfortunately still flaky (5-10%) even after doing all this. Should
I disable it?

[xiyuan, 2016/07/14 22:59:32]

Yes, we should disable it until resolved so that sheriffs have an easier life.

-  // Validating decoded content does not fit well for unit tests.
-  ArcAppIcon::DisableSafeDecodingForTesting();
-
   const arc::mojom::AppInfo& app = fake_apps()[0];
   const std::string app_id = ArcAppTest::GetAppId(app);
 
+  ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_.get());
+  ASSERT_NE(nullptr, prefs);
+
   bridge_service()->SetReady();
   app_instance()->RefreshAppList();
   app_instance()->SendRefreshAppList(std::vector<arc::mojom::AppInfo>(
       fake_apps().begin(), fake_apps().begin() + 1));
 
   FakeAppIconLoaderDelegate delegate;
   ArcAppIconLoader icon_loader(profile(),
                                app_list::kListIconSize,
                                &delegate);
   EXPECT_EQ(0UL, delegate.update_image_cnt());
   icon_loader.FetchImage(app_id);
   EXPECT_EQ(1UL, delegate.update_image_cnt());
   EXPECT_EQ(app_id, delegate.app_id());
 
   // Validate default image.
   ValidateIcon(delegate.image());
 
   const std::vector<ui::ScaleFactor>& scale_factors =
       ui::GetSupportedScaleFactors();
+  ArcAppItem* app_item = FindArcItem(ArcAppTest::GetAppId(app));

[xiyuan, 2016/07/14 22:59:32]

nit: Isn't ArcAppTest::GetAppId(app) here the |app_id| in line 866?

[Luis Héctor Chávez, 2016/07/14 23:25:45]

Good catch, done.

   for (auto& scale_factor : scale_factors) {
     std::string png_data;
     EXPECT_TRUE(app_instance()->GenerateAndSendIcon(
         app, static_cast<arc::mojom::ScaleFactor>(scale_factor), &png_data));
+    const float scale = ui::GetScaleForScaleFactor(scale_factor);
+    // Force the icon to be loaded.
+    app_item->icon().GetRepresentation(scale);
+    WaitForIconReady(prefs, ArcAppTest::GetAppId(app), scale_factor);

[xiyuan, 2016/07/14 22:59:32]

nit: ArcAppTest::GetAppId(app) -> app_id ?

[Luis Héctor Chávez, 2016/07/14 23:25:45]

Done.

   }
 
-  // Process pending tasks, this installs icons.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-
-  // Allow one more circle to read and decode installed icons.
-  content::BrowserThread::GetBlockingPool()->FlushForTesting();
-  base::RunLoop().RunUntilIdle();
-
   // Validate loaded image.
   EXPECT_EQ(1 + scale_factors.size(), delegate.update_image_cnt());
   EXPECT_EQ(app_id, delegate.app_id());
   ValidateIcon(delegate.image());
 }
 
 TEST_F(ArcAppModelBuilderTest, AppLauncher) {
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile());
   ASSERT_NE(nullptr, prefs);
 
@@ skipping 33 matching lines @@
   EXPECT_FALSE(prefs->HasObserver(&launcher1));
   EXPECT_TRUE(prefs->HasObserver(&launcher3));
 
   ArcAppLauncher launcher2(profile(), id2, true);
   EXPECT_TRUE(launcher2.app_launched());
   app_instance()->WaitForIncomingMethodCall();
   EXPECT_FALSE(prefs->HasObserver(&launcher2));
   ASSERT_EQ(2u, app_instance()->launch_requests().size());
   EXPECT_TRUE(app_instance()->launch_requests()[1]->IsForApp(app2));
 }