Add field trials and platform to permission report

chrome/browser/safe_browsing/permission_reporter_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/safe_browsing/permission_reporter.h"
 
+#include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/field_trial.h"
 #include "chrome/common/safe_browsing/permission_report.pb.h"
+#include "components/variations/active_field_trials.h"
 #include "content/public/browser/permission_type.h"
 #include "net/url_request/report_sender.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::PermissionType;
 
 namespace safe_browsing {
 
 namespace {
+
+typedef std::set<variations::ActiveGroupId, variations::ActiveGroupIdCompare>
+    ActiveGroupIdSet;
+
 // URL to upload permission action reports.
 const char kPermissionActionReportingUploadUrl[] =
     "http://safebrowsing.googleusercontent.com/safebrowsing/clientreport/"
     "permission-action";
 
 const char kDummyOrigin[] = "http://example.test/";
 const PermissionType kDummyPermission = PermissionType::GEOLOCATION;
 const PermissionAction kDummyAction = GRANTED;
 const PermissionReport::PermissionType kDummyPermissionReportPermission =
     PermissionReport::GEOLOCATION;
 const PermissionReport::Action kDummyPermissionReportAction =
     PermissionReport::GRANTED;
 
+const char kDummyTrialOne[] = "trial one";
+const char kDummyGroupOne[] = "group one";
+const char kDummyTrialTwo[] = "trial two";
+const char kDummyGroupTwo[] = "group two";
+
+const char kFeatureOnByDefaultName[] = "OnByDefault";
+struct base::Feature kFeatureOnByDefault {
+  kFeatureOnByDefaultName, base::FEATURE_ENABLED_BY_DEFAULT
+};
+
+const char kFeatureOffByDefaultName[] = "OffByDefault";
+struct base::Feature kFeatureOffByDefault {
+  kFeatureOffByDefaultName, base::FEATURE_DISABLED_BY_DEFAULT
+};
+
 // A mock ReportSender that keeps track of the last report sent.
 class MockReportSender : public net::ReportSender {
  public:
   MockReportSender() : net::ReportSender(nullptr, DO_NOT_SEND_COOKIES) {}
   ~MockReportSender() override {}
 
   void Send(const GURL& report_uri, const std::string& report) override {
     latest_report_uri_ = report_uri;
     latest_report_ = report;
   }
@@ skipping 20 matching lines @@
 
   // Owned by |permission_reporter_|.
   MockReportSender* mock_report_sender_;
 
   std::unique_ptr<PermissionReporter> permission_reporter_;
 };
 
 // Test that PermissionReporter::SendReport sends a serialized report string to
 // SafeBrowsing CSD servers.
 TEST_F(PermissionReporterTest, SendReport) {
+  // Add and activate dummy field trials.
+  base::FieldTrialList field_trial_list(nullptr);

[raymes, 2016/06/22 01:23:44]

nit: is this needed?

[stefanocs, 2016/06/22 02:07:06]

Yes, the constructor will initialize a singleton instance of FieldTrialList.
This instance will be used when calling FieldTrialList::CreateFieldTrial.

https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=0&l=311

+  std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+  base::FieldTrial* trial_one =
+      base::FieldTrialList::CreateFieldTrial(kDummyTrialOne, kDummyGroupOne);
+  base::FieldTrial* trial_two =
+      base::FieldTrialList::CreateFieldTrial(kDummyTrialTwo, kDummyGroupTwo);
+
+  feature_list->RegisterFieldTrialOverride(
+      kFeatureOnByDefaultName, base::FeatureList::OVERRIDE_ENABLE_FEATURE,
+      trial_one);

[raymes, 2016/06/22 01:23:44]

I guess this isn't needed for the on-by-default feature? we could leave it out

[stefanocs, 2016/06/22 02:07:06]

The test doesn't seem to work with this removed. Why do you isn't needed?

[raymes, 2016/06/22 02:16:27]

Oh I assumed the default would apply in that case. It's ok to leave it if
necessary

+  feature_list->RegisterFieldTrialOverride(
+      kFeatureOffByDefaultName, base::FeatureList::OVERRIDE_ENABLE_FEATURE,
+      trial_two);
+
+  base::FeatureList::ClearInstanceForTesting();
+  base::FeatureList::SetInstance(std::move(feature_list));
+
+  // Enable features, this will also activate both field trials.
+  base::FeatureList::IsEnabled(kFeatureOnByDefault);
+  base::FeatureList::IsEnabled(kFeatureOffByDefault);

[raymes, 2016/06/22 01:23:44]

These just check whether the feature is enabled, right? Did you meant to use
EXPECT_TRUE here?

[stefanocs, 2016/06/22 02:07:06]

While this seems to be a non-modifying function, calling this function seems to
be necessary to activate the field trials.

https://cs.chromium.org/chromium/src/base/feature_list.cc?rcl=1466536938&l=211

[raymes, 2016/06/22 02:16:27]

I see. Perhaps make a note in the comment

+
+  EXPECT_TRUE(base::FieldTrialList::IsTrialActive(trial_one->trial_name()));
+  EXPECT_TRUE(base::FieldTrialList::IsTrialActive(trial_two->trial_name()));

[raymes, 2016/06/22 01:23:44]

Since there is a lot of setup/checking to test the field trials, perhaps we
could put it into its own test? (SendReportWithFieldTrials)

[stefanocs, 2016/06/22 02:07:05]

Done.

+
   permission_reporter_->SendReport(GURL(kDummyOrigin), kDummyPermission,
                                    kDummyAction);
 
   PermissionReport permission_report;
   ASSERT_TRUE(
       permission_report.ParseFromString(mock_report_sender_->latest_report()));
   EXPECT_EQ(kDummyPermissionReportPermission, permission_report.permission());
   EXPECT_EQ(kDummyPermissionReportAction, permission_report.action());
   EXPECT_EQ(kDummyOrigin, permission_report.origin());
+#if defined(OS_ANDROID)
+  EXPECT_EQ(PermissionReport::ANDROID_PLATFORM,
+            permission_report.platform_type());
+#elif defined(OS_MACOSX) || defined(OS_WIN) || defined(OS_CHROMEOS) || \
+    defined(OS_LINUX)
+  EXPECT_EQ(PermissionReport::DESKTOP_PLATFORM,
+            permission_report.platform_type());
+#endif
+
+  variations::ActiveGroupId field_trial_one =
+      variations::MakeActiveGroupId(kDummyTrialOne, kDummyGroupOne);
+  variations::ActiveGroupId field_trial_two =
+      variations::MakeActiveGroupId(kDummyTrialTwo, kDummyGroupTwo);
+  ActiveGroupIdSet expected_group_ids = {field_trial_one, field_trial_two};
+
+  EXPECT_EQ(2, permission_report.field_trials().size());
+  for (auto field_trial : permission_report.field_trials()) {
+    variations::ActiveGroupId group_id = {field_trial.name_id(),
+                                          field_trial.group_id()};
+    ActiveGroupIdSet::iterator expected_group =
+        expected_group_ids.find(group_id);
+    EXPECT_FALSE(expected_group == expected_group_ids.end());
+    expected_group_ids.erase(expected_group);

[raymes, 2016/06/22 01:23:44]

Could you just call erase on group_id? 

[stefanocs, 2016/06/22 02:07:06]

Done.

+  }
+  EXPECT_EQ(0U, expected_group_ids.size());
 
   EXPECT_EQ(GURL(kPermissionActionReportingUploadUrl),
             mock_report_sender_->latest_report_uri());
 }
 
 }  // namespace safe_browsing