[User classifier] Add a unit-test

components/ntp_snippets/user_classifier_unittest.cc

Index: components/ntp_snippets/user_classifier_unittest.cc
diff --git a/components/ntp_snippets/user_classifier_unittest.cc b/components/ntp_snippets/user_classifier_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5373a1dfb3901096251c169f01bb2d408c9f8cb4
--- /dev/null
+++ b/components/ntp_snippets/user_classifier_unittest.cc
@@ -0,0 +1,305 @@
+// Copyright 2017 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 "components/ntp_snippets/user_classifier.h"
+
+#include "base/memory/ptr_util.h"
+#include "base/test/histogram_tester.h"
+#include "base/test/simple_test_clock.h"
+#include "base/time/time.h"
+#include "components/ntp_snippets/features.h"
+#include "components/ntp_snippets/ntp_snippets_constants.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
+#include "components/variations/variations_params_manager.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::DoubleNear;
+using testing::Eq;
+using testing::Gt;
+using testing::Lt;
+using testing::SizeIs;
+
+namespace ntp_snippets {
+namespace {
+
+char kNowString[] = "2017-03-01 10:45";
+
+class UserClassifierTest : public testing::Test {
+ public:
+  UserClassifierTest() {
+    UserClassifier::RegisterProfilePrefs(test_prefs_.registry());
+  }
+
+  std::unique_ptr<UserClassifier> CreateUserClassifier() {
+    auto test_clock = base::MakeUnique<base::SimpleTestClock>();
+    test_clock_ = test_clock.get();

[Marc Treib, 2017/03/21 15:15:45]

This is dangerous: It hands out the UserClassifier which owns the clock, but
keeps a raw pointer to the clock around. If the UserClassifier gets destroyed,
you have a dangling pointer.

Could the test class retain ownership of the UserClassifier? If that's not
possible, maybe pass the clock pointer out through an out param?

[jkrcal, 2017/03/21 17:29:15]

Fixed - the classifier is now owned by the test class so that the tests
themselves cannot get the dangling pointer without knowing.

+
+    base::Time now;
+    CHECK(base::Time::FromUTCString(kNowString, &now));
+    test_clock_->SetNow(now);
+
+    return base::MakeUnique<UserClassifier>(&test_prefs_,
+                                            std::move(test_clock));
+  }
+
+ protected:
+  TestingPrefServiceSimple test_prefs_;
+  base::SimpleTestClock* test_clock_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(UserClassifierTest);
+};
+
+TEST_F(UserClassifierTest, ShouldBeActiveNtpUserInitially) {
+  auto user_classifier = CreateUserClassifier();
+  EXPECT_THAT(user_classifier->GetUserClass(),
+              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+}
+
+TEST_F(UserClassifierTest,
+       ShouldBecomeActiveSuggestionsConsumerByClickingOften) {
+  auto user_classifier = CreateUserClassifier();
+
+  // After one click still only an active user.
+  user_classifier->OnEvent(UserClassifier::Metric::SUGGESTIONS_USED);
+  EXPECT_THAT(user_classifier->GetUserClass(),
+              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+
+  // One more click to become an active consumer.

[Marc Treib, 2017/03/21 15:15:46]

Do we really want to test that the *second* click already puts you into the
ACTIVE_SUGGESTIONS_CONSUMER bucket?

[jkrcal, 2017/03/21 17:29:15]

Made it a bit more relaxed.

+  test_clock_->Advance(base::TimeDelta::FromHours(1));
+  user_classifier->OnEvent(UserClassifier::Metric::SUGGESTIONS_USED);
+  EXPECT_THAT(user_classifier->GetUserClass(),
+              Eq(UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER));
+}
+
+TEST_F(UserClassifierTest,
+       ShouldBecomeActiveSuggestionsConsumerByClickingOftenWithDecreasedParam) {
+  // Increase the param to one half.
+  variations::testing::VariationParamsManager variation_params(
+      kStudyName,
+      {{"user_classifier_active_consumer_clicks_at_least_once_per_hours",
+        "36"}},
+      {kArticleSuggestionsFeature.name});
+  auto user_classifier = CreateUserClassifier();
+
+  // After two clicks still only an active user.
+  user_classifier->OnEvent(UserClassifier::Metric::SUGGESTIONS_USED);
+  test_clock_->Advance(base::TimeDelta::FromHours(1));
+  user_classifier->OnEvent(UserClassifier::Metric::SUGGESTIONS_USED);
+  EXPECT_THAT(user_classifier->GetUserClass(),
+              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+
+  // One more click to become an active consumer.
+  test_clock_->Advance(base::TimeDelta::FromHours(1));
+  user_classifier->OnEvent(UserClassifier::Metric::SUGGESTIONS_USED);
+  EXPECT_THAT(user_classifier->GetUserClass(),
+              Eq(UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER));
+}
+
+TEST_F(UserClassifierTest, ShouldBecomeRareNtpUserByNoActivity) {
+  auto user_classifier = CreateUserClassifier();
+
+  // After two days of waiting still an active user.
+  test_clock_->Advance(base::TimeDelta::FromDays(2));
+  EXPECT_THAT(user_classifier->GetUserClass(),
+              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+
+  // Two more days to become a rare user.
+  test_clock_->Advance(base::TimeDelta::FromDays(2));
+  EXPECT_THAT(user_classifier->GetUserClass(),
+              Eq(UserClassifier::UserClass::RARE_NTP_USER));
+}
+
+TEST_F(UserClassifierTest,
+       ShouldBecomeRareNtpUserByNoActivityWithDecreasedParam) {
+  // Decrease the param to one half.
+  variations::testing::VariationParamsManager variation_params(
+      kStudyName,
+      {{"user_classifier_rare_user_opens_ntp_at_most_once_per_hours", "48"}},
+      {kArticleSuggestionsFeature.name});
+  auto user_classifier = CreateUserClassifier();
+
+  // After one days of waiting still an active user.
+  test_clock_->Advance(base::TimeDelta::FromDays(1));
+  EXPECT_THAT(user_classifier->GetUserClass(),
+              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+
+  // One more day to become a rare user.
+  test_clock_->Advance(base::TimeDelta::FromDays(1));
+  EXPECT_THAT(user_classifier->GetUserClass(),
+              Eq(UserClassifier::UserClass::RARE_NTP_USER));
+}
+
+class UserClassifierMetricTest
+    : public UserClassifierTest,
+      public ::testing::WithParamInterface<
+          std::pair<UserClassifier::Metric, std::string>> {
+ public:
+  UserClassifierMetricTest() : UserClassifierTest() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(UserClassifierMetricTest);
+};
+
+TEST_P(UserClassifierMetricTest, ShouldDecreaseEstimateAfterEvent) {
+  UserClassifier::Metric metric = GetParam().first;
+  auto user_classifier = CreateUserClassifier();
+
+  // The initial event does not decrease the estimate.
+  user_classifier->OnEvent(metric);
+
+  for (int i = 0; i < 10; i++) {
+    test_clock_->Advance(base::TimeDelta::FromHours(1));
+    double old_metric = user_classifier->GetEstimatedAvgTime(metric);
+    user_classifier->OnEvent(metric);
+    EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric), Lt(old_metric));
+  }
+}
+
+TEST_P(UserClassifierMetricTest, ShouldReportToUmaOnEvent) {
+  UserClassifier::Metric metric = GetParam().first;
+  const std::string& histogram_name = GetParam().second;
+  base::HistogramTester histogram_tester;
+  auto user_classifier = CreateUserClassifier();
+
+  user_classifier->OnEvent(metric);
+  EXPECT_THAT(histogram_tester.GetAllSamples(histogram_name), SizeIs(1));
+}
+
+TEST_P(UserClassifierMetricTest, ShouldConvergeTowardsPattern) {
+  UserClassifier::Metric metric = GetParam().first;
+  auto user_classifier = CreateUserClassifier();
+
+  // Have the pattern of an event every five hours and start changing it towards
+  // an event every 10 hours.
+  for (int i = 0; i < 100; i++) {
+    test_clock_->Advance(base::TimeDelta::FromHours(5));
+    user_classifier->OnEvent(metric);
+  }
+  EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric),
+              DoubleNear(5.0, 0.1));
+  for (int i = 0; i < 3; i++) {
+    test_clock_->Advance(base::TimeDelta::FromHours(10));
+    user_classifier->OnEvent(metric);
+  }
+  EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric), Gt(5.5));
+  for (int i = 0; i < 100; i++) {
+    test_clock_->Advance(base::TimeDelta::FromHours(10));
+    user_classifier->OnEvent(metric);
+  }
+  EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric),
+              DoubleNear(10.0, 0.1));
+}
+
+TEST_P(UserClassifierMetricTest, ShouldIgnoreSubsequentEvents) {
+  UserClassifier::Metric metric = GetParam().first;
+  auto user_classifier = CreateUserClassifier();
+
+  // The initial event
+  user_classifier->OnEvent(metric);
+  // No change for 25 minutes.
+  for (int i = 0; i < 5; i++) {
+    test_clock_->Advance(base::TimeDelta::FromMinutes(5));
+    double old_metric = user_classifier->GetEstimatedAvgTime(metric);
+    user_classifier->OnEvent(metric);
+    EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric), Eq(old_metric));
+  }
+  // After 30 minutes, it gets updated.

[Marc Treib, 2017/03/21 15:15:46]

...I don't get this one. What's going on here?
Maybe the test just needs a more descriptive name?

[jkrcal, 2017/03/21 17:29:15]

Done.

+  test_clock_->Advance(base::TimeDelta::FromMinutes(5));
+  double old_metric = user_classifier->GetEstimatedAvgTime(metric);
+  user_classifier->OnEvent(metric);
+  EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric), Lt(old_metric));
+}
+
+TEST_P(UserClassifierMetricTest,
+       ShouldIgnoreSubsequentEventsWithIncreasedLimit) {
+  UserClassifier::Metric metric = GetParam().first;
+  // Increase the min_hours to 1.0, i.e. 60 minutes.
+  variations::testing::VariationParamsManager variation_params(
+      kStudyName, {{"user_classifier_min_hours", "1.0"}},
+      {kArticleSuggestionsFeature.name});
+  auto user_classifier = CreateUserClassifier();
+
+  // The initial event
+  user_classifier->OnEvent(metric);
+  // No change for 55 minutes.
+  for (int i = 0; i < 11; i++) {
+    test_clock_->Advance(base::TimeDelta::FromMinutes(5));
+    double old_metric = user_classifier->GetEstimatedAvgTime(metric);
+    user_classifier->OnEvent(metric);
+    EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric), Eq(old_metric));
+  }
+  // After 60 minutes, it gets updated.
+  test_clock_->Advance(base::TimeDelta::FromMinutes(5));
+  double old_metric = user_classifier->GetEstimatedAvgTime(metric);
+  user_classifier->OnEvent(metric);
+  EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric), Lt(old_metric));
+}
+
+TEST_P(UserClassifierMetricTest, ShouldCapDelayBetweenEvents) {
+  UserClassifier::Metric metric = GetParam().first;
+  auto user_classifier = CreateUserClassifier();
+
+  // The initial event
+  user_classifier->OnEvent(metric);
+  // Wait for an insane amount of time
+  test_clock_->Advance(base::TimeDelta::FromDays(365));
+  user_classifier->OnEvent(metric);
+  double metric_after_a_year = user_classifier->GetEstimatedAvgTime(metric);
+
+  // Now repeat the same with s/one year/one week.
+  user_classifier->ClearClassificationForDebugging();
+  user_classifier->OnEvent(metric);
+  test_clock_->Advance(base::TimeDelta::FromDays(7));
+  user_classifier->OnEvent(metric);
+
+  // The results should be the same.
+  EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric),
+              Eq(metric_after_a_year));
+}
+
+TEST_P(UserClassifierMetricTest,
+       ShouldCapDelayBetweenEventsWithDecreasedLimit) {
+  UserClassifier::Metric metric = GetParam().first;
+  // Decrease the max_hours to 72, i.e. 3 days.
+  variations::testing::VariationParamsManager variation_params(
+      kStudyName, {{"user_classifier_max_hours", "72"}},
+      {kArticleSuggestionsFeature.name});
+  auto user_classifier = CreateUserClassifier();
+
+  // The initial event
+  user_classifier->OnEvent(metric);
+  // Wait for an insane amount of time
+  test_clock_->Advance(base::TimeDelta::FromDays(365));
+  user_classifier->OnEvent(metric);
+  double metric_after_a_year = user_classifier->GetEstimatedAvgTime(metric);
+
+  // Now repeat the same with s/one year/two days.
+  user_classifier->ClearClassificationForDebugging();
+  user_classifier->OnEvent(metric);
+  test_clock_->Advance(base::TimeDelta::FromDays(3));
+  user_classifier->OnEvent(metric);
+
+  // The results should be the same.
+  EXPECT_THAT(user_classifier->GetEstimatedAvgTime(metric),
+              Eq(metric_after_a_year));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ,

[Marc Treib, 2017/03/21 15:15:45]

What's this?

[jkrcal, 2017/03/21 17:29:15]

This is a prefix for instantiating the tests. An empty prefix, in particular :)
I added a comment.

+    UserClassifierMetricTest,
+    testing::Values(
+        std::make_pair(UserClassifier::Metric::NTP_OPENED,
+                       "NewTabPage.UserClassifier.AverageHoursToOpenNTP"),
+        std::make_pair(
+            UserClassifier::Metric::SUGGESTIONS_SHOWN,
+            "NewTabPage.UserClassifier.AverageHoursToShowSuggestions"),
+        std::make_pair(
+            UserClassifier::Metric::SUGGESTIONS_USED,
+            "NewTabPage.UserClassifier.AverageHoursToUseSuggestions")));
+
+}  // namespace
+}  // namespace ntp_snippets