Skip to content

Commit

Permalink
[BugBash] Modernize selected tests in chrome/browser/app_service
Browse files Browse the repository at this point in the history 
This CL rewrites some RunLoop-based tests using TestFuture wherever
possible.

Bug: 1412072
Change-Id: I0a5d8127432c7207448b094e6dc44c63b07fa7e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4214691
Reviewed-by: Maggie Cai <mxcai@chromium.org>
Commit-Queue: Andrew Rayskiy <greengrape@google.com>
Cr-Commit-Position: refs/heads/main@{#1100362}
  • Loading branch information
Andrew Rayskiy authored and Chromium LUCI CQ committed Feb 2, 2023
1 parent 4246d2e commit fcc7c88
Show file tree
Hide file tree
Showing 8 changed files with 126 additions and 248 deletions.
168 changes: 49 additions & 119 deletions chrome/browser/apps/app_service/app_icon/app_icon_factory_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,33 +64,21 @@ class AppIconFactoryTest : public testing::Test {

void SetUp() override { ASSERT_TRUE(tmp_dir_.CreateUniqueTempDir()); }

void RunLoadIconFromFileWithFallback(apps::IconValuePtr fallback_response,
bool* callback_called,
bool* fallback_called,
bool RunLoadIconFromFileWithFallback(apps::IconValuePtr fallback_response,
apps::IconValuePtr* result) {
*callback_called = false;
*fallback_called = false;
bool fallback_called = false;

base::test::TestFuture<apps::IconValuePtr> success_future;
apps::LoadIconFromFileWithFallback(
apps::IconType::kUncompressed, 200, GetPath(), apps::IconEffects::kNone,
base::BindOnce(
[](bool* called, apps::IconValuePtr* result, base::OnceClosure quit,
apps::IconValuePtr icon) {
*called = true;
*result = std::move(icon);
std::move(quit).Run();
},
base::Unretained(callback_called), base::Unretained(result),
run_loop_.QuitClosure()),
base::BindOnce(
[](bool* called, apps::IconValuePtr response,
apps::LoadIconCallback callback) {
*called = true;
std::move(callback).Run(std::move(response));
},
base::Unretained(fallback_called), std::move(fallback_response)));

run_loop_.Run();
success_future.GetCallback(),
base::BindLambdaForTesting([&](apps::LoadIconCallback callback) {
fallback_called = true;
std::move(callback).Run(std::move(fallback_response));
}));

*result = success_future.Take();
return fallback_called;
}

std::string GetPngData(const std::string file_name) {
Expand All @@ -113,18 +101,10 @@ class AppIconFactoryTest : public testing::Test {
apps::IconType icon_type,
apps::IconEffects icon_effects,
apps::IconValuePtr& output_icon) {
apps::LoadIconFromCompressedData(
icon_type, kSizeInDip, icon_effects, png_data_as_string,
base::BindOnce(
[](apps::IconValuePtr* result,
base::OnceClosure load_app_icon_callback,
apps::IconValuePtr icon) {
*result = std::move(icon);
std::move(load_app_icon_callback).Run();
},
&output_icon, run_loop_.QuitClosure()));
run_loop_.Run();

base::test::TestFuture<apps::IconValuePtr> future;
apps::LoadIconFromCompressedData(icon_type, kSizeInDip, icon_effects,
png_data_as_string, future.GetCallback());
output_icon = future.Take();
ASSERT_TRUE(output_icon);
ASSERT_EQ(icon_type, output_icon->icon_type);
ASSERT_FALSE(output_icon->is_placeholder_icon);
Expand All @@ -151,16 +131,12 @@ class AppIconFactoryTest : public testing::Test {
#if BUILDFLAG(IS_CHROMEOS_ASH)
apps::IconValuePtr RunLoadIconFromResource(apps::IconType icon_type,
apps::IconEffects icon_effects) {
bool is_placeholder_icon = false;
apps::IconValuePtr icon_value;
apps::LoadIconFromResource(
icon_type, kSizeInDip, IDR_LOGO_CROSTINI_DEFAULT, is_placeholder_icon,
icon_effects, base::BindLambdaForTesting([&](apps::IconValuePtr icon) {
icon_value = std::move(icon);
run_loop_.Quit();
}));
run_loop_.Run();
return icon_value;
base::test::TestFuture<apps::IconValuePtr> future;
apps::LoadIconFromResource(icon_type, kSizeInDip, IDR_LOGO_CROSTINI_DEFAULT,
/*is_placeholder_icon=*/false, icon_effects,
future.GetCallback());
auto icon = future.Take();
return icon;
}

void GenerateCrostiniPenguinIcon(gfx::ImageSkia& output_image_skia) {
Expand Down Expand Up @@ -195,7 +171,6 @@ class AppIconFactoryTest : public testing::Test {
protected:
content::BrowserTaskEnvironment task_env_{};
base::ScopedTempDir tmp_dir_{};
base::RunLoop run_loop_{};
};

TEST_F(AppIconFactoryTest, LoadFromFileSuccess) {
Expand All @@ -206,10 +181,8 @@ TEST_F(AppIconFactoryTest, LoadFromFileSuccess) {

auto fallback_response = std::make_unique<apps::IconValue>();
auto result = std::make_unique<apps::IconValue>();
bool callback_called, fallback_called;
RunLoadIconFromFileWithFallback(std::move(fallback_response),
&callback_called, &fallback_called, &result);
EXPECT_TRUE(callback_called);
bool fallback_called =
RunLoadIconFromFileWithFallback(std::move(fallback_response), &result);
EXPECT_FALSE(fallback_called);
ASSERT_TRUE(result);

Expand All @@ -227,10 +200,8 @@ TEST_F(AppIconFactoryTest, LoadFromFileFallback) {
fallback_response->uncompressed = expect_image;

auto result = std::make_unique<apps::IconValue>();
bool callback_called, fallback_called;
RunLoadIconFromFileWithFallback(std::move(fallback_response),
&callback_called, &fallback_called, &result);
EXPECT_TRUE(callback_called);
bool fallback_called =
RunLoadIconFromFileWithFallback(std::move(fallback_response), &result);
EXPECT_TRUE(fallback_called);
ASSERT_TRUE(result);
EXPECT_TRUE(result->uncompressed.BackedBySameObjectAs(expect_image));
Expand All @@ -239,34 +210,27 @@ TEST_F(AppIconFactoryTest, LoadFromFileFallback) {
TEST_F(AppIconFactoryTest, LoadFromFileFallbackFailure) {
auto fallback_response = std::make_unique<apps::IconValue>();
auto result = std::make_unique<apps::IconValue>();
bool callback_called, fallback_called;
RunLoadIconFromFileWithFallback(std::move(fallback_response),
&callback_called, &fallback_called, &result);
EXPECT_TRUE(callback_called);
bool fallback_called =
RunLoadIconFromFileWithFallback(std::move(fallback_response), &result);
EXPECT_TRUE(fallback_called);
ASSERT_TRUE(result);
}

TEST_F(AppIconFactoryTest, LoadFromFileFallbackDoesNotReturn) {
auto result = std::make_unique<apps::IconValue>();
bool callback_called = false, fallback_called = false;
base::test::TestFuture<apps::IconValuePtr> success_future;

bool fallback_called = false;
apps::LoadIconFromFileWithFallback(
apps::IconType::kUncompressed, 200, GetPath(), apps::IconEffects::kNone,
base::BindLambdaForTesting([&](apps::IconValuePtr icon) {
callback_called = true;
result = std::move(icon);
run_loop_.Quit();
}),
base::BindLambdaForTesting([&](apps::LoadIconCallback callback) {
fallback_called = true;
apps::IconType::kUncompressed, /*size_hint_in_dip=*/200, GetPath(),
apps::IconEffects::kNone, success_future.GetCallback(),
base::BindLambdaForTesting([&](apps::LoadIconCallback) {
// Drop the callback here, like a buggy fallback might.
fallback_called = true;
}));

run_loop_.Run();

EXPECT_TRUE(callback_called);
EXPECT_TRUE(success_future.Wait());
EXPECT_TRUE(fallback_called);
auto result = success_future.Take();
ASSERT_TRUE(result);
}

Expand Down Expand Up @@ -334,21 +298,11 @@ TEST_F(AppIconFactoryTest, ArcNonAdaptiveIconToImageSkia) {
png_data_as_string.end()),
std::vector<uint8_t>(), std::vector<uint8_t>());

bool callback_called = false;
apps::ArcRawIconPngDataToImageSkia(
std::move(icon), 100,
base::BindOnce(
[](bool* called, base::OnceClosure quit,
const gfx::ImageSkia& image) {
if (!image.isNull()) {
*called = true;
}
std::move(quit).Run();
},
base::Unretained(&callback_called), run_loop_.QuitClosure()));

run_loop_.Run();
EXPECT_TRUE(callback_called);
base::test::TestFuture<const gfx::ImageSkia&> future;
apps::ArcRawIconPngDataToImageSkia(std::move(icon), 100,
future.GetCallback());
auto image = future.Take();
EXPECT_TRUE(!image.isNull());
}

TEST_F(AppIconFactoryTest, ArcAdaptiveIconToImageSkia) {
Expand All @@ -362,21 +316,11 @@ TEST_F(AppIconFactoryTest, ArcAdaptiveIconToImageSkia) {
std::vector<uint8_t>(png_data_as_string.begin(),
png_data_as_string.end()));

bool callback_called = false;
apps::ArcRawIconPngDataToImageSkia(
std::move(icon), 100,
base::BindOnce(
[](bool* called, base::OnceClosure quit,
const gfx::ImageSkia& image) {
if (!image.isNull()) {
*called = true;
}
std::move(quit).Run();
},
base::Unretained(&callback_called), run_loop_.QuitClosure()));

run_loop_.Run();
EXPECT_TRUE(callback_called);
base::test::TestFuture<const gfx::ImageSkia&> future;
apps::ArcRawIconPngDataToImageSkia(std::move(icon), 100,
future.GetCallback());
auto image = future.Take();
EXPECT_TRUE(!image.isNull());
}

TEST_F(AppIconFactoryTest, ArcActivityIconsToImageSkias) {
Expand Down Expand Up @@ -413,24 +357,10 @@ TEST_F(AppIconFactoryTest, ArcActivityIconsToImageSkias) {
std::vector<uint8_t>(png_data_as_string.begin(),
png_data_as_string.end()))));

std::vector<gfx::ImageSkia> result;
bool callback_called = false;
apps::ArcActivityIconsToImageSkias(
icons, base::BindOnce(
[](bool* called, std::vector<gfx::ImageSkia>* result,
base::OnceClosure quit,
const std::vector<gfx::ImageSkia>& images) {
*called = true;
for (auto image : images) {
result->emplace_back(image);
}
std::move(quit).Run();
},
base::Unretained(&callback_called), base::Unretained(&result),
run_loop_.QuitClosure()));
run_loop_.Run();

EXPECT_TRUE(callback_called);
base::test::TestFuture<const std::vector<gfx::ImageSkia>&> future;
apps::ArcActivityIconsToImageSkias(icons, future.GetCallback());

auto result = future.Take();
EXPECT_EQ(4U, result.size());
EXPECT_TRUE(result[0].isNull());
EXPECT_FALSE(result[1].isNull());
Expand Down
91 changes: 31 additions & 60 deletions chrome/browser/apps/app_service/app_icon/web_app_icon_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,11 @@ class WebAppIconFactoryTest : public ChromeRenderViewHostTestHarness {
}
}

base::RunLoop run_loop;
base::test::TestFuture<bool> future;
icon_manager_->WriteData(app_id, std::move(icon_bitmaps), {}, {},
base::BindLambdaForTesting([&](bool success) {
EXPECT_TRUE(success);
run_loop.Quit();
}));
run_loop.Run();
future.GetCallback());
bool success = future.Get();
EXPECT_TRUE(success);
}

void GenerateWebAppIcon(const std::string& app_id,
Expand All @@ -139,34 +137,23 @@ class WebAppIconFactoryTest : public ChromeRenderViewHostTestHarness {
apps::ScaleToSize scale_to_size_in_px,
gfx::ImageSkia& output_image_skia,
bool skip_icon_effects = false) {
base::RunLoop run_loop;
icon_manager().ReadIcons(
app_id, purpose, sizes_px,
base::BindOnce(
[](gfx::ImageSkia* image_skia, int size_in_dip,
apps::ScaleToSize scale_to_size_in_px,
base::OnceClosure load_app_icon_callback,
std::map<SquareSizePx, SkBitmap> icon_bitmaps) {
for (auto it : scale_to_size_in_px) {
int icon_size_in_px =
gfx::ScaleToFlooredSize(gfx::Size(size_in_dip, size_in_dip),
it.first)
.width();

SkBitmap bitmap = icon_bitmaps[it.second];
if (bitmap.width() != icon_size_in_px) {
bitmap = skia::ImageOperations::Resize(
bitmap, skia::ImageOperations::RESIZE_LANCZOS3,
icon_size_in_px, icon_size_in_px);
}
image_skia->AddRepresentation(
gfx::ImageSkiaRep(bitmap, it.first));
}
std::move(load_app_icon_callback).Run();
},
&output_image_skia, kSizeInDip, scale_to_size_in_px,
run_loop.QuitClosure()));
run_loop.Run();
base::test::TestFuture<std::map<SquareSizePx, SkBitmap>> future;
icon_manager().ReadIcons(app_id, purpose, sizes_px, future.GetCallback());
auto icon_bitmaps = future.Take();

for (auto [scale, size_in_px] : scale_to_size_in_px) {
int icon_size_in_px =
gfx::ScaleToFlooredSize(gfx::Size(kSizeInDip, kSizeInDip), scale)
.width();

SkBitmap bitmap = icon_bitmaps[size_in_px];
if (bitmap.width() != icon_size_in_px) {
bitmap = skia::ImageOperations::Resize(
bitmap, skia::ImageOperations::RESIZE_LANCZOS3, icon_size_in_px,
icon_size_in_px);
}
output_image_skia.AddRepresentation(gfx::ImageSkiaRep(bitmap, scale));
}

if (!skip_icon_effects) {
extensions::ChromeAppIcon::ResizeFunction resize_function;
Expand Down Expand Up @@ -240,38 +227,22 @@ class WebAppIconFactoryTest : public ChromeRenderViewHostTestHarness {
void LoadIconFromWebApp(const std::string& app_id,
apps::IconEffects icon_effects,
gfx::ImageSkia& output_image_skia) {
base::RunLoop run_loop;

auto icon_type = apps::IconType::kUncompressed;
icon_type = apps::IconType::kStandard;

apps::LoadIconFromWebApp(
profile(), icon_type, kSizeInDip, app_id, icon_effects,
base::BindOnce(
[](gfx::ImageSkia* image, base::OnceClosure load_app_icon_callback,
apps::IconValuePtr icon) {
*image = icon->uncompressed;
std::move(load_app_icon_callback).Run();
},
&output_image_skia, run_loop.QuitClosure()));
run_loop.Run();

base::test::TestFuture<apps::IconValuePtr> future;
apps::LoadIconFromWebApp(profile(), apps::IconType::kStandard, kSizeInDip,
app_id, icon_effects, future.GetCallback());
auto icon = future.Take();
output_image_skia = icon->uncompressed;
EnsureRepresentationsLoaded(output_image_skia);
}

apps::IconValuePtr LoadCompressedIconBlockingFromWebApp(
const std::string& app_id,
apps::IconEffects icon_effects) {
base::RunLoop run_loop;
apps::IconValuePtr icon_value;
apps::LoadIconFromWebApp(
profile(), apps::IconType::kCompressed, kSizeInDip, app_id,
icon_effects, base::BindLambdaForTesting([&](apps::IconValuePtr icon) {
icon_value = std::move(icon);
run_loop.Quit();
}));
run_loop.Run();
return icon_value;
base::test::TestFuture<apps::IconValuePtr> future;
apps::LoadIconFromWebApp(profile(), apps::IconType::kCompressed, kSizeInDip,
app_id, icon_effects, future.GetCallback());
auto icon = future.Take();
return icon;
}

#if BUILDFLAG(IS_CHROMEOS_ASH)
Expand Down
11 changes: 6 additions & 5 deletions chrome/browser/apps/app_service/app_service_proxy_lacros_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,15 @@ TEST(AppServiceProxyLacrosTest, Launch) {
proxy.SetCrosapiAppServiceProxyForTesting(&mock_proxy);

base::RunLoop waiter;
LaunchResult result;
proxy.LaunchAppWithUrl(
kAppId, event_flag, GURL(kUrl), launch_source,
std::make_unique<WindowInfo>(display::kDefaultDisplayId),
base::BindLambdaForTesting([&](LaunchResult&& result_arg) {
EXPECT_EQ(result_arg.state, LaunchResult::State::SUCCESS);
waiter.Quit();
}));
base::BindOnce(
[](base::OnceClosure callback, LaunchResult&& result_arg) {
EXPECT_EQ(LaunchResult::State::SUCCESS, result_arg.state);
std::move(callback).Run();
},
waiter.QuitClosure()));
mock_proxy.Wait();
waiter.Run();

Expand Down

0 comments on commit fcc7c88

Please sign in to comment.