Sends switches to the Software Reporter to enable matching data collection.

chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc

 // Copyright 2015 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/srt_fetcher_win.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_path.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/srt_client_info_win.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/component_updater/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 
 namespace safe_browsing {
 
 namespace {
 
 class SRTFetcherTest : public InProcessBrowserTest,
@@ skipping 21 matching lines @@
   int LaunchReporter(const SwReporterInvocation& invocation) override {
     ++reporter_launch_count_;
     reporter_launch_parameters_ = invocation;
     return exit_code_to_report_;
   }
 
   void NotifyLaunchReady() override { launch_ready_notified_ = true; }
 
   void NotifyReporterDone() override { reporter_done_notified_ = true; }
 
-  void SetDaysSinceLastReport(int days) {
+  // Sets |path| in the local state to a date corresponding to |days| days ago.
+  void SetDateInLocalState(const std::string& path, int days) {
     PrefService* local_state = g_browser_process->local_state();
-    local_state->SetInt64(prefs::kSwReporterLastTimeTriggered,
+    DCHECK_NE(local_state, nullptr);
+    local_state->SetInt64(path,
                           (base::Time::Now() - base::TimeDelta::FromDays(days))
                               .ToInternalValue());
   }
 
+  void SetDaysSinceLastReport(int days) {
+    SetDateInLocalState(prefs::kSwReporterLastTimeTriggered, days);
+  }
+
   void ExpectToRunAgain(int days) {
     ASSERT_TRUE(task_runner_->HasPendingTask());
     EXPECT_LE(task_runner_->NextPendingTaskDelay(),
               base::TimeDelta::FromDays(days));
     EXPECT_GT(task_runner_->NextPendingTaskDelay(),
               base::TimeDelta::FromDays(days) - base::TimeDelta::FromHours(1));
   }
 
+  // Sets local state for last time the software reporter sent logs to |days|
+  // days ago.
+  void SetLastTimeSentReport(int days) {
+    SetDateInLocalState(prefs::kSwReporterLastTimeSentReport, days);
+  }
+
+  int64_t GetLastTimeSentReport() {
+    const PrefService* local_state = g_browser_process->local_state();
+    DCHECK_NE(local_state, nullptr);
+    DCHECK(local_state->HasPrefPath(prefs::kSwReporterLastTimeSentReport));
+    return local_state->GetInt64(prefs::kSwReporterLastTimeSentReport);
+  }
+
+  void ExpectLastTimeSentReportNotSet() {
+    PrefService* local_state = g_browser_process->local_state();
+    DCHECK_NE(local_state, nullptr);
+    EXPECT_FALSE(
+        local_state->HasPrefPath(prefs::kSwReporterLastTimeSentReport));
+  }
+
+  void ExpectLastReportSentAsToday() {
+    const PrefService* local_state = g_browser_process->local_state();
+    DCHECK_NE(local_state, nullptr);
+    const base::Time now = base::Time::Now();
+    const base::Time last_time_sent_logs = base::Time::FromInternalValue(
+        local_state->GetInt64(prefs::kSwReporterLastTimeSentReport));
+
+    // Checks if the last time set logs is set as no more than one hour ago,
+    // which should be enough time if the execution does not fail.
+    EXPECT_LT(now - base::TimeDelta::FromHours(1), last_time_sent_logs);

[csharp, 2016/09/02 21:16:25]

This doesn't seem to quite line up with the function name. Could you change one
to keep them in sync.

This also makes me wonder if we should make now() mockable for this test, but
I'm not sure it is worth opening that can of worms

[ftirelo, 2016/09/06 16:37:43]

Done.

+    EXPECT_LT(last_time_sent_logs, now);
+  }
+
   void TestReporterLaunchCycle(int expected_launch_count,
                                const base::FilePath& expected_launch_path) {
     // This test has an unfortunate amount of knowledge of the internals of
     // ReporterRunner, because it needs to pump the right message loops at the
     // right time so that all its internal messages are delivered. This
     // function might need to be updated if the internals change.
     //
     // The basic sequence is:
 
     // 1. TryToRun kicks the whole thing off. If the reporter should not be
@@ skipping 59 matching lines @@
 
     // At this point another call to TryToRun should be scheduled, whether or
     // not LaunchAndWaitForExit was called.
     ASSERT_TRUE(task_runner_->HasPendingTask());
 
     // Make sure the flags are false for the next launch cycle test.
     ASSERT_FALSE(launch_ready_notified_);
     reporter_done_notified_ = false;
   }
 
+  // Expects |reporter_launch_parameters_| contain exactly the command line

[csharp, 2016/09/02 21:16:25]

nit: contain -> to contain(?)

[ftirelo, 2016/09/06 16:37:43]

Done.

+  // switches specified in |expected_switches|.
+  void ExpectLoggingSwitches(const std::set<std::string>& expected_switches) {
+    const base::CommandLine::SwitchMap& invocation_switches =

[csharp, 2016/09/02 21:16:25]

What about using std::set_symmetric_difference

[ftirelo, 2016/09/06 16:37:43]

std::set_symmetric_different relies on the order of the sets. Since we have no
control on how CommandLine::SwitchesMap is implemented (today it's a map, but
there is no guarantee it will not be changed to an unordered_map in the future),
I'd rather not create that dependency now.

Another difficulty in making the change, is that we are not really comparing
collections of the same type (one is a map the other is a set). The function
above will also require either a comparator function or an adapter for the map
iterator (none is available in base/), and I'm not sure if the final outcome
will be worth the added complexity.

I simplified the code though, by checking first if both have the same size.

+        reporter_launch_parameters_.command_line.GetSwitches();
+    // Checks if all expected switches are in the invocation switches.
+    for (const std::string& expected_switch : expected_switches) {
+      EXPECT_NE(invocation_switches.find(expected_switch),
+                invocation_switches.end());
+    }
+    // Checks if all invocation switches are in the expected switches.
+    for (const auto& invocation_switch : invocation_switches) {
+      EXPECT_NE(expected_switches.find(invocation_switch.first),
+                expected_switches.end());
+    }
+  }
+
+  void EnableSBExtendedReporting() {
+    Browser* browser = chrome::FindLastActive();
+    ASSERT_NE(browser, nullptr);
+    Profile* profile = browser->profile();
+    ASSERT_NE(profile, nullptr);
+    profile->GetPrefs()->SetBoolean(
+        prefs::kSafeBrowsingExtendedReportingEnabled, true);
+  }
+
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   bool prompt_trigger_called_ = false;
   int reporter_launch_count_ = 0;
   SwReporterInvocation reporter_launch_parameters_;
   int exit_code_to_report_ = kReporterFailureExitCode;
   bool launch_ready_notified_ = false;
   bool reporter_done_notified_ = false;
 };
 
 }  // namespace
@@ skipping 136 matching lines @@
   // The reporter should launch again using path3, since enough time has
   // passed, even though the parameters haven't changed.
   {
     SCOPED_TRACE("Run with same parameters");
     SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
     RunReporter(path3);
     TestReporterLaunchCycle(3, path3);
   }
 }
 
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_FeatureDisabled) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  RunReporter();
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches({/*expect no switches*/});
+  ExpectLastTimeSentReportNotSet();
+}
+
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_NoSBExtendedReporting) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  RunReporter();
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches({/*expect no switches*/});
+  ExpectLastTimeSentReportNotSet();
+}
+
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledFirstRun) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  EnableSBExtendedReporting();
+  // Note: don't set last time sent logs in the local state.
+  // SBER is enabled and there is no record in the local state of the last time
+  // logs have been sent, so we should send logs in this run.
+  RunReporter();
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches(
+      std::set<std::string>{kExtendedSafeBrowsingEnabledSwitch,

[csharp, 2016/09/02 21:16:25]

Could you make this set a const?

[ftirelo, 2016/09/06 16:37:43]

Done.

+                            kChromeVersionSwitch, kChromeChannelSwitch});
+  ExpectLastReportSentAsToday();
+}
+
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledNoRecentLogging) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  // SBER is enabled and last time logs were sent was more than
+  // |kDaysBetweenReporterLogsSent| day ago, so we should send logs in this run.
+  EnableSBExtendedReporting();
+  SetLastTimeSentReport(kDaysBetweenReporterLogsSent + 3);
+  RunReporter();
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches(
+      std::set<std::string>{kExtendedSafeBrowsingEnabledSwitch,
+                            kChromeVersionSwitch, kChromeChannelSwitch});
+  ExpectLastReportSentAsToday();
+}
+
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledRecentlyLogged) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  // SBER is enabled, but logs have been sent less than
+  // |kDaysBetweenReporterLogsSent| day ago, so we shouldn't send any logs in
+  // this run.
+  EnableSBExtendedReporting();
+  SetLastTimeSentReport(kDaysBetweenReporterLogsSent - 1);
+  int64_t last_time_sent_logs = GetLastTimeSentReport();
+  RunReporter();

[csharp, 2016/09/02 21:16:25]

All of your tests have this same four middle lines, make a helper function?

Or maybe you could just use TestReporterLaunchCycle

[ftirelo, 2016/09/06 16:37:42]

Done.

+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches(std::set<std::string>{/*expect no switches*/});
+  EXPECT_EQ(last_time_sent_logs, GetLastTimeSentReport());
+}
+
 }  // namespace safe_browsing