settings: Add async system salt retrieval logic in DeviceOAuth2TokenServiceFactory

chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc

Index: chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc
diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f2546493f85a0bfff47690cf2d90cf00b18d7694
--- /dev/null
+++ b/chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc
@@ -0,0 +1,157 @@
+// Copyright 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 "chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "chromeos/cryptohome/system_salt_getter.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_cryptohome_client.h"
+#include "chromeos/dbus/fake_dbus_thread_manager.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace {
+
+// Copies the token service and increments the counter.
+void CopyTokenServiceAndCount(
+    chromeos::DeviceOAuth2TokenService** out_token_service,
+    int* counter,
+    chromeos::DeviceOAuth2TokenService* in_token_service) {
+  *out_token_service = in_token_service;
+  ++(*counter);
+}
+
+// Sets up and tears down DeviceOAuth2TokenServiceFactory and its
+// dependencies. Also exposes FakeDBusThreadManager.
+class ScopedDeviceOAuth2TokenServiceFactorySetUp {
+ public:
+  ScopedDeviceOAuth2TokenServiceFactorySetUp()
+      : fake_dbus_manager_(new FakeDBusThreadManager) {
+    // Take ownership of fake_dbus_manager_.
+    DBusThreadManager::InitializeForTesting(fake_dbus_manager_);
+    SystemSaltGetter::Initialize();
+    DeviceOAuth2TokenServiceFactory::Initialize();
+  }
+
+  ~ScopedDeviceOAuth2TokenServiceFactorySetUp() {
+    DeviceOAuth2TokenServiceFactory::Shutdown();
+    SystemSaltGetter::Shutdown();
+    DBusThreadManager::Shutdown();
+  }
+
+  FakeDBusThreadManager* fake_dbus_manager() {
+    return fake_dbus_manager_;
+  }
+
+ private:
+  FakeDBusThreadManager* fake_dbus_manager_;
+};
+
+}  // namespace
+
+class DeviceOAuth2TokenServiceFactoryTest : public testing::Test {
+ protected:
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+// Test a case where Get() is called before the factory is initialized.

[pneubeck (no reviews), 2013/10/24 19:44:13]

considering how much testing this logic requires, I really hope that we don't
have to implement and test this in more places than this one.

otherwise, we might consider putting this logic into some template/base class
and test it once and for all.

[satorux1, 2013/10/25 02:36:02]

I think smaller test cases are often clearer than a jumbo test case that covers
many things, hence I split test cases here.
That'd reduce the number of lines, but make tests less easy to follow and
change. I think this level of redundancy is acceptable.

[pneubeck (no reviews), 2013/10/25 07:35:02]

I think my comment was misleading.
I meant, if this pattern/logic of "storing callbacks until a shared object is
asynchronously created" has to be used somewhere outside of
DeviceOAuth2TokenServiceFactory, then we should consider sharing that code.

Your implementation and tests, I think, are perfectly fine.

+TEST_F(DeviceOAuth2TokenServiceFactoryTest, Get_Uninitialized) {
+  DeviceOAuth2TokenService* token_service = NULL;
+  int counter = 0;
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service, &counter));
+  // The callback will be run asynchronously.
+  EXPECT_EQ(0, counter);
+  EXPECT_FALSE(token_service);
+
+  // This lets the factory to run callback with NULL.

[pneubeck (no reviews), 2013/10/24 19:44:13]

nit: somethings wrong with this sentence, maybe remove 'to' and add 'the' before
'callback'
 (I'm not a native speaker, so not sure)

[satorux1, 2013/10/25 02:36:02]

thanks. done.

+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, counter);
+  EXPECT_FALSE(token_service);
+}
+
+// Test a case where Get() is called from only one caller.
+TEST_F(DeviceOAuth2TokenServiceFactoryTest, Get_Simple) {
+  ScopedDeviceOAuth2TokenServiceFactorySetUp scoped_setup;
+
+  DeviceOAuth2TokenService* token_service = NULL;
+  int counter = 0;
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service, &counter));
+  // The callback will be run asynchronously.
+  EXPECT_EQ(0, counter);
+  EXPECT_FALSE(token_service);
+
+  // This lets FakeCryptohomeClient return the system salt.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, counter);
+  EXPECT_TRUE(token_service);
+}
+
+// Test a case where Get() is called from multiple callers, before the token
+// service is ready, and confirm that the callback of every caller is run.
+TEST_F(DeviceOAuth2TokenServiceFactoryTest, Get_MultipleCallers) {

[pneubeck (no reviews), 2013/10/24 19:44:13]

This test subsumes "Get_Simple". Maybe remove "Get_Simple" then.
Or replace "Get_Simple" by the more special corner case, where Get() is _not_
called before RunUntilIdle. So that the pending_callbacks_ list is empty and
nothing crashes.

I also don't see a test where Get() is called successfully after RunUntilIdle.

[satorux1, 2013/10/25 02:36:02]

I wanted to separate Simple And MultipleCallers to make the intentions of the
tests clear, but I see your point. Modified Get_Simple to call Get() after
RunUntilIdle, to confirm it again.

+  ScopedDeviceOAuth2TokenServiceFactorySetUp scoped_setup;
+
+  DeviceOAuth2TokenService* token_service1 = NULL;
+  DeviceOAuth2TokenService* token_service2 = NULL;
+  DeviceOAuth2TokenService* token_service3 = NULL;
+  int counter = 0;
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service1, &counter));
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service2, &counter));
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service3, &counter));
+  // The token service will be returned asynchronously.
+  EXPECT_EQ(0, counter);
+  EXPECT_FALSE(token_service1);
+  EXPECT_FALSE(token_service2);
+  EXPECT_FALSE(token_service3);
+
+  // This lets FakeCryptohomeClient return the system salt.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(3, counter);
+  EXPECT_TRUE(token_service1);
+  EXPECT_TRUE(token_service2);
+  EXPECT_TRUE(token_service3);
+
+  // Make sure that token_service1,2,3 are the same one.
+  EXPECT_EQ(token_service1, token_service2);
+  EXPECT_EQ(token_service1, token_service3);
+}
+
+// Test a case where it failed to obtain the system salt.
+TEST_F(DeviceOAuth2TokenServiceFactoryTest, Get_NoSystemSalt) {
+  ScopedDeviceOAuth2TokenServiceFactorySetUp scoped_setup;
+  scoped_setup.fake_dbus_manager()->fake_cryptohome_client()->
+      set_system_salt(std::vector<uint8>());
+
+  DeviceOAuth2TokenService* token_service = NULL;
+  int counter = 0;
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service, &counter));
+  // The callback will be run asynchronously.
+  EXPECT_EQ(0, counter);
+  EXPECT_FALSE(token_service);
+
+  // This lets FakeCryptohomeClient return the system salt, which is empty.
+  // NULL should be returned to the callback in this case.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, counter);
+  EXPECT_FALSE(token_service);
+
+  // Try it again, but the result should remain the same (NULL returned).
+  DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&CopyTokenServiceAndCount, &token_service, &counter));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, counter);
+  EXPECT_FALSE(token_service);
+}
+
+}  // namespace chromeos