Skip to content

Commit

Permalink
Move StartRecordingUpstartOperations to FakeUpstartClient
Browse files Browse the repository at this point in the history 
This CL moves StartRecordingUpstartOperations and related helper
functions to FakeUpstartClient, as it was duplicated in multiple tests.

This CL also results in fixing the following bugs in
ArcAdbdMonitorBridgeTest:
- FakeUpstartClient wasn't shut down and re-initialized in each test
  cases, resulting in upstart_operations_ being accumulated across test
  cases.
- In TestStartArcVmAdbdFailure, injecting the failure of stopping
  arcvm-adbd Upstart job wasn't working as intended because the callback
  (stop_job_cb_ of FakeUpstartClient) was overwritten by
  StartRecordingUpstartOperations.

BUG=b:305608912
TEST=unit_tests --gtest_filter="ArcAdbdMonitorBridgeTest.*"
TEST=ash_components_unittests --gtest_filter="ArcUtilTest.*"
TEST=ash_components_unittests --gtest_filter="ArcVmClientAdapterTest.*"

Change-Id: I81be64999a98c522fa5f802c690caa68ac3d00d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4938258
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Commit-Queue: Momoko Hattori <momohatt@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1212571}
  • Loading branch information
@momohatt
momohatt authored and Chromium LUCI CQ committed Oct 20, 2023
1 parent 5f54243 commit a17e676
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 157 deletions.
44 changes: 10 additions & 34 deletions ash/components/arc/arc_util_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,34 +139,10 @@ class ArcUtilTest : public ash::AshTestBase {
}));
}

void StartRecordingUpstartOperations() {
auto* upstart_client = ash::FakeUpstartClient::Get();
upstart_client->set_start_job_cb(
base::BindLambdaForTesting([this](const std::string& job_name,
const std::vector<std::string>& env) {
upstart_operations_.emplace_back(job_name, true);
return ash::FakeUpstartClient::StartJobResult(true /* success */);
}));
upstart_client->set_stop_job_cb(
base::BindLambdaForTesting([this](const std::string& job_name,
const std::vector<std::string>& env) {
upstart_operations_.emplace_back(job_name, false);
return true;
}));
}

const std::vector<std::pair<std::string, bool>>& upstart_operations() const {
return upstart_operations_;
}

PrefService* profile_prefs() { return &profile_prefs_; }

private:
TestingPrefServiceSimple profile_prefs_;

// List of upstart operations recorded. When it's "start" the boolean is set
// to true.
std::vector<std::pair<std::string, bool>> upstart_operations_;
};

TEST_F(ArcUtilTest, IsArcAvailable_None) {
Expand Down Expand Up @@ -495,7 +471,7 @@ TEST_F(ArcUtilTest, ConfigureUpstartJobs_Success) {
JobDesc{"Job_2dC", UpstartOperation::JOB_START, {}},
};
bool result = false;
StartRecordingUpstartOperations();
ash::FakeUpstartClient::Get()->StartRecordingUpstartOperations();
ConfigureUpstartJobs(
jobs,
base::BindLambdaForTesting(
Expand All @@ -506,16 +482,16 @@ TEST_F(ArcUtilTest, ConfigureUpstartJobs_Success) {
task_environment()->RunUntilQuit();
EXPECT_TRUE(result);

auto ops = upstart_operations();
auto ops = ash::FakeUpstartClient::Get()->upstart_operations();
ASSERT_EQ(4u, ops.size());
EXPECT_EQ(ops[0].first, "Job_2dA");
EXPECT_FALSE(ops[0].second);
EXPECT_EQ(ops[1].first, "Job_2dB");
EXPECT_FALSE(ops[1].second);
EXPECT_EQ(ops[2].first, "Job_2dB");
EXPECT_TRUE(ops[2].second);
EXPECT_EQ(ops[3].first, "Job_2dC");
EXPECT_TRUE(ops[3].second);
EXPECT_EQ(ops[0].name, "Job_2dA");
EXPECT_EQ(ops[0].type, ash::FakeUpstartClient::UpstartOperationType::STOP);
EXPECT_EQ(ops[1].name, "Job_2dB");
EXPECT_EQ(ops[1].type, ash::FakeUpstartClient::UpstartOperationType::STOP);
EXPECT_EQ(ops[2].name, "Job_2dB");
EXPECT_EQ(ops[2].type, ash::FakeUpstartClient::UpstartOperationType::START);
EXPECT_EQ(ops[3].name, "Job_2dC");
EXPECT_EQ(ops[3].type, ash::FakeUpstartClient::UpstartOperationType::START);
}

TEST_F(ArcUtilTest, ConfigureUpstartJobs_StopFail) {
Expand Down
91 changes: 24 additions & 67 deletions ash/components/arc/session/arc_vm_client_adapter_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,15 +424,6 @@ class ArcVmClientAdapterTest : public testing::Test,
void ExpectFalse(bool result) { EXPECT_FALSE(result); }

protected:
enum class UpstartOperationType { START, STOP };

struct UpstartOperation {
std::string name;
std::vector<std::string> env;
UpstartOperationType type;
};
using UpstartOperations = std::vector<UpstartOperation>;

void SetAccountId(const AccountId& account_id) {
arc_service_manager_.set_account_id(account_id);
}
Expand Down Expand Up @@ -568,24 +559,6 @@ class ArcVmClientAdapterTest : public testing::Test,
}));
}

void StartRecordingUpstartOperations() {
auto* upstart_client = ash::FakeUpstartClient::Get();
upstart_client->set_start_job_cb(
base::BindLambdaForTesting([this](const std::string& job_name,
const std::vector<std::string>& env) {
upstart_operations_.push_back(
{job_name, env, UpstartOperationType::START});
return ash::FakeUpstartClient::StartJobResult(true /* success */);
}));
upstart_client->set_stop_job_cb(
base::BindLambdaForTesting([this](const std::string& job_name,
const std::vector<std::string>& env) {
upstart_operations_.push_back(
{job_name, env, UpstartOperationType::STOP});
return true;
}));
}

// We expect ConciergeClient::StopVm to have been called two times,
// once to clear a stale VM in StartMiniArc(), and another on this
// call to StopArcInstance().
Expand Down Expand Up @@ -627,9 +600,6 @@ class ArcVmClientAdapterTest : public testing::Test,
return is_system_shutdown_;
}
void reset_is_system_shutdown() { is_system_shutdown_ = absl::nullopt; }
const UpstartOperations& upstart_operations() const {
return upstart_operations_;
}
TestConciergeClient* GetTestConciergeClient() {
return static_cast<TestConciergeClient*>(ash::ConciergeClient::Get());
}
Expand Down Expand Up @@ -679,10 +649,6 @@ class ArcVmClientAdapterTest : public testing::Test,
bool host_rootfs_writable_;
bool system_image_ext_format_;

// List of upstart operations recorded. When it's "start" the boolean is set
// to true.
UpstartOperations upstart_operations_;

std::unique_ptr<TestArcVmBootNotificationServer> boot_server_;

FakeDemoModeDelegate demo_mode_delegate_;
Expand Down Expand Up @@ -816,24 +782,15 @@ TEST_F(ArcVmClientAdapterTest, StartMiniArc_StopArcVmPreLoginServicesJobFail) {
// Tests that |kArcVmPreLoginServicesJobName| is properly stopped and then
// started in StartMiniArc().
TEST_F(ArcVmClientAdapterTest, StartMiniArc_JobRestart) {
StartRecordingUpstartOperations();
ash::FakeUpstartClient::Get()->StartRecordingUpstartOperations();
StartMiniArc();

const auto& ops = upstart_operations();
// Find the STOP operation for the job.
auto it = base::ranges::find_if(ops, [](const UpstartOperation& op) {
return op.type == UpstartOperationType::STOP &&
op.name == kArcVmPreLoginServicesJobName;
});
ASSERT_NE(it, ops.end());
++it;
ASSERT_NE(it, ops.end());
// Find the START operation for the job.
it = base::ranges::find_if(it, ops.end(), [](const UpstartOperation& op) {
return op.type == UpstartOperationType::START &&
op.name == kArcVmPreLoginServicesJobName;
});
ASSERT_NE(it, ops.end());
const auto& ops =
ash::FakeUpstartClient::Get()->GetRecordedUpstartOperationsForJob(
kArcVmPreLoginServicesJobName);
ASSERT_EQ(ops.size(), 2u);
EXPECT_EQ(ops[0].type, ash::FakeUpstartClient::UpstartOperationType::STOP);
EXPECT_EQ(ops[1].type, ash::FakeUpstartClient::UpstartOperationType::START);
}

// Tests that StopArcInstance() eventually notifies the observer.
Expand Down Expand Up @@ -1054,35 +1011,35 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_StartArcVmPostLoginServicesFailure) {
// by default.
TEST_F(ArcVmClientAdapterTest, StartMiniArc_UreadaheadByDefault) {
StartParams start_params(GetPopulatedStartParams());
StartRecordingUpstartOperations();
ash::FakeUpstartClient::Get()->StartRecordingUpstartOperations();
StartMiniArcWithParams(true, std::move(start_params));

const auto& ops = upstart_operations();
const auto it = base::ranges::find_if(ops, [](const UpstartOperation& op) {
return op.type == UpstartOperationType::START &&
kArcVmPreLoginServicesJobName == op.name;
});
ASSERT_NE(ops.end(), it);
EXPECT_TRUE(it->env.empty());
const auto& ops =
ash::FakeUpstartClient::Get()->GetRecordedUpstartOperationsForJob(
kArcVmPreLoginServicesJobName);
ASSERT_EQ(ops.size(), 2u);
EXPECT_EQ(ops[0].type, ash::FakeUpstartClient::UpstartOperationType::STOP);
EXPECT_EQ(ops[1].type, ash::FakeUpstartClient::UpstartOperationType::START);
EXPECT_TRUE(ops[1].env.empty());
}

// Tests that StartMiniArc()'s JOB_STOP_AND_START for
// |kArcVmPreLoginServicesJobName| has DISABLE_UREADAHEAD variable.
TEST_F(ArcVmClientAdapterTest, StartMiniArc_DisableUreadahead) {
StartParams start_params(GetPopulatedStartParams());
start_params.disable_ureadahead = true;
StartRecordingUpstartOperations();
ash::FakeUpstartClient::Get()->StartRecordingUpstartOperations();
StartMiniArcWithParams(true, std::move(start_params));

const auto& ops = upstart_operations();
const auto it = base::ranges::find_if(ops, [](const UpstartOperation& op) {
return op.type == UpstartOperationType::START &&
kArcVmPreLoginServicesJobName == op.name;
});
ASSERT_NE(ops.end(), it);
const auto& ops =
ash::FakeUpstartClient::Get()->GetRecordedUpstartOperationsForJob(
kArcVmPreLoginServicesJobName);
ASSERT_EQ(ops.size(), 2u);
EXPECT_EQ(ops[0].type, ash::FakeUpstartClient::UpstartOperationType::STOP);
EXPECT_EQ(ops[1].type, ash::FakeUpstartClient::UpstartOperationType::START);
const auto it_ureadahead =
base::ranges::find(it->env, "DISABLE_UREADAHEAD=1");
EXPECT_NE(it->env.end(), it_ureadahead);
base::ranges::find(ops[1].env, "DISABLE_UREADAHEAD=1");
EXPECT_NE(ops[1].env.end(), it_ureadahead);
}

// Tests that StartMiniArc() handles arcvm-post-vm-start-services stop failures
Expand Down
77 changes: 21 additions & 56 deletions chrome/browser/ash/arc/adbd/arc_adbd_monitor_bridge_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include "ash/components/arc/test/fake_adbd_monitor_instance.h"
#include "ash/components/arc/test/fake_arc_session.h"
#include "base/memory/raw_ptr.h"
#include "base/ranges/algorithm.h"
#include "base/run_loop.h"
#include "base/test/bind.h"
#include "chrome/test/base/testing_profile.h"
Expand Down Expand Up @@ -57,6 +56,7 @@ class ArcAdbdMonitorBridgeTest : public testing::Test {
instance_.reset();
profile_.reset();
arc_service_manager_.reset();
ash::UpstartClient::Shutdown();
}

protected:
Expand All @@ -66,10 +66,6 @@ class ArcAdbdMonitorBridgeTest : public testing::Test {

ArcAdbdMonitorBridge* arc_adbd_monitor_bridge() const { return bridge_; }

const std::vector<std::pair<std::string, bool>>& upstart_operations() const {
return upstart_operations_;
}

void InjectUpstartStopJobFailure(const std::string& job_name_to_fail) {
auto* upstart_client = ash::FakeUpstartClient::Get();
upstart_client->set_stop_job_cb(base::BindLambdaForTesting(
Expand All @@ -80,40 +76,20 @@ class ArcAdbdMonitorBridgeTest : public testing::Test {
}));
}

void StartRecordingUpstartOperations() {
auto* upstart_client = ash::FakeUpstartClient::Get();
upstart_client->set_start_job_cb(
base::BindLambdaForTesting([this](const std::string& job_name,
const std::vector<std::string>& env) {
upstart_operations_.emplace_back(job_name, true);
return ash::FakeUpstartClient::StartJobResult(true /* success */);
}));
upstart_client->set_stop_job_cb(
base::BindLambdaForTesting([this](const std::string& job_name,
const std::vector<std::string>& env) {
upstart_operations_.emplace_back(job_name, false);
return true;
}));
}

private:
content::BrowserTaskEnvironment task_environment_;
std::unique_ptr<FakeAdbdMonitorInstance> instance_;
std::unique_ptr<TestingProfile> profile_;
std::unique_ptr<ArcServiceManager> arc_service_manager_;
raw_ptr<ArcAdbdMonitorBridge, DanglingUntriaged | ExperimentalAsh> bridge_;

// List of upstart operations recorded. When it's "start" the boolean is set
// to true.
std::vector<std::pair<std::string, bool>> upstart_operations_;
};

// Testing bridge constructor/destructor in setup/teardown.
TEST_F(ArcAdbdMonitorBridgeTest, TestConstructDestruct) {}

// Testing bridge start arcvm-adbd successfully
TEST_F(ArcAdbdMonitorBridgeTest, TestStartArcVmAdbdSuccess) {
StartRecordingUpstartOperations();
ash::FakeUpstartClient::Get()->StartRecordingUpstartOperations();
arc_adbd_monitor_bridge()->EnableAdbOverUsbForTesting();
base::RunLoop run_loop;
arc_adbd_monitor_bridge()->OnStartArcVmAdbdTesting(base::BindOnce(
Expand All @@ -124,23 +100,19 @@ TEST_F(ArcAdbdMonitorBridgeTest, TestStartArcVmAdbdSuccess) {
&run_loop));
run_loop.Run();

const auto& ops = upstart_operations();
// Find the STOP operation for the job.
auto it = base::ranges::find(
ops, std::make_pair(std::string(kArcVmAdbdJobName), false));
ASSERT_NE(ops.end(), it);
++it;
ASSERT_NE(ops.end(), it);
// The next operation must be START for the job.
EXPECT_EQ(it->first, kArcVmAdbdJobName);
EXPECT_TRUE(it->second); // true means START.
const auto& ops =
ash::FakeUpstartClient::Get()->GetRecordedUpstartOperationsForJob(
kArcVmAdbdJobName);
ASSERT_EQ(ops.size(), 2u);
EXPECT_EQ(ops[0].type, ash::FakeUpstartClient::UpstartOperationType::STOP);
EXPECT_EQ(ops[1].type, ash::FakeUpstartClient::UpstartOperationType::START);
}

// Testing bridge start arcvm-adbd regardless stop failure.
TEST_F(ArcAdbdMonitorBridgeTest, TestStartArcVmAdbdFailure) {
// Inject failure to FakeUpstartClient.
InjectUpstartStopJobFailure(kArcVmAdbdJobName);
StartRecordingUpstartOperations();
ash::FakeUpstartClient::Get()->StartRecordingUpstartOperations();
arc_adbd_monitor_bridge()->EnableAdbOverUsbForTesting();
base::RunLoop run_loop;
arc_adbd_monitor_bridge()->OnStartArcVmAdbdTesting(base::BindOnce(
Expand All @@ -151,22 +123,17 @@ TEST_F(ArcAdbdMonitorBridgeTest, TestStartArcVmAdbdFailure) {
&run_loop));
run_loop.Run();

const auto& ops = upstart_operations();
// Find the STOP operation for the job.
auto it = base::ranges::find(
ops, std::make_pair(std::string(kArcVmAdbdJobName), false));
EXPECT_EQ(ops.size(), 2u);
ASSERT_NE(ops.end(), it);
++it;
ASSERT_NE(ops.end(), it);
// The next operation must be START for the job.
EXPECT_EQ(it->first, kArcVmAdbdJobName);
EXPECT_TRUE(it->second); // true means START.
const auto& ops =
ash::FakeUpstartClient::Get()->GetRecordedUpstartOperationsForJob(
kArcVmAdbdJobName);
ASSERT_EQ(ops.size(), 2u);
EXPECT_EQ(ops[0].type, ash::FakeUpstartClient::UpstartOperationType::STOP);
EXPECT_EQ(ops[1].type, ash::FakeUpstartClient::UpstartOperationType::START);
}

// Testing bridge handle stop arcvm-adbd job failure well
TEST_F(ArcAdbdMonitorBridgeTest, TestStopArcVmAdbdSuccess) {
StartRecordingUpstartOperations();
ash::FakeUpstartClient::Get()->StartRecordingUpstartOperations();
arc_adbd_monitor_bridge()->EnableAdbOverUsbForTesting();
base::RunLoop run_loop;
arc_adbd_monitor_bridge()->OnStopArcVmAdbdTesting(base::BindOnce(
Expand All @@ -177,13 +144,11 @@ TEST_F(ArcAdbdMonitorBridgeTest, TestStopArcVmAdbdSuccess) {
&run_loop));
run_loop.Run();

const auto& ops = upstart_operations();
// Find the STOP operation for the job.
auto it = base::ranges::find(
ops, std::make_pair(std::string(kArcVmAdbdJobName), false));
EXPECT_EQ(ops.size(), 1u);
// The next operation must be START for the job.
EXPECT_EQ(it->first, kArcVmAdbdJobName);
const auto& ops =
ash::FakeUpstartClient::Get()->GetRecordedUpstartOperationsForJob(
kArcVmAdbdJobName);
ASSERT_EQ(ops.size(), 1u);
EXPECT_EQ(ops[0].type, ash::FakeUpstartClient::UpstartOperationType::STOP);
}

} // namespace
Expand Down

0 comments on commit a17e676

Please sign in to comment.