Skip to content

Commit

Permalink
Updater: Parameterize IntegrationTest.
Browse files Browse the repository at this point in the history 
Bug: 1096654
Change-Id: Ia86a3a4f1355812cd32d89bcc34f5a220b5673ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2642930
Commit-Queue: Mila Green <milagreen@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#860393}
  • Loading branch information
Mila Green authored and Chromium LUCI CQ committed Mar 5, 2021
1 parent 7e92705 commit f6d82f8
Show file tree
Hide file tree
Showing 74 changed files with 1,876 additions and 851 deletions.
59 changes: 51 additions & 8 deletions chrome/updater/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,13 @@ if (is_win || is_mac) {
"policy_manager.h",
"registration_data.cc",
"registration_data.h",
"service_scope.h",
"splash_screen.h",
"unzipper.cc",
"unzipper.h",
"update_service.cc",
"update_service.h",
"update_service_internal.h",
"updater_scope.h",
"util.cc",
"util.h",
]
Expand All @@ -89,6 +89,17 @@ if (is_win || is_mac) {
"//third_party/zlib/google:zip",
"//url",
]

if (is_mac) {
sources += [
"mac/mac_util.h",
"mac/mac_util.mm",
]

deps += [ "//chrome/common/mac:launchd" ]

frameworks = [ "Foundation.framework" ]
}
}

# Use this source set for code which has platform-specific modules.
Expand Down Expand Up @@ -224,7 +235,6 @@ if (is_win || is_mac) {
"//chrome/updater/mac:installer_sources",
"//chrome/updater/mac:network_fetcher_sources",
"//chrome/updater/mac:updater_setup_sources",
"//chrome/updater/mac:util",
"//chrome/updater/mac:xpc_names",
]
}
Expand Down Expand Up @@ -326,10 +336,8 @@ if (is_win || is_mac) {
]
}

# These tests are run serially since they mutate system state.
test("updater_tests") {
source_set("updater_test_sources") {
testonly = true

sources = [
"app/app_server_unittest.cc",
"enum_traits_unittest.cc",
Expand All @@ -340,15 +348,14 @@ if (is_win || is_mac) {
"policy_manager_unittest.cc",
"policy_service_unittest.cc",
"prefs_unittest.cc",
"run_all_unittests.cc",
"service_scope_unittest.cc",
"tag_unittest.cc",
"test/integration_tests.cc",
"test/integration_tests.h",
"test/server.cc",
"test/server.h",
"unittest_util_unittest.cc",
"update_service_unittest.cc",
"updater_scope_unittest.cc",
"updater_unittest.cc",
]

Expand Down Expand Up @@ -407,7 +414,6 @@ if (is_win || is_mac) {
"//chrome/updater/mac:updater_bundle",
"//chrome/updater/mac:updater_setup_tests",
"//chrome/updater/mac:updater_tests",
"//chrome/updater/mac:util",
"//chrome/updater/mac:xpc_names",
"//third_party/ocmock",
]
Expand All @@ -419,6 +425,43 @@ if (is_win || is_mac) {
}
}

# These tests are run serially since they mutate system state.
test("updater_tests") {
testonly = true

sources = [ "run_all_unittests.cc" ]

deps = [
":updater_test_sources",
"//base/test:test_support",
"//chrome/common:constants",
"//testing/gtest",
]

data_deps = [ ":updater_integration_tests_helper" ]

if (is_mac) {
data_deps += [
"//chrome/updater/mac:updater_bundle",
"//chrome/updater/test/test_app",
]
}
}

executable("updater_integration_tests_helper") {
testonly = true

sources = [ "test/integration_tests_helper.cc" ]

deps = [
":base",
":lib",
":updater_test_sources",
"//base",
"//url",
]
}

process_version("branding_header_py") {
process_only = true

Expand Down
18 changes: 9 additions & 9 deletions chrome/updater/activity.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,30 +14,30 @@
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "chrome/updater/activity_impl.h"
#include "chrome/updater/updater_scope.h"

namespace updater {
namespace {
constexpr int kDaysUnknown = -2;
}

ActivityDataService::ActivityDataService(bool is_machine)
: is_machine_(is_machine) {}
ActivityDataService::ActivityDataService(UpdaterScope scope) : scope_(scope) {}

void ActivityDataService::GetActiveBits(
const std::vector<std::string>& ids,
base::OnceCallback<void(const std::set<std::string>&)> callback) const {
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE, {base::MayBlock()},
base::BindOnce(
[](const std::vector<std::string>& ids, bool is_machine) {
[](UpdaterScope scope, const std::vector<std::string>& ids) {
std::set<std::string> result;
for (const auto& id : ids) {
if (GetActiveBit(id, is_machine))
if (GetActiveBit(scope, id))
result.insert(id);
}
return result;
},
ids, is_machine_),
scope_, ids),
std::move(callback));
}

Expand All @@ -47,16 +47,16 @@ void ActivityDataService::GetAndClearActiveBits(
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE, {base::MayBlock()},
base::BindOnce(
[](const std::vector<std::string>& ids, bool is_machine) {
[](UpdaterScope scope, const std::vector<std::string>& ids) {
std::set<std::string> result;
for (const auto& id : ids) {
if (GetActiveBit(id, is_machine))
if (GetActiveBit(scope, id))
result.insert(id);
ClearActiveBit(id, is_machine);
ClearActiveBit(scope, id);
}
return result;
},
ids, is_machine_),
scope_, ids),
std::move(callback));
}

Expand Down
5 changes: 3 additions & 2 deletions chrome/updater/activity.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@
#include <vector>

#include "base/callback_forward.h"
#include "chrome/updater/updater_scope.h"
#include "components/update_client/activity_data_service.h"

namespace updater {

class ActivityDataService final : public update_client::ActivityDataService {
public:
explicit ActivityDataService(bool is_machine);
explicit ActivityDataService(UpdaterScope scope);
ActivityDataService(const ActivityDataService&) = delete;
ActivityDataService& operator=(const ActivityDataService&) = delete;
~ActivityDataService() override = default;
Expand All @@ -35,7 +36,7 @@ class ActivityDataService final : public update_client::ActivityDataService {
int GetDaysSinceLastRollCall(const std::string& id) const override;

private:
bool is_machine_;
UpdaterScope scope_;
};

} // namespace updater
Expand Down
6 changes: 4 additions & 2 deletions chrome/updater/activity_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@

#include <string>

#include "chrome/updater/updater_scope.h"

namespace updater {

bool GetActiveBit(const std::string& id, bool is_machine_);
bool GetActiveBit(UpdaterScope scope, const std::string& id);

void ClearActiveBit(const std::string& id, bool is_machine_);
void ClearActiveBit(UpdaterScope scope, const std::string& id);

} // namespace updater

Expand Down
28 changes: 10 additions & 18 deletions chrome/updater/activity_impl_mac.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@

#include "base/files/file_util.h"
#include "base/logging.h"
#include "chrome/updater/mac/mac_util.h"
#include "chrome/updater/updater_branding.h"
#include "chrome/updater/updater_scope.h"

namespace updater {
namespace {

base::FilePath GetActiveFile(const std::string& id) {
base::FilePath GetActiveFile(UpdaterScope scope, const std::string& id) {
// TODO(crbug.com/1096654): Add support for UpdaterScope::kSystem.
return base::GetHomeDir()
.AppendASCII("Library")
.AppendASCII(COMPANY_SHORTNAME_STRING)
Expand All @@ -24,25 +27,14 @@ base::FilePath GetActiveFile(const std::string& id) {

} // namespace

bool GetActiveBit(const std::string& id, bool is_machine_) {
if (is_machine_) {
// TODO(crbug.com/1096654): Add support for the machine case. Machine
// installs must look for values in each home dir.
return false;
} else {
base::FilePath path = GetActiveFile(id);
return base::PathExists(path) && base::PathIsWritable(path);
}
bool GetActiveBit(UpdaterScope scope, const std::string& id) {
const base::FilePath path = GetActiveFile(scope, id);
return base::PathExists(path) && base::PathIsWritable(path);
}

void ClearActiveBit(const std::string& id, bool is_machine_) {
if (is_machine_) {
// TODO(crbug.com/1096654): Add support for the machine case. Machine
// installs must clear values in each home dir.
} else {
if (!base::DeleteFile(GetActiveFile(id)))
VLOG(2) << "Failed to clear activity bit at " << GetActiveFile(id);
}
void ClearActiveBit(UpdaterScope scope, const std::string& id) {
if (!base::DeleteFile(GetActiveFile(scope, id)))
VLOG(2) << "Failed to clear activity bit at " << GetActiveFile(scope, id);
}

} // namespace updater
65 changes: 36 additions & 29 deletions chrome/updater/activity_impl_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "base/strings/utf_string_conversions.h"
#include "base/win/registry.h"
#include "base/win/windows_types.h"
#include "chrome/updater/updater_scope.h"
#include "chrome/updater/win/constants.h"

namespace updater {
Expand All @@ -24,41 +25,47 @@ std::wstring GetAppClientStateKey(const std::string& id) {

} // namespace

bool GetActiveBit(const std::string& id, bool is_machine_) {
if (is_machine_) {
// TODO(crbug.com/1096654): Add support for the machine case. Machine
// installs must look for values under HKLM and under every HKU\<sid>.
return false;
} else {
// TODO(crbug/1159498): Standardize registry access.
base::win::RegKey key;
if (key.Open(HKEY_CURRENT_USER, GetAppClientStateKey(id).c_str(),
KEY_READ | KEY_WOW64_32KEY) == ERROR_SUCCESS) {
std::wstring value;
if (key.ReadValue(kDidRun, &value) == ERROR_SUCCESS && value == L"1")
return true;
bool GetActiveBit(UpdaterScope scope, const std::string& id) {
switch (scope) {
case UpdaterScope::kUser: {
// TODO(crbug/1159498): Standardize registry access.
base::win::RegKey key;
if (key.Open(HKEY_CURRENT_USER, GetAppClientStateKey(id).c_str(),
KEY_READ | KEY_WOW64_32KEY) == ERROR_SUCCESS) {
std::wstring value;
if (key.ReadValue(kDidRun, &value) == ERROR_SUCCESS && value == L"1")
return true;
}
return false;
}
return false;
case UpdaterScope::kSystem:
// TODO(crbug.com/1096654): Add support for the machine case. Machine
// installs must look for values under HKLM and under every HKU\<sid>.
return false;
}
}

void ClearActiveBit(const std::string& id, bool is_machine_) {
if (is_machine_) {
// TODO(crbug.com/1096654): Add support for the machine case. Machine
// installs must clear values under HKLM and under every HKU\<sid>.
} else {
// TODO(crbug/1159498): Standardize registry access.
base::win::RegKey key;
if (key.Open(HKEY_CURRENT_USER, GetAppClientStateKey(id).c_str(),
KEY_WRITE | KEY_WOW64_32KEY) == ERROR_SUCCESS) {
const LONG result = key.WriteValue(kDidRun, L"0");
if (result) {
VLOG(2) << "Failed to clear HKCU activity key for " << id << ": "
<< result;
void ClearActiveBit(UpdaterScope scope, const std::string& id) {
switch (scope) {
case UpdaterScope::kUser: {
// TODO(crbug/1159498): Standardize registry access.
base::win::RegKey key;
if (key.Open(HKEY_CURRENT_USER, GetAppClientStateKey(id).c_str(),
KEY_WRITE | KEY_WOW64_32KEY) == ERROR_SUCCESS) {
const LONG result = key.WriteValue(kDidRun, L"0");
if (result) {
VLOG(2) << "Failed to clear HKCU activity key for " << id << ": "
<< result;
}
} else {
VLOG(2) << "Failed to open HKCU activity key with write for " << id;
}
} else {
VLOG(2) << "Failed to open HKCU activity key with write for " << id;
break;
}
case UpdaterScope::kSystem:
// TODO(crbug.com/1096654): Add support for the machine case. Machine
// installs must clear values under HKLM and under every HKU\<sid>.
break;
}
}

Expand Down
7 changes: 6 additions & 1 deletion chrome/updater/app/app.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@
#include "base/run_loop.h"
#include "base/task/thread_pool/thread_pool_instance.h"
#include "base/threading/thread_restrictions.h"
#include "chrome/updater/updater_scope.h"

namespace updater {

constexpr base::StringPiece App::kThreadPoolName;

App::App() = default;
App::App() : updater_scope_(GetProcessScope()) {}
App::~App() = default;

void App::InitializeThreadPool() {
Expand Down Expand Up @@ -50,4 +51,8 @@ void App::Shutdown(int exit_code) {
std::move(quit_).Run(exit_code);
}

UpdaterScope App::updater_scope() const {
return updater_scope_;
}

} // namespace updater
Loading

0 comments on commit f6d82f8

Please sign in to comment.