Fix a race condition in preference metric reporting.

chrome/browser/prefs/pref_hash_filter_unittest.cc

Index: chrome/browser/prefs/pref_hash_filter_unittest.cc
diff --git a/chrome/browser/prefs/pref_hash_filter_unittest.cc b/chrome/browser/prefs/pref_hash_filter_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b4c27a7ef0bb5aea5307cf27918692a8c4607c36
--- /dev/null
+++ b/chrome/browser/prefs/pref_hash_filter_unittest.cc
@@ -0,0 +1,204 @@
+// 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/prefs/pref_hash_filter.h"
+
+#include <map>
+#include <set>
+#include <string>
+#include <utility>
+#include "base/basictypes.h"

[gab, 2013/12/11 18:37:14]

nit: +empty line above

[erikwright (departed), 2013/12/12 22:02:57]

Done.

+#include "base/compiler_specific.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
+#include "chrome/browser/prefs/pref_hash_store.h"
+

[gab, 2013/12/11 18:37:14]

nit: remove empty line

[erikwright (departed), 2013/12/12 22:02:57]

Done.

+#include "testing/gtest/include/gtest/gtest.h"
+
+class MockPrefHashStore : public PrefHashStore {
+ public:
+  MockPrefHashStore() {}
+
+  void SetCheckResult(const std::string& path,
+                      PrefHashStore::ValueState result);
+
+  const std::set<std::string>& checked_paths() const {
+    return checked_paths_;
+  }
+
+  const std::set<std::string>& stored_paths() const {
+    return stored_paths_;
+  }
+
+  std::string checked_value(const std::string& path) const {
+    std::map<std::string, std::string>::iterator value =
+        checked_values_.find(path);
+    if (value != checked_values_.end())
+      return value->second;
+    return std::string();
+  }
+
+  std::string stored_value(const std::string& path) const {
+    std::map<std::string, std::string>::const_iterator value =
+        stored_values_.find(path);
+    if (value != stored_values_.end())
+      return value->second;
+    return std::string();
+  }
+
+  virtual PrefHashStore::ValueState CheckValue(
+      const std::string& path, const base::Value* value) const OVERRIDE;
+  virtual void StoreValue(const std::string& path,
+                          const base::Value* new_value) OVERRIDE;
+ private:
+  std::map<std::string, PrefHashStore::ValueState>
+      check_results_;

[gab, 2013/12/11 18:37:14]

nit: doesn't this fit above?

[erikwright (departed), 2013/12/12 22:02:57]

Done.

+  mutable std::set<std::string> checked_paths_;
+  std::set<std::string> stored_paths_;
+  mutable std::map<std::string, std::string> checked_values_;
+  std::map<std::string, std::string> stored_values_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockPrefHashStore);
+};
+
+void MockPrefHashStore::SetCheckResult(
+    const std::string& path, PrefHashStore::ValueState result) {
+  check_results_.insert(std::make_pair(path, result));
+}
+
+PrefHashStore::ValueState MockPrefHashStore::CheckValue(
+    const std::string& path, const base::Value* value) const {
+  EXPECT_TRUE(checked_paths_.insert(path).second);
+  std::string as_string;
+  if (value)
+    EXPECT_TRUE(value->GetAsString(&as_string));
+  checked_values_.insert(std::make_pair(path, as_string));
+  std::map<std::string, PrefHashStore::ValueState>::const_iterator result =
+      check_results_.find(path);
+  if (result != check_results_.end())
+    return result->second;
+  return PrefHashStore::UNCHANGED;
+}
+
+void MockPrefHashStore::StoreValue(const std::string& path,
+                                   const base::Value* new_value) {
+  std::string as_string;
+  if (new_value)
+    EXPECT_TRUE(new_value->GetAsString(&as_string));
+  stored_paths_.insert(path);
+  stored_values_.insert(std::make_pair(path, as_string));
+}
+
+scoped_ptr<PrefHashFilter> CreatePrefHashFilter(

[gab, 2013/12/11 18:37:14]

+Comment (here and on methods above in general).

[erikwright (departed), 2013/12/12 22:02:57]

Done.

+    MockPrefHashStore** mock_pref_hash_store) {
+  scoped_ptr<MockPrefHashStore> temp_mock_pref_hash_store(
+      new MockPrefHashStore);
+  if (mock_pref_hash_store)
+    *mock_pref_hash_store = temp_mock_pref_hash_store.get();
+  return scoped_ptr<PrefHashFilter>(
+      new PrefHashFilter(temp_mock_pref_hash_store.Pass()));
+}
+
+class PrefHashFilterTest : public testing::Test {

[gab, 2013/12/11 18:37:14]

I've generally seen ::testing::Test instead of testing::Test and think you
should change this here for consistency.

[erikwright (departed), 2013/12/12 22:02:57]

I counted. Without :: is more widespread by a factor of 6:1

+ public:
+  PrefHashFilterTest() : mock_pref_hash_store_(NULL) {}
+
+  virtual void SetUp() OVERRIDE {
+    // Capture the name of one tracked pref.
+    MockPrefHashStore* temp_hash_store = NULL;
+    scoped_ptr<PrefHashFilter> temp_filter =
+        CreatePrefHashFilter(&temp_hash_store);
+    temp_filter->FilterOnLoad(&pref_store_contents_);
+    ASSERT_FALSE(temp_hash_store->checked_paths().empty());
+    tracked_path_ = *temp_hash_store->checked_paths().begin();

[gab, 2013/12/11 18:37:14]

I find this overly complex just to get the name of a tracked_path :(.

Could we perhaps expose kTrackedPrefs in an internals:: namespace?

Feels to would be useful to have better tests below (i.e., ASSERT exacts sizes
on checks instead of "greater than 0"...).

[erikwright (departed), 2013/12/12 22:02:57]

I actually prefer this approach because it's not tied to the values that are
actually being tracked. If we so choose, we could explicitly look for some that
we expect to be tracked, but doing so by referencing kTrackedPrefs is a bit
circular.

DCHECKing exact sizes is just a painful thing to manually update when new values
are added.

[gab, 2013/12/12 22:56:35]

How about adding a test-only constructor that takes in the list of tracked
preference? Then you can pass in whatever you want as a list of test tracked
prefs.

I'm thinking of doing that for the CL I referred to earlier; especially since
the kTrackedPrefs are going to hold metadata in my CL.

(I'm happy with doing it in my CL and letting you commit this as-is FWIW)

+
+    // Construct a PrefHashFilter and MockPrefHashStore for the test.
+    pref_hash_filter_ = CreatePrefHashFilter(&mock_pref_hash_store_);
+  }
+
+ protected:
+  std::string tracked_path_;
+  MockPrefHashStore* mock_pref_hash_store_;
+  base::DictionaryValue pref_store_contents_;
+  scoped_ptr<PrefHashFilter> pref_hash_filter_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrefHashFilterTest);
+};
+
+TEST_F(PrefHashFilterTest, EmptyAndUnchanged) {
+  pref_hash_filter_->FilterOnLoad(&pref_store_contents_);
+  // More than 0 paths checked.
+  ASSERT_LT(0u, mock_pref_hash_store_->checked_paths().size());
+  // No paths stored, since they all return |UNCHANGED|.
+  ASSERT_EQ(0u, mock_pref_hash_store_->stored_paths().size());
+  // Since there was nothing in |pref_store_contents_| the checked value should
+  // have been NULL.
+  ASSERT_EQ(std::string(), mock_pref_hash_store_->checked_value(tracked_path_));

[gab, 2013/12/11 18:37:14]

It is important that there be a difference between NULL and empty string; please
make |tracked_path_| a pointer to able to check this.

[erikwright (departed), 2013/12/12 22:02:57]

It's a pain for memory safety reasons to maintain the pointers, so I've added a
special value for NULLs.

+}
+
+TEST_F(PrefHashFilterTest, FilterUpdate) {
+  base::StringValue string_value("string value");
+  pref_hash_filter_->FilterUpdate(tracked_path_, &string_value);
+  // One path should be stored.
+  ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths().size());
+  ASSERT_EQ(tracked_path_, *mock_pref_hash_store_->stored_paths().begin());
+  ASSERT_EQ("string value", mock_pref_hash_store_->stored_value(tracked_path_));
+}
+
+TEST_F(PrefHashFilterTest, EmptyAndUnknown){
+  mock_pref_hash_store_->SetCheckResult(tracked_path_,
+                                        PrefHashStore::UNKNOWN_VALUE);
+  pref_hash_filter_->FilterOnLoad(&pref_store_contents_);
+  ASSERT_LT(0u, mock_pref_hash_store_->checked_paths().size());
+  ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths().size());
+  ASSERT_EQ(tracked_path_, *mock_pref_hash_store_->stored_paths().begin());
+  ASSERT_EQ(std::string(), mock_pref_hash_store_->stored_value(tracked_path_));
+}
+
+TEST_F(PrefHashFilterTest, InitialValueUnknown) {
+  pref_store_contents_.Set(tracked_path_,
+                           new base::StringValue("string value"));
+
+  mock_pref_hash_store_->SetCheckResult(tracked_path_,
+                                        PrefHashStore::UNKNOWN_VALUE);
+  pref_hash_filter_->FilterOnLoad(&pref_store_contents_);
+  ASSERT_LT(0u, mock_pref_hash_store_->checked_paths().size());
+  ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths().size());
+  ASSERT_EQ(tracked_path_, *mock_pref_hash_store_->stored_paths().begin());
+  ASSERT_EQ("string value", mock_pref_hash_store_->stored_value(tracked_path_));
+}
+
+TEST_F(PrefHashFilterTest, InitialValueChanged) {
+  pref_store_contents_.Set(tracked_path_,
+                           new base::StringValue("string value"));
+
+  mock_pref_hash_store_->SetCheckResult(tracked_path_,
+                                        PrefHashStore::CHANGED);
+  pref_hash_filter_->FilterOnLoad(&pref_store_contents_);
+  ASSERT_LT(0u, mock_pref_hash_store_->checked_paths().size());
+  ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths().size());
+  ASSERT_EQ(tracked_path_, *mock_pref_hash_store_->stored_paths().begin());
+  ASSERT_EQ("string value", mock_pref_hash_store_->stored_value(tracked_path_));
+}
+
+TEST_F(PrefHashFilterTest, EmptyCleared) {
+  mock_pref_hash_store_->SetCheckResult(tracked_path_,
+                                        PrefHashStore::CLEARED);
+  pref_hash_filter_->FilterOnLoad(&pref_store_contents_);
+  ASSERT_LT(0u, mock_pref_hash_store_->checked_paths().size());
+  ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths().size());
+  ASSERT_EQ(tracked_path_, *mock_pref_hash_store_->stored_paths().begin());
+  ASSERT_EQ(std::string(), mock_pref_hash_store_->stored_value(tracked_path_));
+}
+
+TEST_F(PrefHashFilterTest, EmptyMigrated) {
+  mock_pref_hash_store_->SetCheckResult(tracked_path_,
+                                        PrefHashStore::MIGRATED);
+  pref_hash_filter_->FilterOnLoad(&pref_store_contents_);
+  ASSERT_LT(0u, mock_pref_hash_store_->checked_paths().size());
+  ASSERT_EQ(1u, mock_pref_hash_store_->stored_paths().size());
+  ASSERT_EQ(tracked_path_, *mock_pref_hash_store_->stored_paths().begin());
+  ASSERT_EQ(std::string(), mock_pref_hash_store_->stored_value(tracked_path_));
+}