Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(6260)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: chrome/browser/ui/app_list/arc/arc_app_unittest.cc

Issue 2150583002: Fix ArcAppModelBuilderTest (Closed) Base URL: https://chromium.googlesource.com/a/chromium/src.git@master
Patch Set: Also fix other tests in the suite Created 4 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« chrome/browser/chromeos/profiles/profile_helper.h ('K') | « chrome/browser/ui/app_list/arc/arc_app_test.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/ui/app_list/arc/arc_app_unittest.cc
diff --git a/chrome/browser/ui/app_list/arc/arc_app_unittest.cc b/chrome/browser/ui/app_list/arc/arc_app_unittest.cc
index 508bac33da23ea4d6d563e321a85e67e41768423..bb18bff45cf8ef5558bcb704c8e93f069d49e566 100644
--- a/chrome/browser/ui/app_list/arc/arc_app_unittest.cc
+++ b/chrome/browser/ui/app_list/arc/arc_app_unittest.cc
@@ -66,6 +66,18 @@ class FakeAppIconLoaderDelegate : public AppIconLoaderDelegate {
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 {
@@ -80,6 +92,9 @@ class ArcAppModelBuilderTest : public AppListTestBase {
AppListTestBase::SetUp();
arc_test_.SetUp(profile_.get());
CreateBuilder();
+
+ // Validating decoded content does not fit well for unit tests.
+ ArcAppIcon::DisableSafeDecodingForTesting();
}
void TearDown() override {
@@ -547,9 +562,9 @@ TEST_F(ArcAppModelBuilderTest, LaunchApps) {
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();
@@ -587,9 +602,9 @@ TEST_F(ArcAppModelBuilderTest, LaunchShortcuts) {
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();
@@ -622,12 +637,14 @@ TEST_F(ArcAppModelBuilderTest, RequestIcons) {
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
@@ -662,8 +679,7 @@ TEST_F(ArcAppModelBuilderTest, RequestIcons) {
}
}
-// 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));
@@ -686,15 +702,12 @@ TEST_F(ArcAppModelBuilderTest, DISABLED_RequestShortcutIcons) {
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 =
@@ -719,7 +732,7 @@ TEST_F(ArcAppModelBuilderTest, DISABLED_RequestShortcutIcons) {
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));
}
}
@@ -740,37 +753,26 @@ TEST_F(ArcAppModelBuilderTest, InstallIcon) {
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);
}
@@ -792,24 +794,27 @@ TEST_F(ArcAppModelBuilderTest, RemoveAppCleanUpFolder) {
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 co
+ EXPECT_FALSE(base::PathExists(app_path));
}
TEST_F(ArcAppModelBuilderTest, LastLaunchTime) {
@@ -857,12 +862,12 @@ TEST_F(ArcAppModelBuilderTest, LastLaunchTime) {
}
TEST_F(ArcAppModelBuilderTest, IconLoader) {
Luis Héctor Chávez 2016/07/14 22:44:32 This test is unfortunately still flaky (5-10%) eve
xiyuan 2016/07/14 22:59:32 Yes, we should disable it until resolved so that s
- // 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>(
@@ -882,20 +887,17 @@ TEST_F(ArcAppModelBuilderTest, IconLoader) {
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
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());
« chrome/browser/chromeos/profiles/profile_helper.h ('K') | « chrome/browser/ui/app_list/arc/arc_app_test.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698