Cleanup desktop NTP metrics recording, part 2

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

 // Copyright 2013 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/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 {}
 };
 
 base::HistogramBase::Count GetTotalCount(const std::string& histogram_name) {
   base::HistogramBase* histogram = base::StatisticsRecorder::FindHistogram(
       histogram_name);
   // Return 0 if history is uninitialized.
   return histogram ? histogram->SnapshotSamples()->TotalCount() : 0;
 }
 
@@ skipping 33 matching lines @@
 std::vector<int> ServerNavigations(const Samples& values) {
   return GetBinCounts("NewTabPage.MostVisited.server", values);
 }
 
 std::vector<int> ClientNavigations(const Samples& values) {
   return GetBinCounts("NewTabPage.MostVisited.client", values);
 }
 
 }  // namespace
 
-class NTPUserDataLoggerTest : public testing::Test {
-  content::TestBrowserThreadBundle thread_bundle_;
-};
+TEST(NTPUserDataLoggerTest, TestNumberOfTiles) {
+  base::StatisticsRecorder::Initialize();
 
-TEST_F(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/");
 
   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));
 
+  // We should not log again for the same NTP.
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
-
-  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.
-  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/"),
                                GURL("http://chromium.org"));
   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);
   logger.LogMostVisitedImpression(9, NTPLoggingTileSource::CLIENT);
   EXPECT_THAT(TotalImpressions({8, 9}), ElementsAre(0, 0));
   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));
 }