Support VariationsRestrictParameter in VariationsService for Chrome OS

chrome/browser/chromeos/policy/variations_service_policy_browsertest.cc

+// Copyright (c) 2013 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 <vector>
+
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/path_service.h"
+#include "base/stl_util.h"
+#include "chrome/browser/chromeos/cros/cros_in_process_browser_test.h"
+#include "chrome/browser/chromeos/policy/device_policy_builder.h"
+#include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h"
+#include "chrome/browser/metrics/variations/variations_service.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chromeos/dbus/fake_session_manager_client.h"
+#include "chromeos/dbus/mock_dbus_thread_manager.h"
+#include "chromeos/dbus/mock_image_burner_client.h"
+#include "crypto/rsa_private_key.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::_;
+using ::testing::AnyNumber;
+using ::testing::Return;
+
+namespace em = enterprise_management;

[Alexei Svitkine (slow), 2013/04/18 19:27:53]

You're only using em:: once in the file, this doesn't warrant this line.

[Mathieu, 2013/04/18 19:51:14]

Done.

+
+namespace policy {
+
+class DeviceVariationsServicePolicyTest :
+    public chromeos::CrosInProcessBrowserTest {
+ protected:
+  DeviceVariationsServicePolicyTest() {};
+
+  virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
+    chromeos::MockDBusThreadManager* mock_dbus_thread_manager =
+        new chromeos::MockDBusThreadManager;
+
+    EXPECT_CALL(*mock_dbus_thread_manager->mock_image_burner_client(),
+                ResetEventHandlers())
+        .Times(AnyNumber());
+    EXPECT_CALL(*mock_dbus_thread_manager->mock_image_burner_client(),
+                SetEventHandlers(_, _))
+        .Times(AnyNumber());
+
+    SetUpSessionManager(mock_dbus_thread_manager);
+
+    chromeos::DBusThreadManager::InitializeForTesting(mock_dbus_thread_manager);
+    CrosInProcessBrowserTest::SetUpInProcessBrowserTestFixture();
+  }
+
+  void SetUpSessionManager(
+    chromeos::MockDBusThreadManager* mock_dbus_thread_manager) {
+    EXPECT_CALL(*mock_dbus_thread_manager, GetSessionManagerClient())
+        .WillRepeatedly(Return(&session_manager_client_));
+
+    // Install the owner key.
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    base::FilePath owner_key_file = temp_dir_.path().AppendASCII("owner.key");
+    std::vector<uint8> owner_key_bits;
+    ASSERT_TRUE(device_policy_.signing_key()->ExportPublicKey(&owner_key_bits));
+    ASSERT_EQ(
+        file_util::WriteFile(
+            owner_key_file,
+            reinterpret_cast<const char*>(vector_as_array(&owner_key_bits)),
+            owner_key_bits.size()),
+        static_cast<int>(owner_key_bits.size()));
+    ASSERT_TRUE(PathService::Override(chrome::FILE_OWNER_KEY, owner_key_file));
+
+    // Setup the device policy DeviceVariationsRestrictParameter.
+    em::ChromeDeviceSettingsProto& proto(device_policy_.payload());
+    proto.mutable_variations_parameter()->set_parameter("restricted");
+    RefreshDevicePolicy();
+  }

[Alexei Svitkine (slow), 2013/04/18 19:27:53]

This seems like a lot of boilerplate for a pretty small test case.

I'm assuming you copy-pasted most of this out of an existing test file
somewhere?

Is it possible to make a common test super-class file where this boiletplate
will live, thus allowing each test to simply inherit from it without duplicating
the code?

[Mathieu, 2013/04/18 19:51:14]

It comes from:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

In particular ExistingUserControllerPublicSessionTest, which subclasses a bigger
class in the same file.

The SetUpInProcessBrowserTestFixture methods ended up having a different number
of expectations and this file mocks way fewer interfaces.

Also, the two tests are in different namespaces (chromeos and policy). 

I agree it's some amount of boilerplate, perhaps mnissler@ can suggest a better
way?

[Alexei Svitkine (slow), 2013/04/18 20:01:27]

I suggest making a common superclass, derived from CrosInProcessBrowserTest,
that will have utility methods for the parts that are common to both tests, for
example RefreshDevicePolicy() and InstallOwnerKey() and changing this class and
ExistingUserControllerTest to derive from it and call those methods.

+
+  void RefreshDevicePolicy() {
+    // Reset the key to its original state.
+    device_policy_.set_signing_key(
+        policy::PolicyBuilder::CreateTestSigningKey());
+    device_policy_.Build();
+    // Trick the device into thinking it's enterprise-enrolled by
+    // removing the local private key.  This will allow it to accept
+    // cloud policy for device owner settings.
+    device_policy_.set_signing_key(
+        make_scoped_ptr<crypto::RSAPrivateKey>(NULL));
+    device_policy_.set_new_signing_key(
+        make_scoped_ptr<crypto::RSAPrivateKey>(NULL));
+    session_manager_client_.set_device_policy(device_policy_.GetBlob());
+    session_manager_client_.OnPropertyChangeComplete(true);
+  }
+
+  virtual void TearDownInProcessBrowserTestFixture() OVERRIDE {
+    CrosInProcessBrowserTest::TearDownInProcessBrowserTestFixture();
+    chromeos::DBusThreadManager::Shutdown();
+  }
+
+ private:
+  // Mock out policy loads/stores from/to the device.
+  chromeos::FakeSessionManagerClient session_manager_client_;
+
+  // Stores the device owner key.
+  base::ScopedTempDir temp_dir_;
+
+  // Carries Chrome OS device policies for tests.
+  DevicePolicyBuilder device_policy_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeviceVariationsServicePolicyTest);
+};
+
+IN_PROC_BROWSER_TEST_F(DeviceVariationsServicePolicyTest, VariationsURLValid) {
+  const std::string default_variations_url =
+      chrome_variations::VariationsService::
+          GetDefaultVariationsServerURLForTesting();
+
+  // Device policy has updated the cros settings.
+  EXPECT_EQ(default_variations_url + "?restrict=restricted",
+            chrome_variations::VariationsService::GetVariationsServerURL(
+                g_browser_process->local_state()).spec());
+}
+
+} // namespace policy