De-flakify MetricsServiceTest.RegisterSyntheticTrial.

The test relied on TimeTicks::Now() changing between
several lines of code. This is flaky, since on a
sufficiently fast machine, that may not end up the
case.

This CL fixes that cause of flakyness (and re-enables
the test) by making sure TimeTicks::Now() changes.
According to the docs for TimeTicks::Now(), its
resolution is ~1-15ms, so this shouldn't cause a
huge delay in the test.

Also changes the SyntheticTrialGroup constructor
to not take the start_time parameter since it gets
clobbered by the API anyway.

BUG=320433

Review URL: https://codereview.chromium.org/178383002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@253410 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 33 insertions, 20 deletions

diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index a3c0b83..4865ffd 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -356,10 +356,7 @@ bool SendSeparateInitialStabilityLog() {
 }  // namespace
 
 
-SyntheticTrialGroup::SyntheticTrialGroup(uint32 trial,
-                                         uint32 group,
-                                         base::TimeTicks start)
-    : start_time(start) {
+SyntheticTrialGroup::SyntheticTrialGroup(uint32 trial, uint32 group) {
   id.name = trial;
   id.group = group;
 }
@@ -1821,14 +1818,14 @@ void MetricsService::RegisterSyntheticFieldTrial(
     if (synthetic_trial_groups_[i].id.name == trial.id.name) {
       if (synthetic_trial_groups_[i].id.group != trial.id.group) {
         synthetic_trial_groups_[i].id.group = trial.id.group;
-        synthetic_trial_groups_[i].start_time = trial.start_time;
+        synthetic_trial_groups_[i].start_time = base::TimeTicks::Now();
       }
       return;
     }
   }
 
-  SyntheticTrialGroup trial_group(
-      trial.id.name, trial.id.group, base::TimeTicks::Now());
+  SyntheticTrialGroup trial_group = trial;
+  trial_group.start_time = base::TimeTicks::Now();
   synthetic_trial_groups_.push_back(trial_group);
 }
 
diff --git a/chrome/browser/metrics/metrics_service.h b/chrome/browser/metrics/metrics_service.h
index 199d6b7..5eb7fee 100644
--- a/chrome/browser/metrics/metrics_service.h
+++ b/chrome/browser/metrics/metrics_service.h
@@ -91,8 +91,7 @@ struct SyntheticTrialGroup {
   // This constructor is private specifically so as to control which code is
   // able to access it. New code that wishes to use it should be added as a
   // friend class.
-  SyntheticTrialGroup(uint32 trial, uint32 group, base::TimeTicks start);
-
+  SyntheticTrialGroup(uint32 trial, uint32 group);
 };
 
 class MetricsService
diff --git a/chrome/browser/metrics/metrics_service_unittest.cc b/chrome/browser/metrics/metrics_service_unittest.cc
index 49c069c..2d38c44 100644
--- a/chrome/browser/metrics/metrics_service_unittest.cc
+++ b/chrome/browser/metrics/metrics_service_unittest.cc
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/command_line.h"
+#include "base/threading/platform_thread.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
@@ -94,6 +95,14 @@ class MetricsServiceTest : public testing::Test {
     return testing_local_state_.Get();
   }
 
+  // Waits until base::TimeTicks::Now() no longer equals |value|. This should
+  // take between 1-15ms per the documented resolution of base::TimeTicks.
+  void WaitUntilTimeChanges(const base::TimeTicks& value) {
+    while (base::TimeTicks::Now() == value) {
+      base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
+    }
+  }
+
   // Returns true if there is a synthetic trial in the given vector that matches
   // the given trial name and trial group; returns false otherwise.
   bool HasSyntheticTrial(
@@ -266,23 +275,26 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) {
   EXPECT_EQ(1, uma_log.system_profile().stability().crash_count());
 }
 
-// Crashes on at least Mac and Linux.  http://crbug.com/320433
-TEST_F(MetricsServiceTest, DISABLED_RegisterSyntheticTrial) {
+TEST_F(MetricsServiceTest, RegisterSyntheticTrial) {
   MetricsService service;
 
   // Add two synthetic trials and confirm that they show up in the list.
   SyntheticTrialGroup trial1(metrics::HashName("TestTrial1"),
-                             metrics::HashName("Group1"),
-                             base::TimeTicks::Now());
+                             metrics::HashName("Group1"));
   service.RegisterSyntheticFieldTrial(trial1);
 
   SyntheticTrialGroup trial2(metrics::HashName("TestTrial2"),
-                             metrics::HashName("Group2"),
-                             base::TimeTicks::Now());
+                             metrics::HashName("Group2"));
   service.RegisterSyntheticFieldTrial(trial2);
+  // Ensure that time has advanced by at least a tick before proceeding.
+  WaitUntilTimeChanges(base::TimeTicks::Now());
 
   service.log_manager_.BeginLoggingWithLog(new MetricsLog("clientID", 1),
                                            MetricsLog::INITIAL_LOG);
+  // Save the time when the log was started (it's okay for this to be greater
+  // than the time recorded by the above call since it's used to ensure the
+  // value changes).
+  const base::TimeTicks begin_log_time = base::TimeTicks::Now();
 
   std::vector<chrome_variations::ActiveGroupId> synthetic_trials;
   service.GetCurrentSyntheticFieldTrials(&synthetic_trials);
@@ -290,10 +302,13 @@ TEST_F(MetricsServiceTest, DISABLED_RegisterSyntheticTrial) {
   EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "Group1"));
   EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial2", "Group2"));
 
+  // Ensure that time has advanced by at least a tick before proceeding.
+  WaitUntilTimeChanges(begin_log_time);
+
   // Change the group for the first trial after the log started.
+  // TODO(asvitkine): Assumption that this is > than BeginLoggingWithLog() time.
   SyntheticTrialGroup trial3(metrics::HashName("TestTrial1"),
-                             metrics::HashName("Group2"),
-                             base::TimeTicks::Now());
+                             metrics::HashName("Group2"));
   service.RegisterSyntheticFieldTrial(trial3);
   service.GetCurrentSyntheticFieldTrials(&synthetic_trials);
   EXPECT_EQ(1U, synthetic_trials.size());
@@ -301,14 +316,16 @@ TEST_F(MetricsServiceTest, DISABLED_RegisterSyntheticTrial) {
 
   // Add a new trial after the log started and confirm that it doesn't show up.
   SyntheticTrialGroup trial4(metrics::HashName("TestTrial3"),
-                             metrics::HashName("Group3"),
-                             base::TimeTicks::Now());
+                             metrics::HashName("Group3"));
   service.RegisterSyntheticFieldTrial(trial4);
   service.GetCurrentSyntheticFieldTrials(&synthetic_trials);
   EXPECT_EQ(1U, synthetic_trials.size());
   EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial2", "Group2"));
 
-  // Start a new log.
+  // Ensure that time has advanced by at least a tick before proceeding.
+  WaitUntilTimeChanges(base::TimeTicks::Now());
+
+  // Start a new log and ensure all three trials appear in it.
   service.log_manager_.FinishCurrentLog();
   service.log_manager_.BeginLoggingWithLog(new MetricsLog("clientID", 1),
                                            MetricsLog::ONGOING_LOG);