UMA for the Stale Previews timestamp

chrome/browser/previews/previews_infobar_delegate_unittest.cc

Index: chrome/browser/previews/previews_infobar_delegate_unittest.cc
diff --git a/chrome/browser/previews/previews_infobar_delegate_unittest.cc b/chrome/browser/previews/previews_infobar_delegate_unittest.cc
index 107da1d9e9a50ab2046207a4a3e93e6cc09aefa6..faa5b1c4feec191fcf3cbb9e7a2050e0697bc49a 100644
--- a/chrome/browser/previews/previews_infobar_delegate_unittest.cc
+++ b/chrome/browser/previews/previews_infobar_delegate_unittest.cc
@@ -12,8 +12,10 @@
 #include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
+#include "base/metrics/field_trial_param_associator.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/optional.h"
+#include "base/strings/string16.h"
 #include "base/test/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -71,6 +73,9 @@ const char kUMAPreviewsInfoBarActionOffline[] =
 const char kUMAPreviewsInfoBarActionLitePage[] =
     "Previews.InfoBarAction.LitePage";
 
+// Key of the UMA Previews.InfoBarTimestamp histogram.
+const char kUMAPreviewsInfoBarTimestamp[] = "Previews.InfoBarTimestamp";
+
 class TestPreviewsWebContentsObserver
     : public content::WebContentsObserver,
       public content::WebContentsUserData<TestPreviewsWebContentsObserver> {
@@ -178,14 +183,15 @@ class PreviewsInfoBarDelegateUnitTest : public ChromeRenderViewHostTestHarness {
         infobar_service->infobar_at(0)->delegate());
   }
 
-  void EnableStalePreviewsTimestamp() {
+  void EnableStalePreviewsTimestamp(
+      std::map<std::string, std::string> variation_params) {

[tbansal1, 2017/06/22 21:12:16]

#inclide <map>

[megjablon, 2017/06/22 23:40:11]

Done.

+    field_trial_list_.reset();

[tbansal1, 2017/06/22 21:12:16]

Is this needed given the following line is also calling reset?

[megjablon, 2017/06/22 23:40:11]

It's needed because otherwise the constructor throws an error: 

[field_trial.cc(500)] Check failed: !global_.

https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=d795cd03...

+    field_trial_list_.reset(new base::FieldTrialList(nullptr));
+    base::FieldTrialParamAssociator::GetInstance()->ClearAllParamsForTesting();
+
     const std::string kTrialName = "TrialName";
     const std::string kGroupName = "GroupName";
 
-    std::map<std::string, std::string> variation_params;
-    variation_params["min_staleness_in_minutes"] = "2";
-    variation_params["max_staleness_in_minutes"] = "1440";
-
     base::AssociateFieldTrialParams(kTrialName, kGroupName, variation_params);
     base::FieldTrial* field_trial =
         base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName);
@@ -609,7 +615,10 @@ TEST_F(PreviewsInfoBarDelegateUnitTest, PingbackClientClearedTabClosed) {
 }
 
 TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampMintuesTest) {
-  EnableStalePreviewsTimestamp();
+  base::HistogramTester tester;
+  // Use default params.
+  std::map<std::string, std::string> variation_params;
+  EnableStalePreviewsTimestamp(variation_params);
 
   int staleness_in_minutes = 5;
 
@@ -618,14 +627,19 @@ TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampMintuesTest) {
       base::Time::Now() - base::TimeDelta::FromMinutes(staleness_in_minutes),
       true /* is_data_saver_user */);
 
-  ASSERT_EQ(
+  EXPECT_EQ(
       l10n_util::GetStringFUTF16(IDS_PREVIEWS_INFOBAR_TIMESTAMP_MINUTES,
                                  base::IntToString16(staleness_in_minutes)),
       infobar->GetTimestampText());
+  tester.ExpectUniqueSample(kUMAPreviewsInfoBarTimestamp,
+                            PreviewsInfoBarDelegate::TIMESTAMP_SHOWN, 1);
 }
 
 TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampHourTest) {
-  EnableStalePreviewsTimestamp();
+  base::HistogramTester tester;
+  // Use default params.
+  std::map<std::string, std::string> variation_params;
+  EnableStalePreviewsTimestamp(variation_params);
 
   int staleness_in_minutes = 65;
 
@@ -634,12 +648,17 @@ TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampHourTest) {
       base::Time::Now() - base::TimeDelta::FromMinutes(staleness_in_minutes),
       true /* is_data_saver_user */);
 
-  ASSERT_EQ(l10n_util::GetStringUTF16(IDS_PREVIEWS_INFOBAR_TIMESTAMP_ONE_HOUR),
+  EXPECT_EQ(l10n_util::GetStringUTF16(IDS_PREVIEWS_INFOBAR_TIMESTAMP_ONE_HOUR),
             infobar->GetTimestampText());
+  tester.ExpectUniqueSample(kUMAPreviewsInfoBarTimestamp,
+                            PreviewsInfoBarDelegate::TIMESTAMP_SHOWN, 1);
 }
 
 TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampHoursTest) {
-  EnableStalePreviewsTimestamp();
+  base::HistogramTester tester;
+  // Use default params.
+  std::map<std::string, std::string> variation_params;
+  EnableStalePreviewsTimestamp(variation_params);
 
   int staleness_in_hours = 2;
 
@@ -648,7 +667,122 @@ TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampHoursTest) {
       base::Time::Now() - base::TimeDelta::FromHours(staleness_in_hours),
       true /* is_data_saver_user */);
 
-  ASSERT_EQ(l10n_util::GetStringFUTF16(IDS_PREVIEWS_INFOBAR_TIMESTAMP_HOURS,
+  EXPECT_EQ(l10n_util::GetStringFUTF16(IDS_PREVIEWS_INFOBAR_TIMESTAMP_HOURS,
                                        base::IntToString16(staleness_in_hours)),
             infobar->GetTimestampText());
+  tester.ExpectUniqueSample(kUMAPreviewsInfoBarTimestamp,
+                            PreviewsInfoBarDelegate::TIMESTAMP_SHOWN, 1);
+}
+
+TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampFinchParamsUMA) {
+  base::HistogramTester tester;
+  std::map<std::string, std::string> variation_params;
+  variation_params["min_staleness_in_minutes"] = "1";
+  variation_params["max_staleness_in_minutes"] = "5";
+  EnableStalePreviewsTimestamp(variation_params);
+
+  int staleness_in_minutes = 1;
+
+  PreviewsInfoBarDelegate* infobar = CreateInfoBar(
+      previews::PreviewsType::LITE_PAGE,
+      base::Time::Now() - base::TimeDelta::FromMinutes(staleness_in_minutes),
+      true /* is_data_saver_user */);
+
+  EXPECT_EQ(
+      l10n_util::GetStringFUTF16(IDS_PREVIEWS_INFOBAR_TIMESTAMP_MINUTES,
+                                 base::IntToString16(staleness_in_minutes)),
+      infobar->GetTimestampText());
+  tester.ExpectBucketCount(kUMAPreviewsInfoBarTimestamp,
+                           PreviewsInfoBarDelegate::TIMESTAMP_SHOWN, 1);
+
+  // Navigate and show infobar again.
+  NavigateAndCommit(GURL(kTestUrl));
+
+  staleness_in_minutes = 6;
+
+  infobar = CreateInfoBar(
+      previews::PreviewsType::LITE_PAGE,
+      base::Time::Now() - base::TimeDelta::FromMinutes(staleness_in_minutes),
+      true /* is_data_saver_user */);
+
+  EXPECT_EQ(base::string16(), infobar->GetTimestampText());
+  tester.ExpectBucketCount(
+      kUMAPreviewsInfoBarTimestamp,
+      PreviewsInfoBarDelegate::TIMESTAMP_NOT_SHOWN_STALENESS_GREATER_THAN_MAX,
+      1);
+}
+
+TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampNoParamsUMA) {
+  base::HistogramTester tester;
+  std::map<std::string, std::string> variation_params;
+  variation_params["min_staleness_in_minutes"] = "-5";
+  variation_params["max_staleness_in_minutes"] = "1440";
+  EnableStalePreviewsTimestamp(variation_params);
+
+  int staleness_in_minutes = 5;
+
+  PreviewsInfoBarDelegate* infobar = CreateInfoBar(
+      previews::PreviewsType::LITE_PAGE,
+      base::Time::Now() - base::TimeDelta::FromMinutes(staleness_in_minutes),
+      true /* is_data_saver_user */);
+
+  EXPECT_EQ(base::string16(), infobar->GetTimestampText());
+  tester.ExpectUniqueSample(
+      kUMAPreviewsInfoBarTimestamp,
+      PreviewsInfoBarDelegate::TIMESTAMP_NOT_SHOWN_PARAMS_NOT_AVAILABLE, 1);
+}
+
+TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampNotStaleUMA) {
+  base::HistogramTester tester;
+  // Use default params.
+  std::map<std::string, std::string> variation_params;
+  EnableStalePreviewsTimestamp(variation_params);
+
+  int staleness_in_minutes = 1;
+
+  PreviewsInfoBarDelegate* infobar = CreateInfoBar(
+      previews::PreviewsType::LITE_PAGE,
+      base::Time::Now() - base::TimeDelta::FromMinutes(staleness_in_minutes),
+      true /* is_data_saver_user */);
+  ASSERT_EQ(base::string16(), infobar->GetTimestampText());
+  tester.ExpectUniqueSample(
+      kUMAPreviewsInfoBarTimestamp,
+      PreviewsInfoBarDelegate::TIMESTAMP_NOT_SHOWN_PREVIEW_NOT_STALE, 1);
+}
+
+TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampNegativeUMA) {
+  base::HistogramTester tester;
+  // Use default params.
+  std::map<std::string, std::string> variation_params;
+  EnableStalePreviewsTimestamp(variation_params);
+
+  int staleness_in_minutes = -1;
+
+  PreviewsInfoBarDelegate* infobar = CreateInfoBar(
+      previews::PreviewsType::LITE_PAGE,
+      base::Time::Now() - base::TimeDelta::FromMinutes(staleness_in_minutes),
+      true /* is_data_saver_user */);
+  EXPECT_EQ(base::string16(), infobar->GetTimestampText());
+  tester.ExpectUniqueSample(
+      kUMAPreviewsInfoBarTimestamp,
+      PreviewsInfoBarDelegate::TIMESTAMP_NOT_SHOWN_STALENESS_NEGATIVE, 1);
+}
+
+TEST_F(PreviewsInfoBarDelegateUnitTest, PreviewInfobarTimestampMaximumUMA) {

[tbansal1, 2017/06/22 21:12:16]

Would it be useful to combine this test with the ones above?
We can iterate over a struct that stores |staleness_in_minutes| and
|expected_unique_sample|?

[megjablon, 2017/06/22 23:40:11]

I refactored the tests. I didn't add a struct though because I heard we're
trying to move away from using that in our tests. PTAL.

[tbansal1, 2017/06/23 00:37:42]

Thanks, this looks much cleaner.

+  base::HistogramTester tester;
+  // Use default params.
+  std::map<std::string, std::string> variation_params;
+  EnableStalePreviewsTimestamp(variation_params);
+
+  int staleness_in_minutes = 1500;
+
+  PreviewsInfoBarDelegate* infobar = CreateInfoBar(
+      previews::PreviewsType::LITE_PAGE,
+      base::Time::Now() - base::TimeDelta::FromMinutes(staleness_in_minutes),
+      true /* is_data_saver_user */);
+  EXPECT_EQ(base::string16(), infobar->GetTimestampText());
+  tester.ExpectUniqueSample(
+      kUMAPreviewsInfoBarTimestamp,
+      PreviewsInfoBarDelegate::TIMESTAMP_NOT_SHOWN_STALENESS_GREATER_THAN_MAX,
+      1);
 }