Fix a race condition in preference metric reporting.

chrome/browser/prefs/pref_hash_calculator_unittest.cc

Index: chrome/browser/prefs/pref_hash_calculator_unittest.cc
diff --git a/chrome/browser/prefs/pref_hash_calculator_unittest.cc b/chrome/browser/prefs/pref_hash_calculator_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bea87a4958cc48120a44d9279c5d544ef5ddfa52
--- /dev/null
+++ b/chrome/browser/prefs/pref_hash_calculator_unittest.cc
@@ -0,0 +1,87 @@
+// 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_calculator.h"
+
+#include <string>
+
+#include "base/values.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+TEST(PrefHashCalculatorTest, TestCurrentAlgorithm) {
+  base::StringValue string_value_1("string value 1");
+  base::StringValue string_value_2("string value 2");
+  base::DictionaryValue dictionary_value_1;
+  dictionary_value_1.SetInteger("int value", 1);
+  dictionary_value_1.Set("nested empty map", new DictionaryValue);
+  base::DictionaryValue dictionary_value_1_equivalent;
+  dictionary_value_1_equivalent.SetInteger("int value", 1);
+  base::DictionaryValue dictionary_value_2;
+  dictionary_value_2.SetInteger("int value", 2);
+
+  PrefHashCalculator calc1("seed1", "deviceid");
+  PrefHashCalculator calc1_dup("seed1", "deviceid");
+  PrefHashCalculator calc2("seed2", "deviceid");
+  PrefHashCalculator calc3("seed1", "deviceid2");
+
+  // Two calculators with same seed produce same hash.
+  ASSERT_EQ(calc1.Calculate("pref_path", &string_value_1),
+            calc1_dup.Calculate("pref_path", &string_value_1));
+  ASSERT_EQ(PrefHashCalculator::VALID,
+            calc1_dup.Validate(
+                "pref_path",
+                &string_value_1,
+                calc1.Calculate("pref_path", &string_value_1)));
+
+  // Different seeds, different hashes.
+  ASSERT_NE(calc1.Calculate("pref_path", &string_value_1),
+            calc2.Calculate("pref_path", &string_value_1));
+  ASSERT_EQ(PrefHashCalculator::INVALID,
+            calc2.Validate(
+                "pref_path",
+                &string_value_1,
+                calc1.Calculate("pref_path", &string_value_1)));
+
+  // Different device IDs, different hashes.
+  ASSERT_NE(calc1.Calculate("pref_path", &string_value_1),
+            calc3.Calculate("pref_path", &string_value_1));
+
+  // Different values, different hashes.
+  ASSERT_NE(calc1.Calculate("pref_path", &string_value_1),
+            calc1.Calculate("pref_path", &string_value_2));
+
+  // Different paths, different hashes.
+  ASSERT_NE(calc1.Calculate("pref_path", &string_value_1),
+            calc1.Calculate("pref_path_2", &string_value_1));
+
+  // Works for dictionaries.
+  ASSERT_EQ(calc1.Calculate("pref_path", &dictionary_value_1),
+            calc1.Calculate("pref_path", &dictionary_value_1));
+  ASSERT_NE(calc1.Calculate("pref_path", &dictionary_value_1),
+            calc1.Calculate("pref_path", &dictionary_value_2));
+
+  // Empty dictionary children are pruned.
+  ASSERT_EQ(calc1.Calculate("pref_path", &dictionary_value_1),
+            calc1.Calculate("pref_path", &dictionary_value_1_equivalent));
+
+  // NULL value is supported.
+  ASSERT_EQ(calc1.Calculate("pref_path", NULL),
+            calc1.Calculate("pref_path", NULL));

[gab, 2013/12/06 17:23:38]

What's the value of checking the same call twice here?

I think ASSERT_FALSE(calc1.Calculate("pref_path", NULL).empty()); is a good test
here to make sure the hash is calculated AND not empty.

[erikwright (departed), 2013/12/09 17:59:40]

Done.

+}

[gab, 2013/12/06 17:23:38]

I would also like to see tests with hardcoded hashes for each Value type to
ensure future modifications of the algorithm do not start generating different
hashes.

Ideally these would re-use hashes that were hardcoded in the old tests to
confirm that this new implementation also doesn't start generating new hashes.

[erikwright (departed), 2013/12/09 17:59:40]

Done.

+
+TEST(PrefHashCalculatorTest, TestLegacyAlgorithm) {
+  DictionaryValue dict;
+  dict.Set("a", new StringValue("foo"));
+  dict.Set("d", new StringValue("bad"));
+  dict.Set("b", new StringValue("bar"));
+  dict.Set("c", new StringValue("baz"));
+
+  // 32 NULL bytes is the seed that was used to generate the legacy hash.

[gab, 2013/12/06 17:23:38]

Why 32 NULL bytes?! Won't an empty string do the same?

[erikwright (departed), 2013/12/06 19:00:47]

Like the comment says. That is what the old test used. In order to prove that
the algorithm hasn't changed and generates the same hash with the same seed, I
am using the same seed.

+  EXPECT_EQ(PrefHashCalculator::VALID_LEGACY,
+            PrefHashCalculator(std::string(32u, 0), "deviceid").Validate(
+                "pref.path1",
+                &dict,
+                "C503FB7C65EEFD5C07185F616A0AA679"
+                "23C069909933F362022B1F187E73E9A2"));

[gab, 2013/12/06 17:23:38]

I would prefer having this all on one line; it wasn't obvious to me at first
that this was the continuation of the above string (i.e., I thought "why 2
hashes?").

Please put it in a static const variable above.

[erikwright (departed), 2013/12/09 17:59:40]

Done.

+}