Improve the no-op reconcilor so that it doesn't "hang" after the first...

Improve the no-op reconcilor so that it doesn't "hang" after the first execution, but instead successfully completes the reconciliation.
Improve the UMAHistogramHelper so that it can nicely take snapshots between tests, allowing the test writer to easily test ONLY the changes logged in "this" test.
Write a new unit test that executes the reconcilor twice, and make all reconcilor unit tests execute with and without the flag.

BUG=357693
TBR=phajdan.jr@chromium.org

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

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

base/test/statistics_delta_reader.h

@@ -17,6 +17,10 @@ class HistogramSamples;
 
 // This class acts as a differential reader for histogram samples, enabling
 // tests to check that metrics were recorded as they should be.
+//
+// This class is DEPRECATED.
+// TODO(mlerman): Remove all references to this class with UMAHistogramHelper
+// references. crbug.com/384011
 class StatisticsDeltaReader {
  public:
   StatisticsDeltaReader();

chrome/test/base/uma_histogram_helper.cc

@@ -11,6 +11,27 @@
 #include "content/public/browser/histogram_fetcher.h"
 
 UMAHistogramHelper::UMAHistogramHelper() {
+  base::StatisticsRecorder::Initialize();
+}
+
+UMAHistogramHelper::~UMAHistogramHelper() {
+}
+
+void UMAHistogramHelper::PrepareSnapshot(const char* const histogram_names[],
+                                         size_t num_histograms) {
+  for (size_t i = 0; i < num_histograms; ++i) {
+    std::string histogram_name = histogram_names[i];
+
+    base::HistogramBase* histogram =
+        base::StatisticsRecorder::FindHistogram(histogram_name);
+    // If there is no histogram present, then don't record a snapshot. The logic
+    // in the Expect* methods will act to treat no histogram equivalent to
+    // samples with zeros.
+    if (histogram) {
+      histogram_snapshots[histogram_name] =
+          make_linked_ptr(histogram->SnapshotSamples().release());
+    }
+  }
 }
 
 void UMAHistogramHelper::Fetch() {
@@ -82,18 +103,25 @@ void UMAHistogramHelper::CheckBucketCount(
     base::HistogramBase::Sample sample,
     base::HistogramBase::Count expected_count,
     base::HistogramSamples& samples) {
-  EXPECT_EQ(expected_count, samples.GetCount(sample))
+  int actual_count = samples.GetCount(sample);
+  if (histogram_snapshots.count(name))
+    actual_count -= histogram_snapshots[name]->GetCount(sample);
+  EXPECT_EQ(expected_count, actual_count)
       << "Histogram \"" << name
       << "\" does not have the right number of samples (" << expected_count
-      << ") in the expected bucket (" << sample << ").";
+      << ") in the expected bucket (" << sample << "). It has (" << actual_count
+      << ").";
 }
 
 void UMAHistogramHelper::CheckTotalCount(
     const std::string& name,
     base::HistogramBase::Count expected_count,
     base::HistogramSamples& samples) {
-  EXPECT_EQ(expected_count, samples.TotalCount())
+  int actual_count = samples.TotalCount();
+  if (histogram_snapshots.count(name))
+    actual_count -= histogram_snapshots[name]->TotalCount();
+  EXPECT_EQ(expected_count, actual_count)
       << "Histogram \"" << name
       << "\" does not have the right total number of samples ("
-      << expected_count << ").";
+      << expected_count << "). It has (" << actual_count << ").";
 }

chrome/test/base/uma_histogram_helper.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_TEST_BASE_UMA_HISTOGRAM_HELPER_H_
 #define CHROME_TEST_BASE_UMA_HISTOGRAM_HELPER_H_
 
+#include "base/memory/linked_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_samples.h"
@@ -14,25 +15,40 @@
 // intended.
 class UMAHistogramHelper {
  public:
+  // UMAHistogramHelper should be created before the execution of the test case.
   UMAHistogramHelper();
 
+  ~UMAHistogramHelper();
+
+  // Parameters should be string literals of all histograms to snapshot.
+  // Call this before executing the test code. This method can be called
+  // multiple times. The existing snapshots are preserved, except when one of
+  // the |histogram_names| was previously passed as a parameter, then a new
+  // snapshot will replace the existing one.
+  void PrepareSnapshot(const char* const histogram_names[],
+                       size_t num_histograms);
+
   // Each child process may have its own histogram data, make sure this data
   // gets accumulated into the browser process before we examine the histograms.
   void Fetch();
 
   // We know the exact number of samples in a bucket, and that no other bucket
-  // should have samples.
+  // should have samples. If |PrepareSnapshot| was called for the histogram
+  // named |name| then the |expected_count| is the diff from the snapshot.
   void ExpectUniqueSample(const std::string& name,
                           base::HistogramBase::Sample sample,
                           base::HistogramBase::Count expected_count);
 
   // We know the exact number of samples in a bucket, but other buckets may
-  // have samples as well.
+  // have samples as well. If |PrepareSnapshot| was called for histogram named
+  // |name| then the |expected_count| is the diff from the snapshot.
   void ExpectBucketCount(const std::string& name,
                           base::HistogramBase::Sample sample,
                           base::HistogramBase::Count expected_count);
 
-  // We don't know the values of the samples, but we know how many there are.
+  // We don't know the values of the samples, but we know how many there are. If
+  // |PrepareSnapshot| was called for |name| histogram, then the
+  // |count| is the diff from the snapshot.
   void ExpectTotalCount(const std::string& name,
                         base::HistogramBase::Count count);
 
@@ -49,6 +65,10 @@ class UMAHistogramHelper {
                        base::HistogramSamples& samples);
 
   DISALLOW_COPY_AND_ASSIGN(UMAHistogramHelper);
+
+  // The map from histogram names to their snapshots
+  std::map<std::string, linked_ptr<base::HistogramSamples> >
+      histogram_snapshots;
 };
 
 #endif  // CHROME_TEST_BASE_UMA_HISTOGRAM_HELPER_H_

components/signin/core/browser/account_reconcilor.cc

@@ -359,8 +359,10 @@ void AccountReconcilor::GoogleSignedOut(const std::string& username) {
 }
 
 void AccountReconcilor::PerformMergeAction(const std::string& account_id) {
-  if (!switches::IsNewProfileManagement())
+  if (!switches::IsNewProfileManagement()) {
+    MarkAccountAsAddedToCookie(account_id);
     return;
+  }
   VLOG(1) << "AccountReconcilor::PerformMergeAction: " << account_id;
   merge_session_helper_.LogIn(account_id);
 }
@@ -398,8 +400,10 @@ void AccountReconcilor::PerformFinishRemoveAction(
 
 void AccountReconcilor::PerformAddToChromeAction(const std::string& account_id,
                                                  int session_index) {
-  if (!switches::IsNewProfileManagement())
+  if (!switches::IsNewProfileManagement()) {
+    MarkAccountAsAddedToChrome(account_id);
     return;
+  }
   VLOG(1) << "AccountReconcilor::PerformAddToChromeAction:"
           << " account=" << account_id << " session_index=" << session_index;
 
@@ -706,13 +710,9 @@ void AccountReconcilor::ScheduleStartReconcileIfChromeAccountsChanged() {
   }
 }
 
-void AccountReconcilor::MergeSessionCompleted(
-    const std::string& account_id,
-    const GoogleServiceAuthError& error) {
-  VLOG(1) << "AccountReconcilor::MergeSessionCompleted: account_id="
-          << account_id;
-
-  // Remove the account from the list that is being merged.
+// Remove the account from the list that is being merged.
+void AccountReconcilor::MarkAccountAsAddedToCookie(
+    const std::string& account_id) {
   for (std::vector<std::string>::iterator i = add_to_cookie_.begin();
        i != add_to_cookie_.end();
        ++i) {
@@ -721,7 +721,15 @@ void AccountReconcilor::MergeSessionCompleted(
       break;
     }
   }
+}
+
+void AccountReconcilor::MergeSessionCompleted(
+    const std::string& account_id,
+    const GoogleServiceAuthError& error) {
+  VLOG(1) << "AccountReconcilor::MergeSessionCompleted: account_id="
+          << account_id;
 
+  MarkAccountAsAddedToCookie(account_id);
   CalculateIfReconcileIsDone();
   ScheduleStartReconcileIfChromeAccountsChanged();
 }
@@ -748,13 +756,9 @@ void AccountReconcilor::PerformAddAccountToTokenService(
   token_service_->UpdateCredentials(account_id, refresh_token);
 }
 
-void AccountReconcilor::HandleRefreshTokenFetched(
-    const std::string& account_id,
-    const std::string& refresh_token) {
-  if (!refresh_token.empty()) {
-    PerformAddAccountToTokenService(account_id, refresh_token);
-  }
-  // Remove the account from the list that is being updated.
+// Remove the account from the list that is being updated.
+void AccountReconcilor::MarkAccountAsAddedToChrome(
+    const std::string& account_id) {
   for (std::vector<std::pair<std::string, int> >::iterator i =
            add_to_chrome_.begin();
        i != add_to_chrome_.end();
@@ -764,6 +768,14 @@ void AccountReconcilor::HandleRefreshTokenFetched(
       break;
     }
   }
+}
+
+void AccountReconcilor::HandleRefreshTokenFetched(
+    const std::string& account_id,
+    const std::string& refresh_token) {
+  if (!refresh_token.empty())
+    PerformAddAccountToTokenService(account_id, refresh_token);
 
+  MarkAccountAsAddedToChrome(account_id);
   CalculateIfReconcileIsDone();
 }

components/signin/core/browser/account_reconcilor.h

@@ -121,6 +121,8 @@ class AccountReconcilor : public KeyedService,
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileNoopWithDots);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileNoopMultiple);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileAddToCookie);
+  FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest,
+                           StartReconcileAddToCookieTwice);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileAddToChrome);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileBadPrimary);
   FRIEND_TEST_ALL_PREFIXES(AccountReconcilorTest, StartReconcileOnlyOnce);
@@ -172,6 +174,10 @@ class AccountReconcilor : public KeyedService,
       const GoogleServiceAuthError& error,
       const std::vector<std::pair<std::string, bool> >& accounts);
   void ValidateAccountsFromTokenService();
+  // Note internally that this |account_id| is added to the cookie jar.
+  void MarkAccountAsAddedToCookie(const std::string& account_id);
+  // Note internally that this |account_id| is added to the token service.
+  void MarkAccountAsAddedToChrome(const std::string& account_id);
 
   void OnCookieChanged(const net::CanonicalCookie* cookie);