Refactor FileBasedPolicyProvider, introduce AsynchronousPolicyProvider.

chrome/browser/policy/file_based_policy_provider_unittest.cc

Index: chrome/browser/policy/file_based_policy_provider_unittest.cc
diff --git a/chrome/browser/policy/file_based_policy_provider_unittest.cc b/chrome/browser/policy/file_based_policy_provider_unittest.cc
index c78c5869675229064759713b11efaff799860227..c1d28f8d2ae5a3ad16b64964a58f09c6b01ee3ff 100644
--- a/chrome/browser/policy/file_based_policy_provider_unittest.cc
+++ b/chrome/browser/policy/file_based_policy_provider_unittest.cc
@@ -2,130 +2,78 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/file_util.h"

[Mattias Nissler (ping if slow), 2010/12/06 10:26:20]

not needed?

[danno, 2010/12/06 14:05:12]

Done.

+#include "base/scoped_temp_dir.h"

[Mattias Nissler (ping if slow), 2010/12/06 10:26:20]

not needed?

[danno, 2010/12/06 14:05:12]

Done.

+#include "chrome/browser/policy/asynchronous_policy_loader.h"
+#include "chrome/browser/policy/asynchronous_policy_provider.h"

[Mattias Nissler (ping if slow), 2010/12/06 10:26:20]

Needed? You get that include from file_based_policy_provider anyways.

[danno, 2010/12/06 14:05:12]

Done.

+#include "chrome/browser/policy/asynchronous_policy_test_base.h"
 #include "chrome/browser/policy/configuration_policy_pref_store.h"
+#include "chrome/browser/policy/configuration_policy_store_interface.h"

[Mattias Nissler (ping if slow), 2010/12/06 10:26:20]

needed? You include the mock below and AFAICS, you only use that.

[danno, 2010/12/06 14:05:12]

Done.

 #include "chrome/browser/policy/file_based_policy_provider.h"
+#include "chrome/browser/policy/mock_configuration_policy_provider.h"
+#include "chrome/browser/policy/mock_configuration_policy_store.h"
+#include "chrome/common/policy_constants.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
+using testing::InSequence;
 using testing::Mock;

[Mattias Nissler (ping if slow), 2010/12/06 10:26:20]

you don't use this.

[danno, 2010/12/06 14:05:12]

Done.

+using testing::Return;
 
 namespace policy {
 
-// Shorter reload intervals for testing FileBasedPolicyLoader.
-const int kSettleIntervalSecondsForTesting = 0;
-const int kReloadIntervalMinutesForTesting = 1;
-
-// A delegate for testing that can feed arbitrary information to the loader.
-class TestDelegate : public FileBasedPolicyProvider::Delegate {
- public:
-  TestDelegate()
-      : FileBasedPolicyProvider::Delegate(FilePath(FILE_PATH_LITERAL("fake"))) {
-  }
-
-  // FileBasedPolicyProvider::Delegate implementation:
-  virtual DictionaryValue* Load() {
-    return static_cast<DictionaryValue*>(dict_.DeepCopy());
-  }
-
-  virtual base::Time GetLastModification() {
-    return last_modification_;
-  }
-
-  DictionaryValue* dict() { return &dict_; }
-  void set_last_modification(const base::Time& last_modification) {
-    last_modification_ = last_modification;
-  }
-
- private:
-  DictionaryValue dict_;
-  base::Time last_modification_;
-};
-
-// A mock provider that allows us to capture reload notifications.
-class MockPolicyProvider : public ConfigurationPolicyProvider,
-                           public base::SupportsWeakPtr<MockPolicyProvider> {
+class FileBasedPolicyProviderDelegateMock
+    : public FileBasedPolicyProvider::ProviderDelegate {
  public:
-  explicit MockPolicyProvider()
-      : ConfigurationPolicyProvider(
-          ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList()) {
-  }
-
-  virtual bool Provide(ConfigurationPolicyStoreInterface* store) {
-    return true;
-  }
-
-  MOCK_METHOD0(NotifyStoreOfPolicyChange, void());
+  FileBasedPolicyProviderDelegateMock()
+      : FileBasedPolicyProvider::ProviderDelegate(FilePath()) {}
+  MOCK_METHOD0(Load, DictionaryValue*());
+  MOCK_METHOD0(GetLastModification, base::Time());
 };
 
-class FileBasedPolicyLoaderTest : public testing::Test {
- protected:
-  FileBasedPolicyLoaderTest()
-      : ui_thread_(BrowserThread::UI, &loop_),
-        file_thread_(BrowserThread::FILE, &loop_) {}
-
-  virtual void TearDown() {
-    loop_.RunAllPending();
-  }
-
-  MessageLoop loop_;
-
- private:
-  BrowserThread ui_thread_;
-  BrowserThread file_thread_;
-};
-
-TEST_F(FileBasedPolicyLoaderTest, BasicLoad) {
-  TestDelegate* test_delegate = new TestDelegate;
-  test_delegate->dict()->SetString("HomepageLocation", "http://www.google.com");
-
-  scoped_refptr<FileBasedPolicyLoader> loader(
-      new FileBasedPolicyLoader(base::WeakPtr<FileBasedPolicyProvider>(),
-                                test_delegate,
-                                kSettleIntervalSecondsForTesting,
-                                kReloadIntervalMinutesForTesting));
-  scoped_ptr<DictionaryValue> policy(loader->GetPolicy());
-  EXPECT_TRUE(policy.get());
-  EXPECT_EQ(1U, policy->size());
-
-  std::string str_value;
-  EXPECT_TRUE(policy->GetString("HomepageLocation", &str_value));
-  EXPECT_EQ("http://www.google.com", str_value);
-
-  loader->Stop();
+TEST_F(AsynchronousPolicyTestBase, ProviderInit) {
+  base::Time last_modified;
+  scoped_ptr<FileBasedPolicyProviderDelegateMock> provider_delegate(
+      new FileBasedPolicyProviderDelegateMock);
+  EXPECT_CALL(*provider_delegate, GetLastModification()).WillRepeatedly(
+      Return(last_modified));
+  InSequence s;
+  EXPECT_CALL(*provider_delegate, Load()).WillOnce(Return(
+      new DictionaryValue));
+  DictionaryValue* policies = new DictionaryValue();
+  policies->SetBoolean(policy::key::kSyncDisabled, true);
+  EXPECT_CALL(*provider_delegate, Load()).WillOnce(Return(policies));

[Mattias Nissler (ping if slow), 2010/12/06 10:26:20]

Why do you install two expected calls on provider_delegate? Can you put a
comment explaining that?

[danno, 2010/12/06 14:05:12]

Done.

+  provider_.reset(new FileBasedPolicyProvider(
+      ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList(),
+      provider_delegate.release()));
+  EXPECT_CALL(*store_, Apply(policy::kPolicySyncDisabled, _)).Times(1);
+  loop_.RunAllPending();
+  provider_->Provide(store_.get());
 }
 
-TEST_F(FileBasedPolicyLoaderTest, TestRefresh) {
-  MockPolicyProvider provider;
-  TestDelegate* test_delegate = new TestDelegate;
-
-  scoped_refptr<FileBasedPolicyLoader> loader(
-      new FileBasedPolicyLoader(provider.AsWeakPtr(),
-                                test_delegate,
-                                kSettleIntervalSecondsForTesting,
-                                kReloadIntervalMinutesForTesting));
-  scoped_ptr<DictionaryValue> policy(loader->GetPolicy());
-  EXPECT_TRUE(policy.get());
-  EXPECT_EQ(0U, policy->size());
-
-  test_delegate->dict()->SetString("HomepageLocation", "http://www.google.com");
-
-  EXPECT_CALL(provider, NotifyStoreOfPolicyChange()).Times(1);
-  loader->OnFilePathChanged(FilePath(FILE_PATH_LITERAL("fake")));
-
-  // Run the loop. The refresh should be handled immediately since the settle
-  // interval has been disabled.
+TEST_F(AsynchronousPolicyTestBase, ProviderRefresh) {
+  base::Time last_modified;
+  scoped_ptr<FileBasedPolicyProviderDelegateMock> provider_delegate(
+      new FileBasedPolicyProviderDelegateMock);
+  EXPECT_CALL(*provider_delegate, GetLastModification()).WillRepeatedly(
+      Return(last_modified));
+  InSequence s;
+  EXPECT_CALL(*provider_delegate, Load()).WillOnce(Return(
+      new DictionaryValue));
+  DictionaryValue* policies = new DictionaryValue();
+  policies->SetBoolean(policy::key::kSyncDisabled, true);
+  EXPECT_CALL(*provider_delegate, Load()).WillOnce(Return(policies));
+  FileBasedPolicyProvider* file_based_provider =
+      new FileBasedPolicyProvider(
+          ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList(),
+          provider_delegate.release());
+  provider_.reset(file_based_provider);
+  EXPECT_CALL(*store_, Apply(policy::kPolicySyncDisabled, _)).Times(1);
   loop_.RunAllPending();
-  Mock::VerifyAndClearExpectations(&provider);
-
-  policy.reset(loader->GetPolicy());
-  EXPECT_TRUE(policy.get());
-  EXPECT_EQ(1U, policy->size());
-
-  std::string str_value;
-  EXPECT_TRUE(policy->GetString("HomepageLocation", &str_value));
-  EXPECT_EQ("http://www.google.com", str_value);
-
-  loader->Stop();
+  file_based_provider->loader()->Reload();
+  loop_.RunAllPending();
+  provider_->Provide(store_.get());

[Mattias Nissler (ping if slow), 2010/12/06 10:26:20]

Does this trigger the second EXPECT_CALL(*provider_delegate, ...) from above? If
so, can we move it here so it's easier to understand our expectations?

[danno, 2010/12/06 14:05:12]

Done.

 }
 
 }  // namespace policy