UMA for the Stale Previews timestamp

chrome/browser/previews/previews_infobar_delegate_unittest.cc

 // Copyright 2016 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/previews/previews_infobar_delegate.h"
 
 #include <memory>
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #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"
 #include "base/time/time.h"
 #include "chrome/browser/android/android_theme_resources.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/loader/chrome_navigation_data.h"
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h"
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.h"
 #include "chrome/browser/previews/previews_infobar_tab_helper.h"
@@ skipping 37 matching lines @@
 const char kUMAPreviewsInfoBarActionLoFi[] = "Previews.InfoBarAction.LoFi";
 
 // Key of the UMA Previews.InfoBarAction.Offline histogram.
 const char kUMAPreviewsInfoBarActionOffline[] =
     "Previews.InfoBarAction.Offline";
 
 // Key of the UMA Previews.InfoBarAction.LitePage histogram.
 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> {
  public:
   explicit TestPreviewsWebContentsObserver(content::WebContents* web_contents)
       : content::WebContentsObserver(web_contents),
         last_navigation_reload_type_(content::ReloadType::NONE),
         should_have_page_id_(true) {}
   ~TestPreviewsWebContentsObserver() override {}
 
@@ skipping 87 matching lines @@
                    base::Unretained(this)));
 
     InfoBarService* infobar_service =
         InfoBarService::FromWebContents(web_contents());
     EXPECT_EQ(1U, infobar_service->infobar_count());
 
     return static_cast<PreviewsInfoBarDelegate*>(
         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);
 
     std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
     feature_list->RegisterFieldTrialOverride(
         previews::features::kStalePreviewsTimestamp.name,
         base::FeatureList::OVERRIDE_ENABLE_FEATURE, field_trial);
     scoped_feature_list_.InitWithFeatureList(std::move(feature_list));
   }
@@ skipping 403 matching lines @@
                     ->OptOutsSizeForTesting());
 
   drp_test_context_->RunUntilIdle();
 
   EXPECT_EQ(0u, data_reduction_proxy_settings->data_reduction_proxy_service()
                     ->pingback_client()
                     ->OptOutsSizeForTesting());
 }
 
 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;
 
   PreviewsInfoBarDelegate* infobar = CreateInfoBar(
       previews::PreviewsType::LITE_PAGE,
       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;
 
   PreviewsInfoBarDelegate* infobar = CreateInfoBar(
       previews::PreviewsType::LITE_PAGE,
       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;
 
   PreviewsInfoBarDelegate* infobar = CreateInfoBar(
       previews::PreviewsType::LITE_PAGE,
       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);
+}