Refactored device status uploading out of policy fetch infrastructure.

chrome/browser/chromeos/policy/status_uploader_unittest.cc

+// Copyright (c) 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/testing_pref_service.h"
+#include "base/test/test_simple_task_runner.h"
+#include "chrome/browser/chromeos/policy/device_status_collector.h"
+#include "chrome/browser/chromeos/policy/status_uploader.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/device_settings_service.h"
+#include "chrome/common/pref_names.h"
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
+#include "components/policy/core/common/cloud/mock_device_management_service.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::_;
+using ::testing::Return;
+
+namespace em = enterprise_management;
+
+namespace {
+
+class MockDeviceStatusCollector : public policy::DeviceStatusCollector {
+ public:
+  explicit MockDeviceStatusCollector(PrefService* local_state)
+      : DeviceStatusCollector(
+            local_state,
+            nullptr,
+            policy::DeviceStatusCollector::LocationUpdateRequester(),
+            policy::DeviceStatusCollector::VolumeInfoFetcher()) {
+  }
+
+  MOCK_METHOD1(GetDeviceStatus, bool(em::DeviceStatusReportRequest*));
+  MOCK_METHOD1(GetSessionStatus, bool(em::SessionStatusReportRequest*));
+};
+
+}  // namespace
+
+namespace policy {
+class StatusUploaderTest : public testing::Test {
+ public:
+  StatusUploaderTest()
+      : task_runner_(new base::TestSimpleTaskRunner()) {
+    DeviceStatusCollector::RegisterPrefs(prefs_.registry());
+    prefs_.registry()->RegisterIntegerPref(
+        prefs::kDeviceStatusUploadRate,
+        StatusUploader::kDefaultUploadDelayMs);
+  }
+
+  void SetUp() override {
+    client_.reset(new CloudPolicyClient(
+        "machine_id",
+        "machine_model",
+        GetPolicyVerificationKey(),
+        UserAffiliation::USER_AFFILIATION_NONE,
+        &device_management_service_,
+        scoped_refptr<net::URLRequestContextGetter>()));
+    client_->SetupRegistration("dm_token", "client_id");
+  }
+
+
+  // Helper function that creates a StatusUploader object to use in tests.
+  scoped_ptr<StatusUploader> CreateStatusUploader(
+      scoped_ptr<DeviceStatusCollector> collector) {
+    return make_scoped_ptr(new StatusUploader(
+        &prefs_,
+        client_.get(),
+        collector.Pass(),
+        task_runner_));
+  }
+
+  // Given a pending task to upload status, mocks out a server response.
+  void MockResponseForPendingUploadTask() {
+    EXPECT_FALSE(task_runner_->GetPendingTasks().empty());
+    MockDeviceManagementJob* upload_request = NULL;
+
+    EXPECT_CALL(device_management_service_,
+                CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_STATUS, _))
+        .WillOnce(device_management_service_.CreateAsyncJob(&upload_request));
+
+    EXPECT_CALL(device_management_service_, StartJob(_, _, _, _, _, _, _));
+    task_runner_->RunPendingTasks();
+    testing::Mock::VerifyAndClearExpectations(&device_management_service_);
+
+    EXPECT_TRUE(upload_request);

[Mattias Nissler (ping if slow), 2015/01/23 13:46:35]

This needs to be ASSERT_TRUE, otherwise you crash below.

[Andrew T Wilson (Slow), 2015/01/23 19:16:19]

Done.

+    // Make sure no status upload is queued up yet (since an upload is in
+    // progress).
+    EXPECT_TRUE(task_runner_->GetPendingTasks().empty());
+
+    upload_request->SendResponse(DM_STATUS_SUCCESS,
+                                 em::DeviceManagementResponse());
+  }
+
+  base::MessageLoop loop_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;
+  chromeos::ScopedTestCrosSettings test_cros_settings_;

[Mattias Nissler (ping if slow), 2015/01/23 13:46:35]

It's kinda sad that we have to bring up this heavy-wait BS just to test the
status uploader...

[Andrew T Wilson (Slow), 2015/01/23 19:16:19]

Yeah, this is the cost of having a CrosSettings singleton.

+  scoped_ptr<CloudPolicyClient> client_;
+  MockDeviceManagementService device_management_service_;
+  TestingPrefServiceSimple prefs_;
+};
+
+TEST_F(StatusUploaderTest, BasicTest) {
+  scoped_ptr<MockDeviceStatusCollector> collector(
+      new MockDeviceStatusCollector(&prefs_));

[Mattias Nissler (ping if slow), 2015/01/23 13:46:35]

All the tests need a collector - move it to the fixture?

[Andrew T Wilson (Slow), 2015/01/23 19:16:19]

Done.

+  EXPECT_TRUE(task_runner_->GetPendingTasks().empty());
+  scoped_ptr<StatusUploader> uploader(
+      CreateStatusUploader(collector.Pass()).Pass());

[Mattias Nissler (ping if slow), 2015/01/23 13:46:35]

The additional boilerplate hardly justifies why you want a CreateStatusUploader
helper instead of just doing:

StatusUploader uploader(&prefs_, client_.get(), collector.Pass(), task_runner_);

Same below.

[Andrew T Wilson (Slow), 2015/01/23 19:16:19]

Yep, it used to do more stuff. Happy to un-refactor this.

+  EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+  // On startup, first update should happen immediately.
+  EXPECT_EQ(base::TimeDelta(), task_runner_->NextPendingTaskDelay());
+}
+
+TEST_F(StatusUploaderTest, ResetTimerAfterStatusCollection) {

[Mattias Nissler (ping if slow), 2015/01/23 13:46:35]

Would it make sense to check the delays as well to make sure they're not too
short (assuming TestSimpleTaskRunner gives you access to them)?

[Andrew T Wilson (Slow), 2015/01/23 19:16:19]

I originally tried this and it's hard to do well - TestSimpleTaskRunner gives us
a "how long until the next task will fire" but if the test is running slowly
this will be a varying amount less than the expected value, and it felt fragile
to have something like (DCHECK_GT(time_remaining, expected_time -
FUDGE_FACTOR_LETS_HOPE_THE_TEST_DOES_NOT_RUN_SLOWER_THAN_THIS)).

So I decided it's better not to test the delay values - if you have an idea of
how to do this more cleanly let me know and I'll add this.

[Mattias Nissler (ping if slow), 2015/01/26 10:37:02]

cloud_policy_refresh_scheduler_unittest.cc has some docs on how to do this
properly:
https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/...

The idea is basically to measure time before and after you trigger the code
under test and then allow the entire window for valid next refresh delays.

Anyhow, your call - I don't insist on timing tests in this case.

[Andrew T Wilson (Slow), 2015/01/28 13:19:11]

Done.

+  scoped_ptr<MockDeviceStatusCollector> collector(
+      new MockDeviceStatusCollector(&prefs_));
+  // Keep a pointer to the mock collector because the scoped_ptr gets cleared
+  // when it is passed to CreateStatusUploader() below.
+  MockDeviceStatusCollector* mock_collector = collector.get();
+  scoped_ptr<StatusUploader> uploader(
+      CreateStatusUploader(collector.Pass()).Pass());
+  EXPECT_CALL(*mock_collector, GetDeviceStatus(_)).WillRepeatedly(Return(true));
+  EXPECT_CALL(*mock_collector, GetSessionStatus(_)).WillRepeatedly(
+      Return(true));
+  MockResponseForPendingUploadTask();
+
+  // Now that the previous request was satisfied, a task to do the next
+  // upload should be queued.
+  EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+
+  // Handle this response also.
+  MockResponseForPendingUploadTask();
+
+  // Now that the previous request was satisfied, a task to do the next
+  // upload should be queued again.
+  EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+}
+
+TEST_F(StatusUploaderTest, ResetTimerAfterFailedStatusCollection) {
+  scoped_ptr<MockDeviceStatusCollector> collector(
+      new MockDeviceStatusCollector(&prefs_));
+  // Keep a pointer to the mock collector because the scoped_ptr gets cleared
+  // when it is passed to CreateStatusUploader() below.
+  MockDeviceStatusCollector* mock_collector = collector.get();
+  scoped_ptr<StatusUploader> uploader(
+      CreateStatusUploader(collector.Pass()).Pass());
+  EXPECT_CALL(*mock_collector, GetDeviceStatus(_)).WillOnce(Return(false));
+  EXPECT_CALL(*mock_collector, GetSessionStatus(_)).WillOnce(Return(false));
+  task_runner_->RunPendingTasks();
+
+  // Make sure the next status upload is queued up.
+  EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+}
+
+}  // namespace policy