Cleanup desktop NTP metrics recording, part 2

chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc

Index: chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc
diff --git a/chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc b/chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc
index e67065fc953999f888d36320df52d78abde45ef7..32f5ebedb751a4b1de6d62bc875b8a558fb18b36 100644
--- a/chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc
+++ b/chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc
@@ -4,23 +4,26 @@
 
 #include "chrome/browser/ui/webui/ntp/ntp_user_data_logger.h"
 
-#include "base/metrics/histogram_macros.h"
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/metrics/histogram.h"
 #include "base/metrics/statistics_recorder.h"
-#include "base/strings/utf_string_conversions.h"
 #include "chrome/common/search/ntp_logging_events.h"
-#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
 using testing::ElementsAre;
+using testing::ElementsAreArray;
 using Sample = base::HistogramBase::Sample;
 using Samples = std::vector<Sample>;
 
 class TestNTPUserDataLogger : public NTPUserDataLogger {
  public:
-  TestNTPUserDataLogger() : NTPUserDataLogger(NULL) {}
+  TestNTPUserDataLogger() : NTPUserDataLogger(nullptr) {}
   ~TestNTPUserDataLogger() override {}
 };
 
@@ -74,13 +77,13 @@ std::vector<int> ClientNavigations(const Samples& values) {
 
 }  // namespace
 
-class NTPUserDataLoggerTest : public testing::Test {
-  content::TestBrowserThreadBundle thread_bundle_;
-};
-
-TEST_F(NTPUserDataLoggerTest, TestNumberOfTiles) {
+TEST(NTPUserDataLoggerTest, TestNumberOfTiles) {
   base::StatisticsRecorder::Initialize();
 
+  int expect_total = GetTotalCount("NewTabPage.NumberOfTiles");
+  int expect_bin0 = GetBinCount("NewTabPage.NumberOfTiles", 0);
+  int expect_bin8 = GetBinCount("NewTabPage.NumberOfTiles", 8);

[Marc Treib, 2016/10/24 11:51:44]

This is a bit ugly.
Histograms are set up in a way that they survive between unit tests, and
apparently there's no way to reset them. So I'm querying the count at the start
of the test, and then essentially EXPECT the diff to that.
The alternative to this would be to use the HistogramTester class, which does
essentially the same thing. Arguably that's less ugly, but the EXPECTs become a
bit weird (it comes with its own ExpectBinCount etc methods). WDYT?

[sfiera, 2016/10/24 13:01:16]

You don't have to use its Expect*() methods, do you? My reading of the class
header is that you can do something like:

  EXPECT_THAT(
      histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
      ElementsAre(Bucket(8, 1)));

[Marc Treib, 2016/10/24 14:29:11]

Right. Done.

+
   // Ensure non-zero statistics.
   TestNTPUserDataLogger logger;
   logger.ntp_url_ = GURL("chrome://newtab/");
@@ -88,16 +91,17 @@ TEST_F(NTPUserDataLoggerTest, TestNumberOfTiles) {
   base::TimeDelta delta = base::TimeDelta::FromMilliseconds(0);
 
   for (int i = 0; i < 8; ++i)
-    logger.LogEvent(NTP_SERVER_SIDE_SUGGESTION, delta);
-
+    logger.LogMostVisitedImpression(i, NTPLoggingTileSource::SERVER);
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
+  expect_total++;
+  expect_bin8++;
+  EXPECT_EQ(expect_total, GetTotalCount("NewTabPage.NumberOfTiles"));
+  EXPECT_EQ(expect_bin0, GetBinCount("NewTabPage.NumberOfTiles", 0));
+  EXPECT_EQ(expect_bin8, GetBinCount("NewTabPage.NumberOfTiles", 8));
 
-  EXPECT_EQ(1, GetTotalCount("NewTabPage.NumberOfTiles"));
-  EXPECT_EQ(1, GetBinCount("NewTabPage.NumberOfTiles", 8));
-
-  // Statistics should be reset to 0, so we should not log anything else.
+  // We should not log again for the same NTP.
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
-  EXPECT_EQ(1, GetTotalCount("NewTabPage.NumberOfTiles"));
+  EXPECT_EQ(expect_total, GetTotalCount("NewTabPage.NumberOfTiles"));
 
   // Navigating away and back resets stats.
   logger.NavigatedFromURLToURL(GURL("chrome://newtab/"),
@@ -105,48 +109,61 @@ TEST_F(NTPUserDataLoggerTest, TestNumberOfTiles) {
   logger.NavigatedFromURLToURL(GURL("http://chromium.org"),
                                GURL("chrome://newtab/"));
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
-  EXPECT_EQ(2, GetTotalCount("NewTabPage.NumberOfTiles"));
+  expect_total++;
+  expect_bin0++;
+  EXPECT_EQ(expect_total, GetTotalCount("NewTabPage.NumberOfTiles"));
+  EXPECT_EQ(expect_bin0, GetBinCount("NewTabPage.NumberOfTiles", 0));
+  EXPECT_EQ(expect_bin8, GetBinCount("NewTabPage.NumberOfTiles", 8));
 }
 
-TEST_F(NTPUserDataLoggerTest, TestLogMostVisitedImpression) {
+TEST(NTPUserDataLoggerTest, TestLogMostVisitedImpression) {
   base::StatisticsRecorder::Initialize();
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+
+  std::vector<int> expect_total = TotalImpressions({0, 1, 2, 3});
+  std::vector<int> expect_server = ServerImpressions({0, 1, 2, 3});
+  std::vector<int> expect_client = ClientImpressions({0, 1, 2, 3});
 
   TestNTPUserDataLogger logger;
   logger.ntp_url_ = GURL("chrome://newtab/");
 
   // Impressions increment the associated bins.
 
+  logger.LogMostVisitedImpression(0, NTPLoggingTileSource::SERVER);
+  expect_total[0]++;
+  expect_server[0]++;
+  EXPECT_THAT(TotalImpressions({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerImpressions({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientImpressions({0, 1, 2, 3}), ElementsAreArray(expect_client));
+
   logger.LogMostVisitedImpression(1, NTPLoggingTileSource::SERVER);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 0, 0, 0));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 0, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  expect_total[1]++;
+  expect_server[1]++;
+  EXPECT_THAT(TotalImpressions({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerImpressions({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientImpressions({0, 1, 2, 3}), ElementsAreArray(expect_client));
 
-  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::SERVER);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::CLIENT);
+  expect_total[2]++;
+  expect_client[2]++;
+  EXPECT_THAT(TotalImpressions({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerImpressions({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientImpressions({0, 1, 2, 3}), ElementsAreArray(expect_client));
 
   logger.LogMostVisitedImpression(3, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 1, 0));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 1, 0));
-
-  logger.LogMostVisitedImpression(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 1, 1));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 1, 1));
+  expect_total[3]++;
+  expect_client[3]++;
+  EXPECT_THAT(TotalImpressions({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerImpressions({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientImpressions({0, 1, 2, 3}), ElementsAreArray(expect_client));
 
   // But once incremented, they don't increase again unless reset.
   logger.LogMostVisitedImpression(1, NTPLoggingTileSource::SERVER);
   logger.LogMostVisitedImpression(2, NTPLoggingTileSource::CLIENT);
   logger.LogMostVisitedImpression(3, NTPLoggingTileSource::SERVER);
   logger.LogMostVisitedImpression(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 1, 1));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 1, 1));
+  EXPECT_THAT(TotalImpressions({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerImpressions({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientImpressions({0, 1, 2, 3}), ElementsAreArray(expect_client));
 
   // Impressions are silently ignored for tiles >= 8.
   logger.LogMostVisitedImpression(8, NTPLoggingTileSource::SERVER);
@@ -155,55 +172,79 @@ TEST_F(NTPUserDataLoggerTest, TestLogMostVisitedImpression) {
   EXPECT_THAT(ServerImpressions({8, 9}), ElementsAre(0, 0));
   EXPECT_THAT(ClientImpressions({8, 9}), ElementsAre(0, 0));
 
-  // Navigating away from the NTP and back resets stats.
+  // After navigating away from the NTP and back, we record again.
   logger.NavigatedFromURLToURL(GURL("chrome://newtab/"),
                                GURL("http://chromium.org"));
   logger.NavigatedFromURLToURL(GURL("http://chromium.org"),
                                GURL("chrome://newtab/"));
-  logger.LogMostVisitedImpression(1, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::CLIENT);
-  logger.LogMostVisitedImpression(3, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedImpression(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(2, 2, 2, 2));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(2, 1, 1, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 1, 1, 2));
+  logger.LogMostVisitedImpression(0, NTPLoggingTileSource::SERVER);
+  expect_total[0]++;
+  expect_server[0]++;
+  logger.LogMostVisitedImpression(1, NTPLoggingTileSource::CLIENT);
+  expect_total[1]++;
+  expect_client[1]++;
+  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::SERVER);
+  expect_total[2]++;
+  expect_server[2]++;
+  logger.LogMostVisitedImpression(3, NTPLoggingTileSource::CLIENT);
+  expect_total[3]++;
+  expect_client[3]++;
+  EXPECT_THAT(TotalImpressions({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerImpressions({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientImpressions({0, 1, 2, 3}), ElementsAreArray(expect_client));
 }
 
-TEST_F(NTPUserDataLoggerTest, TestLogMostVisitedNavigation) {
+TEST(NTPUserDataLoggerTest, TestLogMostVisitedNavigation) {
   base::StatisticsRecorder::Initialize();
 
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  std::vector<int> expect_total = TotalNavigations({0, 1, 2, 3});
+  std::vector<int> expect_server = ServerNavigations({0, 1, 2, 3});
+  std::vector<int> expect_client = ClientNavigations({0, 1, 2, 3});
 
   TestNTPUserDataLogger logger;
 
+  logger.LogMostVisitedNavigation(0, NTPLoggingTileSource::SERVER);
+  expect_total[0]++;
+  expect_server[0]++;
+  EXPECT_THAT(TotalNavigations({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerNavigations({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientNavigations({0, 1, 2, 3}), ElementsAreArray(expect_client));
+
   logger.LogMostVisitedNavigation(1, NTPLoggingTileSource::SERVER);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(1, 0, 0, 0));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(1, 0, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  expect_total[1]++;
+  expect_server[1]++;
+  EXPECT_THAT(TotalNavigations({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerNavigations({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientNavigations({0, 1, 2, 3}), ElementsAreArray(expect_client));
 
-  logger.LogMostVisitedNavigation(2, NTPLoggingTileSource::SERVER);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  logger.LogMostVisitedNavigation(2, NTPLoggingTileSource::CLIENT);
+  expect_total[2]++;
+  expect_client[2]++;
+  EXPECT_THAT(TotalNavigations({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerNavigations({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientNavigations({0, 1, 2, 3}), ElementsAreArray(expect_client));
 
   logger.LogMostVisitedNavigation(3, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 1, 0));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 1, 0));
-
-  logger.LogMostVisitedNavigation(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 1, 1));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 1, 1));
+  expect_total[3]++;
+  expect_client[3]++;
+  EXPECT_THAT(TotalNavigations({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerNavigations({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientNavigations({0, 1, 2, 3}), ElementsAreArray(expect_client));
 
   // Navigations always increase.
-  logger.LogMostVisitedNavigation(1, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedNavigation(2, NTPLoggingTileSource::CLIENT);
-  logger.LogMostVisitedNavigation(3, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedNavigation(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(2, 2, 2, 2));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(2, 1, 1, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 1, 1, 2));
+  logger.LogMostVisitedNavigation(0, NTPLoggingTileSource::SERVER);
+  expect_total[0]++;
+  expect_server[0]++;
+  logger.LogMostVisitedNavigation(1, NTPLoggingTileSource::CLIENT);
+  expect_total[1]++;
+  expect_client[1]++;
+  logger.LogMostVisitedNavigation(2, NTPLoggingTileSource::SERVER);
+  expect_total[2]++;
+  expect_server[2]++;
+  logger.LogMostVisitedNavigation(3, NTPLoggingTileSource::CLIENT);
+  expect_total[3]++;
+  expect_client[3]++;
+  EXPECT_THAT(TotalNavigations({0, 1, 2, 3}), ElementsAreArray(expect_total));
+  EXPECT_THAT(ServerNavigations({0, 1, 2, 3}), ElementsAreArray(expect_server));
+  EXPECT_THAT(ClientNavigations({0, 1, 2, 3}), ElementsAreArray(expect_client));
 }