Rework UMAHistogramHelper and StatisticsDeltaReader into [Chrome]HistogramTester.

chrome/browser/search/suggestions/blacklist_store_unittest.cc

 // Copyright 2014 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/search/suggestions/blacklist_store.h"
 
 #include <set>
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
-#include "base/metrics/histogram_samples.h"
-#include "base/metrics/statistics_recorder.h"
-#include "base/test/statistics_delta_reader.h"
 #include "chrome/browser/search/suggestions/proto/suggestions.pb.h"
+#include "chrome/test/base/chrome_histogram_tester.h"

[Ilya Sherman, 2014/07/15 03:56:35]

nit: Please include the //base class rather than the //chrome class here.  Ditto
for all the other test files that don't use the //chrome-layer functionality.

[Mike Lerman, 2014/07/16 17:29:03]

Done.

 #include "components/pref_registry/testing_pref_service_syncable.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using user_prefs::TestingPrefServiceSyncable;
 
 namespace suggestions {
 
 namespace {
 
@@ skipping 91 matching lines @@
   EXPECT_TRUE(blacklist_store.BlacklistUrl(GURL(kTestUrlB)));
 
   // Expect to retrieve A or B.
   EXPECT_TRUE(blacklist_store.GetFirstUrlFromBlacklist(&retrieved));
   std::string retrieved_string = retrieved.spec();
   EXPECT_TRUE(retrieved_string == std::string(kTestUrlA) ||
               retrieved_string == std::string(kTestUrlB));
 }
 
 TEST(BlacklistStoreLogTest, LogsBlacklistSize) {
+  base::HistogramTester histogram_tester;
   content::TestBrowserThreadBundle bundle;
-  base::StatisticsDeltaReader statistics_delta_reader;
 
   // Create a first store - blacklist is empty at this point.
   TestingPrefServiceSyncable prefs;
   BlacklistStore::RegisterProfilePrefs(prefs.registry());
   scoped_ptr<BlacklistStore> blacklist_store(new BlacklistStore(&prefs));
-  scoped_ptr<base::HistogramSamples> samples(
-      statistics_delta_reader.GetHistogramSamplesSinceCreation(
-          "Suggestions.LocalBlacklistSize"));
-  EXPECT_EQ(1, samples->TotalCount());
-  EXPECT_EQ(1, samples->GetCount(0));
+
+  histogram_tester.ExpectTotalCount("Suggestions.LocalBlacklistSize", 1);
+  histogram_tester.ExpectUniqueSample("Suggestions.LocalBlacklistSize", 0, 1);
 
   // Add some content to the blacklist.
   EXPECT_TRUE(blacklist_store->BlacklistUrl(GURL(kTestUrlA)));
   EXPECT_TRUE(blacklist_store->BlacklistUrl(GURL(kTestUrlB)));
 
   // Create a new BlacklistStore and verify the counts.
   blacklist_store.reset(new BlacklistStore(&prefs));
-  samples = statistics_delta_reader.GetHistogramSamplesSinceCreation(
-      "Suggestions.LocalBlacklistSize");
-  EXPECT_EQ(2, samples->TotalCount());
-  EXPECT_EQ(1, samples->GetCount(0));
-  EXPECT_EQ(1, samples->GetCount(2));
+
+  histogram_tester.ExpectTotalCount("Suggestions.LocalBlacklistSize", 2);
+  histogram_tester.ExpectBucketCount("Suggestions.LocalBlacklistSize", 0, 1);
+  histogram_tester.ExpectBucketCount("Suggestions.LocalBlacklistSize", 2, 1);

[Ilya Sherman, 2014/07/15 03:56:35]

Hmm, it's a bit unfortunate that both the second and the third parameters to
this function are of integer type -- it makes the semantics of the method call
somewhat hard to deduce at the call site.  I don't have any great ideas on how
to improve this, though -- do you?

[Mike Lerman, 2014/07/16 17:29:03]

Use Objective-C? I agree, this is ugly. Can't think of much. Well... I've seen
unit testing libraries for Java ... we'd have an API like:

histogram_tester.ForHistogram("blah blah").ForSample(2).ExpectValue(1);
or 
histogram_tester.ForHistogram("blah blah").ForUniqueSample(2).ExpectValue(1);
or
histogram_tester.ForHistogram("foo").ExpectTotalCount(2);

I've always found that kind of syntax really wordy in Java, especially since I
was usually working in Eclipse with Javadoc & Intellisense. Here... maybe it's
the right answer? What do you think? I could sketch out the necessary classes
and signatures in the design doc.

[Ilya Sherman, 2014/07/16 18:44:46]

Hmm, that's definitely an interesting thought, worth exploring more.

Another idea is to change the ExpectFoo() methods to be GetFoo() methods instead
-- then we'd have something like

EXPECT_EQ(1, histogram_tester.GetBucketCount("Suggestions.LocalBlacklistSize",
2));

That's at least somewhat clearer -- the 2 is still a little mysterious, but at
least the 1 is more clearly a count within a bucket.  WDYT?

In any case, I think it's reasonable to punt this question to a follow-up CL, so
that this one can remain a little more focused.

[Mike Lerman, 2014/07/16 19:36:35]

Acknowledged.

 }
 
 }  // namespace suggestions