Remove the hash fields from FieldTrials.

chrome/common/metrics/experiments_helper_unittest.cc

 // Copyright (c) 2012 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.
 
 // Tests for the Experiment Helpers.
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/metrics/field_trial.h"
 #include "base/time.h"
 #include "chrome/common/metrics/experiments_helper.h"
 #include "content/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#include <set>
+
 namespace {
 
 // Convenience helper to retrieve the GoogleExperimentID for a FieldTrial. Note
 // that this will do the group assignment in |trial| if not already done.
 experiments_helper::GoogleExperimentID GetIDForTrial(base::FieldTrial* trial) {
   return experiments_helper::GetGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial->name(),
-                                        trial->group_name()));
+      testing::TestMakeNameGroupId(trial->name(), trial->group_name()));
 }
 
 }  // namespace
 
 class ExperimentsHelperTest : public ::testing::Test {
  public:
   ExperimentsHelperTest() {
     // Since the API can only be called on the UI thread, we have to fake that
     // we're on it.
     ui_thread_.reset(new content::TestBrowserThread(
         content::BrowserThread::UI, &message_loop_));
 
     base::Time now = base::Time::NowFromSystemTime();
     base::TimeDelta oneYear = base::TimeDelta::FromDays(365);
     base::Time::Exploded exploded;
 
     base::Time next_year_time = now + oneYear;
     next_year_time.LocalExplode(&exploded);
     next_year_ = exploded.year;
   }
 
  protected:
   int next_year_;
 
  private:
   MessageLoop message_loop_;
   scoped_ptr<content::TestBrowserThread> ui_thread_;
 };
 
+TEST_F(ExperimentsHelperTest, HashName) {
+  // Make sure hashing is stable on all platforms.
+  struct {
+    const char* name;
+    uint32 hash_value;
+  } known_hashes[] = {
+    {"a", 937752454u},
+    {"1", 723085877u},
+    {"Trial Name", 2713117220u},
+    {"Group Name", 3201815843u},
+    {"My Favorite Experiment", 3722155194u},
+    {"My Awesome Group Name", 4109503236u},
+    {"abcdefghijklmonpqrstuvwxyz", 787728696u},
+    {"0123456789ABCDEF", 348858318U}
+  };
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(known_hashes); ++i) {
+    EXPECT_EQ(known_hashes[i].hash_value,
+              testing::TestHashName(known_hashes[i].name));
+  }
+}
+
+TEST_F(ExperimentsHelperTest, GetFieldTrialSelectedGroups) {
+  std::string trial_one("trial one");
+  std::string group_one("group one");
+  std::string trial_two("trial two");
+  std::string group_two("group two");
+
+  base::FieldTrial::SelectedGroups selected_groups;
+  base::FieldTrial::SelectedGroup selected_group;
+  selected_group.trial = trial_one;
+  selected_group.group = group_one;
+  selected_groups.push_back(selected_group);
+
+  selected_group.trial = trial_two;
+  selected_group.group = group_two;
+  selected_groups.push_back(selected_group);
+
+  // Create our expected groups of IDs.
+  std::set<experiments_helper::NameGroupId,
+      experiments_helper::NameGroupIdCompare> expected_groups;
+  expected_groups.insert(testing::TestMakeNameGroupId(trial_one, group_one));
+  expected_groups.insert(testing::TestMakeNameGroupId(trial_two, group_two));
+
+  std::vector<experiments_helper::NameGroupId> name_group_ids;
+  testing::TestGetFieldTrialNameGroupIdsForSelectedGroups(selected_groups,
+                                                         &name_group_ids);
+  EXPECT_EQ(2U, name_group_ids.size());
+  for (size_t i = 0; i < name_group_ids.size(); ++i) {
+    std::set<experiments_helper::NameGroupId,
+        experiments_helper::NameGroupIdCompare>::iterator it =
+            expected_groups.find(name_group_ids[i]);
+    EXPECT_NE(it, expected_groups.end());
+    expected_groups.erase(it);

[MAD, 2012/04/26 19:40:13]

I think it's dangerous for some container classes to erase an entity with an
iterator and then increment this iterator... It doesn't always work, you may
skip elements...

So that might be another good incentive to use the trick I propose in the
previous comment... Have you walked this code in the debugger to make sure you
go through the loop twice?

[SteveT, 2012/04/26 21:06:03]

Oops, maybe it's not clear, but there are actually two containers here... a
vector of produced NameGroupIds, which we iterate over, and a set of expected
NameGroupIds, which we remove from to verify that we see all expected
NameGroupIds (the set is expected to be empty if we successfully see every
NameGroupId, which we check below).

So basically the iterator we erase isn't the same one we're looping over.

Does this make sense, or is this pattern a bit too convoluted and you'd rather I
use your trick?

[MAD, 2012/04/27 17:33:22]

OK, that's fine... The trick I propose would prevent the need to expose the
compare function publicly, but that's fine... I think it might be more obvious
with a C style array, but that might just be me... So I'm OK with your
solution... Except...

To resolve the potential ambiguity, you could use a more meaningful name for the
set iterator... :-)

Thanks!

[SteveT, 2012/04/27 17:59:33]

Good point. I've renamed it to expected_group, which is more clear. Also noticed
that I have to renaming name_group_ids here. Thanks!

+  }
+  EXPECT_EQ(0U, expected_groups.size());
+}
+
 // Test that if the trial is immediately disabled, GetGoogleExperimentID just
 // returns the empty ID.
 TEST_F(ExperimentsHelperTest, DisableImmediately) {
   int default_group_number = -1;
   scoped_refptr<base::FieldTrial> trial(
       base::FieldTrialList::FactoryGetFieldTrial("trial", 100, "default",
                                                  next_year_, 12, 12,
                                                  &default_group_number));
   trial->Disable();
 
   ASSERT_EQ(default_group_number, trial->group());
   ASSERT_EQ(experiments_helper::kEmptyGoogleExperimentID,
             GetIDForTrial(trial.get()));
 }
 
 // Test that successfully associating the FieldTrial with some ID, and then
 // disabling the FieldTrial actually makes GetGoogleExperimentID correctly
 // return the empty ID.
 TEST_F(ExperimentsHelperTest, DisableAfterInitialization) {
   const std::string default_name = "default";
   const std::string non_default_name = "non_default";
 
   scoped_refptr<base::FieldTrial> trial(
       base::FieldTrialList::FactoryGetFieldTrial("trial", 100, default_name,
                                                  next_year_, 12, 12, NULL));
   trial->AppendGroup(non_default_name, 100);
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial->name(), default_name), 123);
+      testing::TestMakeNameGroupId(trial->name(), default_name), 123);
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial->name(), non_default_name), 456);
+      testing::TestMakeNameGroupId(trial->name(), non_default_name), 456);
   ASSERT_EQ(non_default_name, trial->group_name());
   ASSERT_EQ(456U, GetIDForTrial(trial.get()));
   trial->Disable();
   ASSERT_EQ(default_name, trial->group_name());
   ASSERT_EQ(123U, GetIDForTrial(trial.get()));
 }
 
 // Test various successful association cases.
 TEST_F(ExperimentsHelperTest, AssociateGoogleExperimentID) {
   const std::string default_name1 = "default1";
   scoped_refptr<base::FieldTrial> trial_true(
       base::FieldTrialList::FactoryGetFieldTrial("d1", 10, default_name1,
                                                  next_year_, 12, 31, NULL));
   const std::string winner = "TheWinner";
   int winner_group = trial_true->AppendGroup(winner, 10);
 
   // Set GoogleExperimentIDs so we can verify that they were chosen correctly.
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial_true->name(), default_name1),
-      123);
+      testing::TestMakeNameGroupId(trial_true->name(), default_name1), 123);
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial_true->name(), winner),
-      456);
+      testing::TestMakeNameGroupId(trial_true->name(), winner), 456);
 
   EXPECT_EQ(winner_group, trial_true->group());
   EXPECT_EQ(winner, trial_true->group_name());
   EXPECT_EQ(456U, GetIDForTrial(trial_true.get()));
 
   const std::string default_name2 = "default2";
   scoped_refptr<base::FieldTrial> trial_false(
       base::FieldTrialList::FactoryGetFieldTrial("d2", 10, default_name2,
                                                  next_year_, 12, 31, NULL));
   const std::string loser = "ALoser";
   int loser_group = trial_false->AppendGroup(loser, 0);
 
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial_false->name(), default_name2),
-      123);
+      testing::TestMakeNameGroupId(trial_false->name(), default_name2), 123);
   experiments_helper::AssociateGoogleExperimentID(
-      base::FieldTrial::MakeNameGroupId(trial_false->name(), loser),
-      456);
+      testing::TestMakeNameGroupId(trial_false->name(), loser), 456);
 
   EXPECT_NE(loser_group, trial_false->group());
   EXPECT_EQ(123U, GetIDForTrial(trial_false.get()));
 }
 
 // Test that not associating a FieldTrial with any IDs ensure that the empty ID
 // will be returned.
 TEST_F(ExperimentsHelperTest, NoAssociation) {
   const std::string default_name = "default";
   scoped_refptr<base::FieldTrial> no_id_trial(
       base::FieldTrialList::FactoryGetFieldTrial("d3", 10, default_name,
                                                  next_year_, 12, 31, NULL));
   const std::string winner = "TheWinner";
   int winner_group = no_id_trial->AppendGroup(winner, 10);
 
   // Ensure that despite the fact that a normal winner is elected, it does not
   // have a valid GoogleExperimentID associated with it.
   EXPECT_EQ(winner_group, no_id_trial->group());
   EXPECT_EQ(winner, no_id_trial->group_name());
   EXPECT_EQ(experiments_helper::kEmptyGoogleExperimentID,
             GetIDForTrial(no_id_trial.get()));
 }