chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc - Issue 1019283004: Switch to direct use of OwnerSettingsServiceChromeOS::Set() in tests.

Unified Diff: chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc

Issue 1019283004: Switch to direct use of OwnerSettingsServiceChromeOS::Set() in tests. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Rearrange device settings list. Created 5 years, 9 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/ownership/owner_settings_service_chromeos.cc ('K') | « chrome/browser/chromeos/policy/extension_cache_unittest.cc ('k') | chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc » ('j') | chrome/browser/chromeos/settings/device_settings_provider.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc

diff --git a/chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc b/chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc

index 7a1a05cf2e78c6fd92f00cf933bec62f3e472967..8726d02f04480932af0e4fbb844cb0f49f84f49f 100644

--- a/chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc

+++ b/chrome/browser/chromeos/policy/heartbeat_scheduler_unittest.cc

@@ -6,9 +6,7 @@

#include "base/strings/string_number_conversions.h"

#include "base/test/test_simple_task_runner.h"

-#include "chrome/browser/chromeos/settings/cros_settings.h"

-#include "chrome/browser/chromeos/settings/device_settings_service.h"

-#include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"

+#include "chrome/browser/chromeos/settings/device_settings_test_helper.h"

#include "chromeos/settings/cros_settings_names.h"

#include "components/gcm_driver/fake_gcm_driver.h"

#include "content/public/test/test_utils.h"

@@ -54,7 +52,7 @@ class MockGCMDriver : public testing::StrictMock<gcm::FakeGCMDriver> {

}

};

-class HeartbeatSchedulerTest : public testing::Test {

+class HeartbeatSchedulerTest : public chromeos::OwnerSettingsTestBase {

Mattias Nissler (ping if slow) 2015/03/27 13:45:58 Instead of pulling in the heavy-weight full device

Ivan Podogov 2015/03/27 14:40:59 True, sometimes it's the only option left... Altho

Mattias Nissler (ping if slow) 2015/03/27 16:20:23 The boilerplate reduction is certainly great. Howe

Ivan Podogov 2015/03/30 08:54:28 OK, I guess I'll make such a class. Do you think w

Mattias Nissler (ping if slow) 2015/03/30 09:03:51 Judgement call, depends on how convenient it ends

On 2015/03/30 08:54:28, Ivan Podogov wrote: > On 2015/03/27 16:20:23, Mattias Nissler wrote: > > On 2015/03/27 14:40:59, Ivan Podogov wrote: > > > On 2015/03/27 13:45:58, Mattias Nissler wrote: > > > > Instead of pulling in the heavy-weight full device settings test > dependency > > > > here, why not leave StubCrosSettingsProvider in place as before and make > it > > > have > > > > its own Set() function that isn't related to > CrosSettingsProvider::DoSet()? > > I > > > > think it's fine to have Set() in test code in order to create the > > environment > > > > the test needs. > > > > > > True, sometimes it's the only option left... Although I really like how the > > > tests had their size, copy-paste amount and the number of stuff included in > > them > > > reduced. See the ExtensionCache test, for example: it basically has every > > single > > > irrelevant thing removed from it. And the tests themselves are more end-to > end > > > in this case, they even check correctness of the policy protobuf decoding > > (which > > > isn't really checked anywhere else). > > > > The boilerplate reduction is certainly great. However, you can achieve the > same > > without pulling in DeviceSettingsTestHelper by just factoring the common > > StubCrosSettingsProvider stuff that's currently copied-and-pasted into a > common > > base class. > > > > Another argument in favor of not pulling in DeviceSettingsTestHelper is that > all > > the affected tests (at least the ones I've looked at) are unit tests, so it > > doesn't make sense to have most of the production machinery for device > settings > > that does the entire round trip to SessionManagerClient running within the > test > > (and being implicitly tested as well). Instead, you want to mock that away as > > good as you can, and StubCrosSettingsProvider is great at that. > > OK, I guess I'll make such a class. > Do you think we should add SetInteger/SetBoolean/SetString methods to the > CrosSettingsProvider (or at least to the StubCrosSettingsProvider) for > convenience?

Judgement call, depends on how convenient it ends up being ;) If the boilerplate it avoids isn't too significant and there are only few callers, then don't bother. If you find yourself adding the same block of code in various places, add helpers.

Ivan Podogov 2015/03/30 09:10:23 I'm going to go over more than a dozen tests, and

On 2015/03/30 09:03:51, Mattias Nissler wrote: > On 2015/03/30 08:54:28, Ivan Podogov wrote: > > On 2015/03/27 16:20:23, Mattias Nissler wrote: > > > On 2015/03/27 14:40:59, Ivan Podogov wrote: > > > > On 2015/03/27 13:45:58, Mattias Nissler wrote: > > > > > Instead of pulling in the heavy-weight full device settings test > > dependency > > > > > here, why not leave StubCrosSettingsProvider in place as before and make > > it > > > > have > > > > > its own Set() function that isn't related to > > CrosSettingsProvider::DoSet()? > > > I > > > > > think it's fine to have Set() in test code in order to create the > > > environment > > > > > the test needs. > > > > > > > > True, sometimes it's the only option left... Although I really like how > the > > > > tests had their size, copy-paste amount and the number of stuff included > in > > > them > > > > reduced. See the ExtensionCache test, for example: it basically has every > > > single > > > > irrelevant thing removed from it. And the tests themselves are more end-to > > end > > > > in this case, they even check correctness of the policy protobuf decoding > > > (which > > > > isn't really checked anywhere else). > > > > > > The boilerplate reduction is certainly great. However, you can achieve the > > same > > > without pulling in DeviceSettingsTestHelper by just factoring the common > > > StubCrosSettingsProvider stuff that's currently copied-and-pasted into a > > common > > > base class. > > > > > > Another argument in favor of not pulling in DeviceSettingsTestHelper is that > > all > > > the affected tests (at least the ones I've looked at) are unit tests, so it > > > doesn't make sense to have most of the production machinery for device > > settings > > > that does the entire round trip to SessionManagerClient running within the > > test > > > (and being implicitly tested as well). Instead, you want to mock that away > as > > > good as you can, and StubCrosSettingsProvider is great at that. > > > > OK, I guess I'll make such a class. > > Do you think we should add SetInteger/SetBoolean/SetString methods to the > > CrosSettingsProvider (or at least to the StubCrosSettingsProvider) for > > convenience? > > Judgement call, depends on how convenient it ends up being ;) If the boilerplate > it avoids isn't too significant and there are only few callers, then don't > bother. If you find yourself adding the same block of code in various places, > add helpers.

I'm going to go over more than a dozen tests, and add "Set(..., base::FundamentalValue(...))" to all of them. All those helpers will be trivial one-liners that will hide FundamentalValue (and the corresponding include) and somewhat increase readability. So, for me personally, it would be much nicer to have them. :)

Mattias Nissler (ping if slow) 2015/03/30 09:11:08 I'd say go a head and add them then :)

On 2015/03/30 09:10:23, Ivan Podogov wrote: > On 2015/03/30 09:03:51, Mattias Nissler wrote: > > On 2015/03/30 08:54:28, Ivan Podogov wrote: > > > On 2015/03/27 16:20:23, Mattias Nissler wrote: > > > > On 2015/03/27 14:40:59, Ivan Podogov wrote: > > > > > On 2015/03/27 13:45:58, Mattias Nissler wrote: > > > > > > Instead of pulling in the heavy-weight full device settings test > > > dependency > > > > > > here, why not leave StubCrosSettingsProvider in place as before and > make > > > it > > > > > have > > > > > > its own Set() function that isn't related to > > > CrosSettingsProvider::DoSet()? > > > > I > > > > > > think it's fine to have Set() in test code in order to create the > > > > environment > > > > > > the test needs. > > > > > > > > > > True, sometimes it's the only option left... Although I really like how > > the > > > > > tests had their size, copy-paste amount and the number of stuff included > > in > > > > them > > > > > reduced. See the ExtensionCache test, for example: it basically has > every > > > > single > > > > > irrelevant thing removed from it. And the tests themselves are more > end-to > > > end > > > > > in this case, they even check correctness of the policy protobuf > decoding > > > > (which > > > > > isn't really checked anywhere else). > > > > > > > > The boilerplate reduction is certainly great. However, you can achieve the > > > same > > > > without pulling in DeviceSettingsTestHelper by just factoring the common > > > > StubCrosSettingsProvider stuff that's currently copied-and-pasted into a > > > common > > > > base class. > > > > > > > > Another argument in favor of not pulling in DeviceSettingsTestHelper is > that > > > all > > > > the affected tests (at least the ones I've looked at) are unit tests, so > it > > > > doesn't make sense to have most of the production machinery for device > > > settings > > > > that does the entire round trip to SessionManagerClient running within the > > > test > > > > (and being implicitly tested as well). Instead, you want to mock that away > > as > > > > good as you can, and StubCrosSettingsProvider is great at that. > > > > > > OK, I guess I'll make such a class. > > > Do you think we should add SetInteger/SetBoolean/SetString methods to the > > > CrosSettingsProvider (or at least to the StubCrosSettingsProvider) for > > > convenience? > > > > Judgement call, depends on how convenient it ends up being ;) If the > boilerplate > > it avoids isn't too significant and there are only few callers, then don't > > bother. If you find yourself adding the same block of code in various places, > > add helpers. > > I'm going to go over more than a dozen tests, and add "Set(..., > base::FundamentalValue(...))" to all of them. All those helpers will be trivial > one-liners that will hide FundamentalValue (and the corresponding include) and > somewhat increase readability. So, for me personally, it would be much nicer to > have them. :)

I'd say go a head and add them then :)

Ivan Podogov 2015/03/30 14:22:57 OK, so I suppose the new patchset should look more

On 2015/03/30 09:11:08, Mattias Nissler wrote: > On 2015/03/30 09:10:23, Ivan Podogov wrote: > > On 2015/03/30 09:03:51, Mattias Nissler wrote: > > > On 2015/03/30 08:54:28, Ivan Podogov wrote: > > > > On 2015/03/27 16:20:23, Mattias Nissler wrote: > > > > > On 2015/03/27 14:40:59, Ivan Podogov wrote: > > > > > > On 2015/03/27 13:45:58, Mattias Nissler wrote: > > > > > > > Instead of pulling in the heavy-weight full device settings test > > > > dependency > > > > > > > here, why not leave StubCrosSettingsProvider in place as before and > > make > > > > it > > > > > > have > > > > > > > its own Set() function that isn't related to > > > > CrosSettingsProvider::DoSet()? > > > > > I > > > > > > > think it's fine to have Set() in test code in order to create the > > > > > environment > > > > > > > the test needs. > > > > > > > > > > > > True, sometimes it's the only option left... Although I really like > how > > > the > > > > > > tests had their size, copy-paste amount and the number of stuff > included > > > in > > > > > them > > > > > > reduced. See the ExtensionCache test, for example: it basically has > > every > > > > > single > > > > > > irrelevant thing removed from it. And the tests themselves are more > > end-to > > > > end > > > > > > in this case, they even check correctness of the policy protobuf > > decoding > > > > > (which > > > > > > isn't really checked anywhere else). > > > > > > > > > > The boilerplate reduction is certainly great. However, you can achieve > the > > > > same > > > > > without pulling in DeviceSettingsTestHelper by just factoring the common > > > > > StubCrosSettingsProvider stuff that's currently copied-and-pasted into a > > > > common > > > > > base class. > > > > > > > > > > Another argument in favor of not pulling in DeviceSettingsTestHelper is > > that > > > > all > > > > > the affected tests (at least the ones I've looked at) are unit tests, so > > it > > > > > doesn't make sense to have most of the production machinery for device > > > > settings > > > > > that does the entire round trip to SessionManagerClient running within > the > > > > test > > > > > (and being implicitly tested as well). Instead, you want to mock that > away > > > as > > > > > good as you can, and StubCrosSettingsProvider is great at that. > > > > > > > > OK, I guess I'll make such a class. > > > > Do you think we should add SetInteger/SetBoolean/SetString methods to the > > > > CrosSettingsProvider (or at least to the StubCrosSettingsProvider) for > > > > convenience? > > > > > > Judgement call, depends on how convenient it ends up being ;) If the > > boilerplate > > > it avoids isn't too significant and there are only few callers, then don't > > > bother. If you find yourself adding the same block of code in various > places, > > > add helpers. > > > > I'm going to go over more than a dozen tests, and add "Set(..., > > base::FundamentalValue(...))" to all of them. All those helpers will be > trivial > > one-liners that will hide FundamentalValue (and the corresponding include) and > > somewhat increase readability. So, for me personally, it would be much nicer > to > > have them. :) > > I'd say go a head and add them then :)

OK, so I suppose the new patchset should look more like what you had in mind.

public:

HeartbeatSchedulerTest()

: task_runner_(new base::TestSimpleTaskRunner()),

@@ -63,24 +61,13 @@ class HeartbeatSchedulerTest : public testing::Test {

}

void SetUp() override {

- // Swap out the DeviceSettingsProvider with our stub settings provider

- // so we can set values for the heartbeat frequency.

- chromeos::CrosSettings* cros_settings = chromeos::CrosSettings::Get();

- device_settings_provider_ =

- cros_settings->GetProvider(chromeos::kReportDeviceVersionInfo);

- EXPECT_TRUE(device_settings_provider_);

- EXPECT_TRUE(

- cros_settings->RemoveSettingsProvider(device_settings_provider_));

- cros_settings->AddSettingsProvider(&stub_settings_provider_);

+ chromeos::OwnerSettingsTestBase::SetUp();

+ ReplaceProvider(chromeos::kHeartbeatEnabled);

}

void TearDown() override {

+ chromeos::OwnerSettingsTestBase::TearDown();

content::RunAllBlockingPoolTasksUntilIdle();

- // Restore the real DeviceSettingsProvider.

- chromeos::CrosSettings* cros_settings = chromeos::CrosSettings::Get();

- EXPECT_TRUE(cros_settings->RemoveSettingsProvider(

- &stub_settings_provider_));

- cros_settings->AddSettingsProvider(device_settings_provider_);

}

void CheckPendingTaskDelay(base::Time last_heartbeat,

@@ -105,14 +92,6 @@ class HeartbeatSchedulerTest : public testing::Test {

EXPECT_GE(now - last_heartbeat, delta);

}

- base::MessageLoop loop_;

- // Helpers used to mock out cros settings.

- chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;

- chromeos::ScopedTestCrosSettings test_cros_settings_;

- chromeos::CrosSettingsProvider* device_settings_provider_;

- chromeos::StubCrosSettingsProvider stub_settings_provider_;

MockGCMDriver gcm_driver_;

// TaskRunner used to run individual tests.

@@ -125,16 +104,14 @@ class HeartbeatSchedulerTest : public testing::Test {

TEST_F(HeartbeatSchedulerTest, Basic) {

// Just makes sure we can spin up and shutdown the scheduler with

// heartbeats disabled.

- chromeos::CrosSettings::Get()->SetBoolean(

- chromeos::kHeartbeatEnabled, false);

+ owner_settings_service_->SetBoolean(chromeos::kHeartbeatEnabled, false);

ASSERT_TRUE(task_runner_->GetPendingTasks().empty());

}

TEST_F(HeartbeatSchedulerTest, PermanentlyFailedGCMRegistration) {

// If heartbeats are enabled, we should register with GCMDriver.

EXPECT_CALL(gcm_driver_, RegisterImpl(kHeartbeatGCMAppID, _));

- chromeos::CrosSettings::Get()->SetBoolean(

- chromeos::kHeartbeatEnabled, true);

+ owner_settings_service_->SetBoolean(chromeos::kHeartbeatEnabled, true);

gcm_driver_.CompleteRegistration(

kHeartbeatGCMAppID, gcm::GCMClient::GCM_DISABLED);

@@ -144,8 +121,7 @@ TEST_F(HeartbeatSchedulerTest, PermanentlyFailedGCMRegistration) {

TEST_F(HeartbeatSchedulerTest, TemporarilyFailedGCMRegistration) {

EXPECT_CALL(gcm_driver_, RegisterImpl(kHeartbeatGCMAppID, _));

- chromeos::CrosSettings::Get()->SetBoolean(

- chromeos::kHeartbeatEnabled, true);

+ owner_settings_service_->SetBoolean(chromeos::kHeartbeatEnabled, true);

gcm_driver_.CompleteRegistration(

kHeartbeatGCMAppID, gcm::GCMClient::SERVER_ERROR);

testing::Mock::VerifyAndClearExpectations(&gcm_driver_);

@@ -165,8 +141,7 @@ TEST_F(HeartbeatSchedulerTest, TemporarilyFailedGCMRegistration) {

TEST_F(HeartbeatSchedulerTest, ChangeHeartbeatFrequency) {

EXPECT_CALL(gcm_driver_, RegisterImpl(kHeartbeatGCMAppID, _));

- chromeos::CrosSettings::Get()->SetBoolean(

- chromeos::kHeartbeatEnabled, true);

+ owner_settings_service_->SetBoolean(chromeos::kHeartbeatEnabled, true);

gcm_driver_.CompleteRegistration(

kHeartbeatGCMAppID, gcm::GCMClient::SUCCESS);

@@ -176,8 +151,7 @@ TEST_F(HeartbeatSchedulerTest, ChangeHeartbeatFrequency) {

testing::Mock::VerifyAndClearExpectations(&gcm_driver_);

const int new_delay = 1234*1000; // 1234 seconds.

- chromeos::CrosSettings::Get()->SetInteger(chromeos::kHeartbeatFrequency,

- new_delay);

+ owner_settings_service_->SetInteger(chromeos::kHeartbeatFrequency, new_delay);

// Now run pending heartbeat task, should send a heartbeat.

gcm::GCMClient::OutgoingMessage message;

EXPECT_CALL(gcm_driver_, SendImpl(kHeartbeatGCMAppID, _, _))

@@ -197,8 +171,7 @@ TEST_F(HeartbeatSchedulerTest, ChangeHeartbeatFrequency) {

TEST_F(HeartbeatSchedulerTest, DisableHeartbeats) {

// Makes sure that we can disable heartbeats on the fly.

EXPECT_CALL(gcm_driver_, RegisterImpl(kHeartbeatGCMAppID, _));

- chromeos::CrosSettings::Get()->SetBoolean(

- chromeos::kHeartbeatEnabled, true);

+ owner_settings_service_->SetBoolean(chromeos::kHeartbeatEnabled, true);

gcm::GCMClient::OutgoingMessage message;

EXPECT_CALL(gcm_driver_, SendImpl(kHeartbeatGCMAppID, _, _))

.WillOnce(SaveArg<2>(&message));

@@ -221,8 +194,8 @@ TEST_F(HeartbeatSchedulerTest, DisableHeartbeats) {

testing::Mock::VerifyAndClearExpectations(&gcm_driver_);

// Now disable heartbeats. Should get no more heartbeats sent.

- chromeos::CrosSettings::Get()->SetBoolean(

- chromeos::kHeartbeatEnabled, false);

+ owner_settings_service_->SetBoolean(chromeos::kHeartbeatEnabled, false);

+ EXPECT_CALL(gcm_driver_, RegisterImpl(kHeartbeatGCMAppID, _));

task_runner_->RunPendingTasks();

EXPECT_TRUE(task_runner_->GetPendingTasks().empty());

}

@@ -232,8 +205,7 @@ TEST_F(HeartbeatSchedulerTest, CheckMessageContents) {

EXPECT_CALL(gcm_driver_, RegisterImpl(kHeartbeatGCMAppID, _));

EXPECT_CALL(gcm_driver_, SendImpl(kHeartbeatGCMAppID, _, _))

.WillOnce(SaveArg<2>(&message));

- chromeos::CrosSettings::Get()->SetBoolean(

- chromeos::kHeartbeatEnabled, true);

+ owner_settings_service_->SetBoolean(chromeos::kHeartbeatEnabled, true);

gcm_driver_.CompleteRegistration(

kHeartbeatGCMAppID, gcm::GCMClient::SUCCESS);

task_runner_->RunPendingTasks();