Skip to content

Commit

Permalink
CodeHealth: Fix SWA Mgr unittest to not require web app sync_bridge
Browse files Browse the repository at this point in the history 
Previously this test directly inserted WebApps into the web app
registrar, which is not exercising the real code that sets up SWAs.
Changed this to use the higher-level testing function for installing a
web app. Also solves a DEPS issue because sync_bridge is not allowed
to be included by this code (it was also previously missing an include
for web_app_sync_bridge.h).

Note this changes the test expectations, but it seems like it should
have been expected to uninstall App2 originally (and in fact this was
the case in a previous iteration of this code in crrev.com/c/4587571).

POC for this change: dmurph@ if glenrob@ is unavailable.

Change-Id: Id72b5236118a8c82558a0754056ab5e15c395e16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5573227
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Auto-Submit: Glen Robertson <glenrob@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1306685}
  • Loading branch information
@pull
Glen Robertson authored and pull[bot] committed May 28, 2024
1 parent e495d9f commit 53c7cc2
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 43 deletions.
2 changes: 1 addition & 1 deletion chrome/browser/ash/system_web_apps/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ source_set("unit_tests") {
"//chrome/common:chrome_features",
"//chrome/test:test_support",
"//chromeos/components/kiosk:test_support",
"//components/webapps/browser:constants",
"//components/webapps/browser",
"//content/test:test_support",
"//ui/base/idle",
"//ui/base/idle:test_support",
Expand Down
66 changes: 24 additions & 42 deletions chrome/browser/ash/system_web_apps/system_web_app_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,18 @@
#include "chrome/test/base/chrome_render_view_host_test_harness.h"
#include "chromeos/components/kiosk/kiosk_test_utils.h"
#include "components/webapps/browser/install_result_code.h"
#include "components/webapps/browser/installable/installable_metrics.h"
#include "content/public/test/test_utils.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "ui/base/idle/idle.h"
#include "ui/base/idle/scoped_set_idle_state.h"
#include "url/gurl.h"

namespace ash {

namespace {
using testing::ElementsAre;

const char kSettingsAppInternalName[] = "OSSettings";
const char kCameraAppInternalName[] = "Camera";

Expand All @@ -54,6 +58,9 @@ GURL AppUrl1() {
GURL AppUrl2() {
return GURL(content::GetWebUIURL("system-app2"));
}
GURL AppUrl3() {
return GURL(content::GetWebUIURL("system-app3"));
}

std::unique_ptr<web_app::WebAppInstallInfo> GetWebAppInstallInfo(
const GURL& url) {
Expand All @@ -76,12 +83,6 @@ web_app::WebAppInstallInfoFactory GetApp2WebAppInfoFactory() {
return factory;
}

struct SystemAppData {
GURL url;
GURL icon_url;
web_app::ExternalInstallSource source;
};

class SystemWebAppWaiter {
public:
explicit SystemWebAppWaiter(SystemWebAppManager* system_web_app_manager) {
Expand Down Expand Up @@ -183,23 +184,6 @@ class SystemWebAppManagerTest : public ChromeRenderViewHostTestHarness {
web_app::GenerateAppId(/*manifest_id=*/std::nullopt, install_url));
}

void InitRegistrarWithSystemApps(
const std::vector<SystemAppData>& system_app_data_list) {
DCHECK(provider().registrar_unsafe().is_empty());
DCHECK(!system_app_data_list.empty());

for (const SystemAppData& data : system_app_data_list) {
std::unique_ptr<web_app::WebApp> web_app = web_app::test::CreateWebApp(
data.url, web_app::WebAppManagement::Type::kSystem);
const webapps::AppId app_id = web_app->app_id();
{
web_app::ScopedRegistryUpdate update =
provider().sync_bridge_unsafe().BeginUpdate();
update->CreateApp(std::move(web_app));
}
}
}

void StartAndWaitForAppsToSynchronize() {
SystemWebAppWaiter waiter(&system_web_app_manager());
system_web_app_manager().Start();
Expand All @@ -225,14 +209,15 @@ class SystemWebAppManagerTest : public ChromeRenderViewHostTestHarness {
TEST_F(SystemWebAppManagerTest, UninstallAppInstalledInPreviousSession) {
// Simulate System Apps and a regular app that were installed in the
// previous session.
InitRegistrarWithSystemApps(
{{AppUrl1(), GURL(content::GetWebUIURL("system-app1/app.ico")),
web_app::ExternalInstallSource::kSystemInstalled},
{AppUrl2(), GURL(content::GetWebUIURL("system-app2/app.ico")),
web_app::ExternalInstallSource::kSystemInstalled},
{GURL(content::GetWebUIURL("system-app3")),
GURL(content::GetWebUIURL("system-app3/app.ico")),
web_app::ExternalInstallSource::kInternalDefault}});
web_app::test::InstallDummyWebApp(
profile(), "App1", AppUrl1(),
webapps::WebappInstallSource::SYSTEM_DEFAULT);
web_app::test::InstallDummyWebApp(
profile(), "App2", AppUrl2(),
webapps::WebappInstallSource::SYSTEM_DEFAULT);
web_app::test::InstallDummyWebApp(
profile(), "App3", AppUrl3(),
webapps::WebappInstallSource::INTERNAL_DEFAULT);

SystemWebAppDelegateMap system_apps;
system_apps.emplace(SystemWebAppType::SETTINGS,
Expand All @@ -258,12 +243,11 @@ TEST_F(SystemWebAppManagerTest, UninstallAppInstalledInPreviousSession) {
options.only_use_app_info_factory = true;
options.system_app_type = SystemWebAppType::SETTINGS;
options.app_info_factory = GetApp1WebAppInfoFactory();
std::vector<web_app::ExternalInstallOptions> expected_install_options_list = {
options};
EXPECT_EQ(externally_managed_app_manager().install_requests(),
expected_install_options_list);
EXPECT_EQ(std::vector<GURL>({}),
externally_managed_app_manager().uninstall_requests());

EXPECT_THAT(externally_managed_app_manager().install_requests(),
ElementsAre(options));
EXPECT_THAT(externally_managed_app_manager().uninstall_requests(),
ElementsAre(AppUrl2()));
}

TEST_F(SystemWebAppManagerTest, AlwaysUpdate) {
Expand Down Expand Up @@ -367,15 +351,13 @@ TEST_F(SystemWebAppManagerTest, UpdateOnVersionChange) {

// Changing the install URL of a system app propagates even without a
// version change.
const GURL kAppUrl3(content::GetWebUIURL("system-app3"));

{
SystemWebAppDelegateMap system_apps;
system_apps.emplace(
SystemWebAppType::SETTINGS,
std::make_unique<UnittestingSystemAppDelegate>(
SystemWebAppType::SETTINGS, kSettingsAppInternalName, kAppUrl3,
base::BindRepeating(&GetWebAppInstallInfo, kAppUrl3)));
SystemWebAppType::SETTINGS, kSettingsAppInternalName, AppUrl3(),
base::BindRepeating(&GetWebAppInstallInfo, AppUrl3())));

system_apps.emplace(SystemWebAppType::CAMERA,
std::make_unique<UnittestingSystemAppDelegate>(
Expand All @@ -391,7 +373,7 @@ TEST_F(SystemWebAppManagerTest, UpdateOnVersionChange) {
EXPECT_FALSE(install_requests[6].force_reinstall);
EXPECT_FALSE(IsInstalled(AppUrl1()));
EXPECT_TRUE(IsInstalled(AppUrl2()));
EXPECT_TRUE(IsInstalled(kAppUrl3));
EXPECT_TRUE(IsInstalled(AppUrl3()));
}

TEST_F(SystemWebAppManagerTest, UpdateOnVersionChangeEvenIfIconsBroken) {
Expand Down

0 comments on commit 53c7cc2

Please sign in to comment.