Update SRTFetcher browser test to use ScopedMockTimeMessageLoopTaskRunner

chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc

Index: chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
diff --git a/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc b/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
index 01409845850f07e3bd50b70e25bc5b4ebfc72b31..eabbd238e786e0194d7891528571023096dd1f3d 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
+++ b/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
@@ -10,12 +10,15 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback.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/test/scoped_mock_time_message_loop_task_runner.h"
 #include "base/time/time.h"
+#include "base/version.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/lifetime/keep_alive_types.h"
+#include "chrome/browser/lifetime/scoped_keep_alive.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/srt_client_info_win.h"
 #include "chrome/browser/ui/browser.h"
@@ -25,7 +28,6 @@
 #include "components/component_updater/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing_db/safe_browsing_prefs.h"
-#include "content/public/test/test_browser_thread_bundle.h"
 
 namespace safe_browsing {
 
@@ -39,20 +41,43 @@ class SRTFetcherTest : public InProcessBrowserTest,
                        public SwReporterTestingDelegate {
  public:
   void SetUpInProcessBrowserTestFixture() override {
-    task_runner_ = new base::TestSimpleTaskRunner;
-
     SetSwReporterTestingDelegate(this);
   }
 
   void SetUpOnMainThread() override {
+    scoped_task_runner_ =
+        std::make_unique<base::ScopedMockTimeMessageLoopTaskRunner>();

[gab, 2017/02/21 22:15:36]

std::make_unique is C++14, you still need to use base::MakeUnique from
ptr_util.h for now in Chromium.

[Joe Mason, 2017/02/22 04:43:37]

Huh, I've been using std::make_unique a lot. Surprised that didn't hit me
before. Done.

     InProcessBrowserTest::SetUpOnMainThread();
+
+    // Move the simulated clock ahead far enough that times in the past won't
+    // underflow.

[gab, 2017/02/21 22:15:36]

I don't understand this?

[Joe Mason, 2017/02/22 04:43:37]

Reworded. Better?

+    task_runner()->FastForwardBy(
+        base::TimeDelta::FromDays(kDaysBetweenSuccessfulSwReporterRuns * 2));
+
     ClearLastTimeSentReport();
   }
 
+  void TearDownOnMainThread() override {
+    // ScopedMockTimeMessageLoopTaskRunner's destructor re-posts pending tasks
+    // to the previous task runner. Get rid of them so they don't run after the
+    // test.
+    task_runner()->ClearPendingTasks();

[gab, 2017/02/21 22:15:36]

It's weird to drop tasks, can we run remaining non-delayed tasks instead?

This makes me think ScopedMockTimeMessageLoopTaskRunner might just want to run
remaining tasks and drop delayed tasks in its destructor (it's brand new,
re-posting made sense initially but maybe that's more trouble than it saves?).

[Joe Mason, 2017/02/22 04:43:37]

The problem is this line here:

https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test...

  // Sometimes tests leave Quit tasks in the MessageLoop (for shame), so let's
  // run all pending messages here to avoid preempting the QuitBrowsers tasks.
  // TODO(jbates) Once crbug.com/134753 is fixed, this can be removed because it
  // will not be possible to post Quit tasks.
  content::RunAllPendingInMessageLoop();

That runs all tasks, not just non-delayed tasks, so if we leave a task that's
scheduled to run in 7 days the test will always time out. 
That would work much better for this use case, and it's easier to reason about
too. I'm not sure if there are other things in the browser tear-down that
require me to delete the ScopedMockTimeMessageLoopTaskRunner first, but it would
be nice to be able to just use it directly in browser tests like you can in unit
tests.

[gab, 2017/02/22 17:19:48]

Hmmm, I doubt this is true? i.e. I don't think delayed tasks have to expire for
this to return.

+
+    // Restore the standard task runner to perform browser cleanup, which posts
+    // some tasks with pending times but does not advance
+    // ScopedMockTimeMessageLoopTaskRunner's mock clock.
+    scoped_task_runner_.reset();
+  }
+
   void TearDownInProcessBrowserTestFixture() override {
     SetSwReporterTestingDelegate(nullptr);
   }
 
+  base::TestMockTimeTaskRunner* task_runner() const {
+    DCHECK(scoped_task_runner_);
+    return scoped_task_runner_->task_runner();

[gab, 2017/02/21 22:15:36]

ScopedMockTimeMessageLoopTaskRunner overrides operator->() specifically so you
can use it like a base::TestMockTimeTaskRunner* so this should be unecessary.

[Joe Mason, 2017/02/22 04:43:37]

Done.

Didn't notice that. Thanks! 

+  }
+
   void RunReporter(const base::FilePath& exe_path = base::FilePath()) {
     auto invocation = SwReporterInvocation::FromFilePath(exe_path);
     invocation.supported_behaviours =
@@ -63,13 +88,11 @@ class SRTFetcherTest : public InProcessBrowserTest,
 
     SwReporterQueue invocations;
     invocations.push(invocation);
-    RunSwReporters(invocations, base::Version("1.2.3"), task_runner_,
-                   task_runner_);
+    RunSwReporters(invocations, base::Version("1.2.3"));
   }
 
   void RunReporterQueue(const SwReporterQueue& invocations) {
-    RunSwReporters(invocations, base::Version("1.2.3"), task_runner_,
-                   task_runner_);
+    RunSwReporters(invocations, base::Version("1.2.3"));
   }
 
   void TriggerPrompt(Browser* browser, const std::string& version) override {
@@ -78,21 +101,26 @@ class SRTFetcherTest : public InProcessBrowserTest,
 
   int LaunchReporter(const SwReporterInvocation& invocation) override {
     ++reporter_launch_count_;
-    reporter_launch_parameters_ = invocation;
+    reporter_launch_parameters_.push_back(invocation);
+    if (first_launch_callback_) {
+      // This is a OnceCallback so it will be cleared after running.

[gab, 2017/02/21 22:15:37]

Don't think you need this, it'll become natural as people get used to
OnceClosure and having such a comment at every such caller will be overkill.

(and anyone that wonders and tries to remove the move will hit a compile error)

[Joe Mason, 2017/02/22 04:43:37]

Done.

+      std::move(first_launch_callback_).Run();
+    }
     return exit_code_to_report_;
   }
 
-  void NotifyLaunchReady() override { launch_ready_notified_ = true; }
+  base::Time Now() const override { return task_runner()->Now(); }
 
-  void NotifyReporterDone() override { reporter_done_notified_ = true; }
+  base::TaskRunner* BlockingTaskRunner() const override {
+    return task_runner();
+  }
 
   // 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();
     DCHECK_NE(local_state, nullptr);
-    local_state->SetInt64(path,
-                          (base::Time::Now() - base::TimeDelta::FromDays(days))
-                              .ToInternalValue());
+    local_state->SetInt64(
+        path, (Now() - base::TimeDelta::FromDays(days)).ToInternalValue());
   }
 
   void SetDaysSinceLastReport(int days) {
@@ -100,10 +128,10 @@ class SRTFetcherTest : public InProcessBrowserTest,
   }
 
   void ExpectToRunAgain(int days) {
-    ASSERT_TRUE(task_runner_->HasPendingTask());
-    EXPECT_LE(task_runner_->NextPendingTaskDelay(),
+    ASSERT_TRUE(task_runner()->HasPendingTask());
+    EXPECT_LE(task_runner()->NextPendingTaskDelay(),
               base::TimeDelta::FromDays(days));
-    EXPECT_GT(task_runner_->NextPendingTaskDelay(),
+    EXPECT_GT(task_runner()->NextPendingTaskDelay(),
               base::TimeDelta::FromDays(days) - base::TimeDelta::FromHours(1));
   }
 
@@ -140,164 +168,59 @@ class SRTFetcherTest : public InProcessBrowserTest,
   void ExpectLastReportSentInTheLastHour() {
     const PrefService* local_state = g_browser_process->local_state();
     DCHECK_NE(local_state, nullptr);
-    const base::Time now = base::Time::Now();
+    const base::Time now = Now();
     const base::Time last_time_sent_logs = base::Time::FromInternalValue(
         local_state->GetInt64(prefs::kSwReporterLastTimeSentReport));
 
     // Checks if the last time sent 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);
-    EXPECT_LT(last_time_sent_logs, now);
-  }
-
-  // Run through the steps needed to launch the reporter, as many times as
-  // needed to launch all the reporters given in |expected_launch_paths|. Test
-  // that each of those launches succeeded. But do not test that ONLY those
-  // launches succeeded.
-  //
-  // After this, if more launches are expected you can call
-  // |TestPartialLaunchCycle| again with another list of paths, to test that
-  // the launch cycle will continue with those paths.
-  //
-  // To test that a list of paths are launched AND NO OTHERS, use
-  // |TestReporterLaunchCycle|.
-  void TestPartialLaunchCycle(
-      const std::vector<base::FilePath>& expected_launch_paths) {
-    // 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
-    // launched now (eg. DaysSinceLastReport is too low) it posts a call to
-    // itself again. (In a regular task runner this will be scheduled with a
-    // delay, but the test task runner ignores delays so TryToRun will be
-    // called again on the next call to RunPendingTasks.)
-    //
-    // 2. When it is time to run a reporter, TryToRun calls NotifyLaunchReady
-    // and then posts a call to LaunchAndWait.
-    //
-    // 3. When the reporter returns, a call to ReporterDone is posted on the UI
-    // thread.
-    //
-    // 4. ReporterDone calls NotifyReporterDone and then posts another call to
-    // TryToRun, which starts the whole process over for the next run.
-    //
-    // Each call to RunPendingTasks only handles messages already on the queue.
-    // It doesn't handle messages posted by those messages. So, we need to call
-    // it in a loop to make sure we're past all pending TryToRun calls before
-    // LaunchAndWaitForExit will be called.
-    //
-    // Once a call to LaunchAndWaitForExit has been posted, TryToRun won't be
-    // called again until we pump the UI message loop in order to run
-    // ReporterDone.
-
-    ASSERT_TRUE(task_runner_->HasPendingTask());
-    ASSERT_FALSE(reporter_done_notified_);
-
-    reporter_launch_count_ = 0;
-    reporter_launch_parameters_ = SwReporterInvocation();
-
-    int current_launch_count = reporter_launch_count_;
-    for (const auto& expected_launch_path : expected_launch_paths) {
-      // If RunReporter was called with no pending messages, and it was already
-      // time to launch the reporter, then |launch_ready_notified_| will
-      // already be true. Otherwise there will be a TryToRun message pending,
-      // which must be processed first.
-      if (!launch_ready_notified_) {
-        task_runner_->RunPendingTasks();
-        // Since we're expecting a launch here, we expect it to schedule
-        // LaunchAndWaitForExit. So NOW |launch_ready_notified_| should be
-        // true.
-        ASSERT_TRUE(task_runner_->HasPendingTask());
-      }
-      ASSERT_TRUE(launch_ready_notified_);
-      ASSERT_EQ(current_launch_count, reporter_launch_count_);
-
-      // Reset |launch_ready_notified_| so that we can tell if TryToRun gets
-      // called again unexpectedly.
-      launch_ready_notified_ = false;
-
-      // Call the pending LaunchAndWaitForExit.
-      task_runner_->RunPendingTasks();
-      ASSERT_FALSE(launch_ready_notified_);
-      ASSERT_FALSE(reporter_done_notified_);
-
-      // At this point LaunchAndWaitForExit has definitely been called if
-      // it's going to be called at all. (If not, TryToRun will have been
-      // scheduled again.)
-      EXPECT_EQ(current_launch_count + 1, reporter_launch_count_);
-      EXPECT_EQ(expected_launch_path,
-                reporter_launch_parameters_.command_line.GetProgram());
-
-      // Pump the UI message loop to process the ReporterDone call (which
-      // will schedule the next TryToRun.) If LaunchAndWaitForExit wasn't
-      // called, this does nothing.
-      base::RunLoop().RunUntilIdle();
-
-      // At this point there are three things that could have happened:
-      //
-      // 1. LaunchAndWaitForExit was not called. There should be a TryToRun
-      // scheduled.
-      //
-      // 2. ReporterDone was called and there was nothing left in the queue
-      // of SwReporterInvocation's. There should be a TryToRun scheduled.
-      //
-      // 3. ReporterDone was called and there were more
-      // SwReporterInvocation's in the queue to run immediately. There should
-      // be a LaunchAndWaitForExit scheduled.
-      //
-      // So in all cases there should be a pending task, and if we are expecting
-      // more launches in this loop, |launch_ready_notified_| will already be
-      // true.
-      ASSERT_TRUE(task_runner_->HasPendingTask());
-
-      // The test task runner does not actually advance the clock. Pretend that
-      // one day has passed. (Otherwise, when we launch the last
-      // SwReporterInvocation in the queue, the next call to TryToRun will
-      // start a whole new launch cycle.)
-      SetDaysSinceLastReport(1);
-
-      reporter_done_notified_ = false;
-      current_launch_count = reporter_launch_count_;
-    }
+    EXPECT_LE(last_time_sent_logs, now);
   }
 
-  // Run through the steps needed to launch the reporter, as many times as
-  // needed to launch all the reporters given in |expected_launch_paths|. Test
-  // that each of those launches succeeded. Then, run through the steps needed
-  // to launch the reporter again, to test that the launch cycle is complete
-  // (no more reporters will be launched).
-  void TestReporterLaunchCycle(
-      const std::vector<base::FilePath>& expected_launch_paths) {
-    TestPartialLaunchCycle(expected_launch_paths);
-
-    // Now that all expected launches have been tested, run the cycle once more
-    // to make sure no more launches happen.
-    ASSERT_TRUE(task_runner_->HasPendingTask());
-    ASSERT_FALSE(reporter_done_notified_);
-    ASSERT_FALSE(launch_ready_notified_);
+  // Expects that after |days_until_launched| days, the reporter will be
+  // launched // |expected_launch_count| times, and TriggerPrompt will be
+  // called if and only if |expect_prompt| is true.
+  void ExpectReporterLaunches(int days_until_launch,
+                              int expected_launch_count,
+                              bool expect_prompt) {
+    ASSERT_TRUE(task_runner()->HasPendingTask());
+    reporter_launch_count_ = 0;
+    reporter_launch_parameters_.clear();
+    prompt_trigger_called_ = false;
 
-    int current_launch_count = reporter_launch_count_;
+    task_runner()->FastForwardBy(base::TimeDelta::FromDays(days_until_launch));
 
-    // Call the pending TryToRun.
-    task_runner_->RunPendingTasks();
+    EXPECT_EQ(reporter_launch_count_, expected_launch_count);
+    EXPECT_EQ(prompt_trigger_called_, expect_prompt);
+  }
 
-    // We expect that this scheduled another TryToRun. If it scheduled
-    // LaunchAndWaitForExit an unexpected launch is about to happen.
-    ASSERT_TRUE(task_runner_->HasPendingTask());
-    ASSERT_FALSE(launch_ready_notified_);
-    ASSERT_FALSE(reporter_done_notified_);
-    ASSERT_EQ(current_launch_count, reporter_launch_count_);
+  // Expects that after |days_until_launched| days, the reporter will be
+  // launched once with each path in |expected_launch_paths|, and TriggerPrompt
+  // will be called if and only if |expect_prompt| is true.
+  void ExpectReporterLaunches(
+      int days_until_launch,
+      const std::vector<base::FilePath>& expected_launch_paths,
+      bool expect_prompt) {
+    ExpectReporterLaunches(days_until_launch, expected_launch_paths.size(),
+                           expect_prompt);
+    ASSERT_EQ(reporter_launch_parameters_.size(), expected_launch_paths.size());
+    for (size_t i = 0; i < expected_launch_paths.size(); ++i) {
+      EXPECT_EQ(expected_launch_paths[i],
+                reporter_launch_parameters_[i].command_line.GetProgram());
+    }
   }
 
-  // Expects |reporter_launch_parameters_| to contain exactly the command line
-  // switches specified in |expected_switches|.
-  void ExpectLoggingSwitches(const std::set<std::string>& expected_switches) {
+  // Expects |invocation|'s command line to contain all the switches required
+  // for reporter logging.
+  void ExpectLoggingSwitches(const SwReporterInvocation& invocation,
+                             bool expect_switches) {
     const base::CommandLine::SwitchMap& invocation_switches =
-        reporter_launch_parameters_.command_line.GetSwitches();
+        invocation.command_line.GetSwitches();
+    std::set<std::string> expected_switches;
+    if (expect_switches)
+      expected_switches = {std::begin(kExpectedSwitches),
+                           std::end(kExpectedSwitches)};
     EXPECT_EQ(expected_switches.size(), invocation_switches.size());
     // Checks if all expected switches are in the invocation switches. It's not
     // necessary to check if all invocation switches are expected, since we
@@ -316,17 +239,20 @@ class SRTFetcherTest : public InProcessBrowserTest,
     SetExtendedReportingPref(profile->GetPrefs(), true);
   }
 
-  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  // Replaces the main MessageLoop's TaskRunner with a TaskRunner on which time
+  // is mocked to allow testing of scheduled launches.
+  std::unique_ptr<base::ScopedMockTimeMessageLoopTaskRunner>
+      scoped_task_runner_;
+
   bool prompt_trigger_called_ = false;
   int reporter_launch_count_ = 0;
-  SwReporterInvocation reporter_launch_parameters_;
+  std::vector<SwReporterInvocation> reporter_launch_parameters_;
   int exit_code_to_report_ = kReporterFailureExitCode;
 
-  // This will be set to true when a call to |LaunchAndWaitForExit| is next in
-  // the task queue.
-  bool launch_ready_notified_ = false;
-
-  bool reporter_done_notified_ = false;
+  // A callback to invoke when the first reporter of a queue is launched. This
+  // can be used to perform actions in the middle of a queue of reporters which
+  // all launch on the same mock clock tick.
+  base::OnceCallback<void()> first_launch_callback_;

[gab, 2017/02/21 22:15:37]

Use OnceClosure instead of OnceCallback<void()>

[Joe Mason, 2017/02/22 04:43:37]

Done.

 };
 
 }  // namespace
@@ -334,24 +260,14 @@ class SRTFetcherTest : public InProcessBrowserTest,
 IN_PROC_BROWSER_TEST_F(SRTFetcherTest, NothingFound) {
   exit_code_to_report_ = kSwReporterNothingFound;
   RunReporter();
-  task_runner_->RunPendingTasks();
-  EXPECT_EQ(1, reporter_launch_count_);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(prompt_trigger_called_);
+  ExpectReporterLaunches(0, 1, false);
   ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 }
 
 IN_PROC_BROWSER_TEST_F(SRTFetcherTest, CleanupNeeded) {
   exit_code_to_report_ = kSwReporterCleanupNeeded;
   RunReporter();
-
-  task_runner_->RunPendingTasks();
-  EXPECT_EQ(1, reporter_launch_count_);
-  // The reply task from the task posted to run the reporter is run on a
-  // specific thread, as opposed to a specific task runner, and that thread is
-  // the current message loop's thread.
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(prompt_trigger_called_);
+  ExpectReporterLaunches(0, 1, true);
   ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 }
 
@@ -359,27 +275,51 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, RanRecently) {
   constexpr int kDaysLeft = 1;
   SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns - kDaysLeft);
   RunReporter();
-
-  // Here we can't run until idle since the ReporterRunner will re-post
-  // infinitely.
-  task_runner_->RunPendingTasks();
-  EXPECT_EQ(0, reporter_launch_count_);
-
+  ExpectReporterLaunches(0, 0, false);
   ExpectToRunAgain(kDaysLeft);
-  task_runner_->ClearPendingTasks();
+  ExpectReporterLaunches(kDaysLeft, 1, false);
+  ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 }
 
 IN_PROC_BROWSER_TEST_F(SRTFetcherTest, WaitForBrowser) {
   Profile* profile = browser()->profile();
-  CloseAllBrowsers();
 
+  // Ensure that even though we're closing the last browser, we don't enter the
+  // "shutting down" state, which will prevent the test from starting another
+  // browser.
+  ScopedKeepAlive test_keep_alive(KeepAliveOrigin::SESSION_RESTORE,
+                                  KeepAliveRestartOption::ENABLED);
+
+  // Restore the standard task runner to perform browser cleanup, which posts
+  // some tasks with pending times but does not advance
+  // ScopedMockTimeMessageLoopTaskRunner's mock clock.
+  scoped_task_runner_.reset();
+  CloseBrowserSynchronously(browser());
+  scoped_task_runner_ =
+      std::make_unique<base::ScopedMockTimeMessageLoopTaskRunner>();
+  ASSERT_EQ(0u, chrome::GetTotalBrowserCount());
+  ASSERT_FALSE(chrome::FindLastActive());
+
+  // Start the reporter while the browser is closed. The prompt should not open.
   exit_code_to_report_ = kSwReporterCleanupNeeded;
   RunReporter();
-
-  task_runner_->RunPendingTasks();
+  ExpectReporterLaunches(0, 1, false);
+
+  // Create a Browser object directly instead of using helper functions like
+  // CreateBrowser, because they all wait on timed events but do not advance the
+  // mock timer. The Browser constructor registers itself with the global
+  // BrowserList, which is cleaned up when InProcessBrowserTest exits.
+  new Browser(Browser::CreateParams(profile));
+  ASSERT_EQ(1u, chrome::GetTotalBrowserCount());
+
+  // Some browser startup tasks are scheduled to run in the first few minutes
+  // after creation. Make sure they've all been processed so that the only
+  // pending task in the queue is the next reporter check.
+  task_runner()->FastForwardBy(base::TimeDelta::FromMinutes(10));
+
+  // On opening the new browser, the prompt should be shown and the reporter
+  // should be scheduled to run later.
   EXPECT_EQ(1, reporter_launch_count_);
-
-  CreateBrowser(profile);
   EXPECT_TRUE(prompt_trigger_called_);
   ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 }
@@ -387,12 +327,7 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, WaitForBrowser) {
 IN_PROC_BROWSER_TEST_F(SRTFetcherTest, Failure) {
   exit_code_to_report_ = kReporterFailureExitCode;
   RunReporter();
-
-  task_runner_->RunPendingTasks();
-  EXPECT_EQ(1, reporter_launch_count_);
-
-  base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(prompt_trigger_called_);
+  ExpectReporterLaunches(0, 1, false);
   ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 }
 
@@ -405,16 +340,24 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, RunDaily) {
             kDaysBetweenSwReporterRunsForPendingPrompt);
   RunReporter();
 
-  task_runner_->RunPendingTasks();
-  EXPECT_EQ(1, reporter_launch_count_);
-  reporter_launch_count_ = 0;
-  base::RunLoop().RunUntilIdle();
+  // Expect the reporter to run immediately, since a prompt is pending and it
+  // has been more than kDaysBetweenSwReporterRunsForPendingPrompt days.
+  ExpectReporterLaunches(0, 1, false);
   ExpectToRunAgain(kDaysBetweenSwReporterRunsForPendingPrompt);
 
+  // Move the clock ahead kDaysBetweenSwReporterRunsForPendingPrompt days. The
+  // expected run should trigger, but not cause the reporter to launch because
+  // a prompt is no longer pending.
   local_state->SetBoolean(prefs::kSwReporterPendingPrompt, false);
-  task_runner_->RunPendingTasks();
-  EXPECT_EQ(0, reporter_launch_count_);
-  base::RunLoop().RunUntilIdle();
+  ExpectReporterLaunches(kDaysBetweenSwReporterRunsForPendingPrompt, 0, false);
+
+  // Instead it should now run kDaysBetweenSuccessfulSwReporterRuns after the
+  // first prompt (of which kDaysBetweenSwReporterRunsForPendingPrompt has
+  // already passed.)
+  int days_left = kDaysBetweenSuccessfulSwReporterRuns -
+                  kDaysBetweenSwReporterRunsForPendingPrompt;
+  ExpectToRunAgain(days_left);
+  ExpectReporterLaunches(days_left, 1, false);
   ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 }
 
@@ -434,27 +377,25 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
     SCOPED_TRACE("N days left until next reporter run");
     SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns - kDaysLeft);
     RunReporter(path1);
-    TestReporterLaunchCycle({});
+    ExpectReporterLaunches(0, {}, false);
   }
 
   // Schedule path2 just as we enter the next reporting period.
   // Now the reporter should launch, just once, using path2.
   {
     SCOPED_TRACE("Reporter runs now");
-    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
     RunReporter(path2);
     // Schedule it twice; it should only actually run once.
     RunReporter(path2);
-    TestReporterLaunchCycle({path2});
+    ExpectReporterLaunches(kDaysLeft, {path2}, false);
   }
 
   // Schedule path3 before any more time has passed.
   // The reporter should not launch.
   {
     SCOPED_TRACE("No more time passed");
-    SetDaysSinceLastReport(0);
     RunReporter(path3);
-    TestReporterLaunchCycle({});
+    ExpectReporterLaunches(0, {}, false);
   }
 
   // Enter the next reporting period as path3 is still scheduled.
@@ -462,8 +403,8 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
   // parameters from the first launch aren't reused.)
   {
     SCOPED_TRACE("Previous run still scheduled");
-    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
-    TestReporterLaunchCycle({path3});
+    ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns, {path3},
+                           false);
   }
 
   // Schedule path3 again in the next reporting period.
@@ -471,9 +412,9 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
   // passed, even though the parameters haven't changed.
   {
     SCOPED_TRACE("Run with same parameters");
-    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
     RunReporter(path3);
-    TestReporterLaunchCycle({path3});
+    ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns, {path3},
+                           false);
   }
 }
 
@@ -492,56 +433,58 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, MultipleLaunches) {
     SCOPED_TRACE("Launch 2 times");
     SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
     RunReporterQueue(invocations);
-    TestReporterLaunchCycle({path1, path2});
+    ExpectReporterLaunches(0, {path1, path2}, false);
+    ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
   }
 
   // Schedule a launch with 2 elements, then another with the same 2. It should
   // just run 2 times, not 4.
   {
     SCOPED_TRACE("Launch 2 times with retry");
-    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
     RunReporterQueue(invocations);
     RunReporterQueue(invocations);
-    TestReporterLaunchCycle({path1, path2});
+    ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns, {path1, path2},
+                           false);
+    ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
   }
 
-  // Schedule a launch with 2 elements, then add a third while the queue is
-  // running.
+  // Another launch with 2 elements is already scheduled. Add a third while the
+  // queue is running.
   {
     SCOPED_TRACE("Add third launch while running");
-    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
-    RunReporterQueue(invocations);
-
-    // Only test the cycle once, to process the first element in queue.
-    TestPartialLaunchCycle({path1});
-
     invocations.push(SwReporterInvocation::FromFilePath(path3));
-    RunReporterQueue(invocations);
+    first_launch_callback_ = base::BindOnce(
+        &SRTFetcherTest::RunReporterQueue, base::Unretained(this), invocations);
 
-    // There is still a 2nd element on the queue - that should execute, and
-    // nothing more.
-    TestReporterLaunchCycle({path2});
+    // Only the first two elements should execute since the third was added
+    // during the cycle.
+    ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns, {path1, path2},
+                           false);
+    ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 
     // Time passes... Now the 3-element queue should run.
-    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
-    TestReporterLaunchCycle({path1, path2, path3});
+    ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns,
+                           {path1, path2, path3}, false);
+    ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
   }
 
   // Second launch should not occur after a failure.
   {
     SCOPED_TRACE("Launch multiple times with failure");
     exit_code_to_report_ = kReporterFailureExitCode;
-    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
     RunReporterQueue(invocations);
-    TestReporterLaunchCycle({path1});
+    ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns, {path1},
+                           false);
+    ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
 
     // If we try again before the reporting period is up, it should not do
     // anything.
-    TestReporterLaunchCycle({});
+    ExpectReporterLaunches(0, {}, false);
 
     // After enough time has passed, should try the queue again.
-    SetDaysSinceLastReport(kDaysBetweenSuccessfulSwReporterRuns);
-    TestReporterLaunchCycle({path1});
+    ExpectReporterLaunches(kDaysBetweenSuccessfulSwReporterRuns, {path1},
+                           false);
+    ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns);
   }
 }
 
@@ -549,8 +492,8 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_NoSBExtendedReporting) {
   exit_code_to_report_ = kSwReporterNothingFound;
   base::test::ScopedFeatureList scoped_feature_list;
   RunReporter();
-  TestReporterLaunchCycle({base::FilePath()});
-  ExpectLoggingSwitches({/*expect no switches*/});
+  ExpectReporterLaunches(0, 1, false);
+  ExpectLoggingSwitches(reporter_launch_parameters_.front(), false);
   ExpectLastTimeSentReportNotSet();
 }
 
@@ -562,9 +505,8 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledFirstRun) {
   // 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();
-  TestReporterLaunchCycle({base::FilePath()});
-  ExpectLoggingSwitches(std::set<std::string>(std::begin(kExpectedSwitches),
-                                              std::end(kExpectedSwitches)));
+  ExpectReporterLaunches(0, 1, false);
+  ExpectLoggingSwitches(reporter_launch_parameters_.front(), true);
   ExpectLastReportSentInTheLastHour();
 }
 
@@ -576,9 +518,8 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledNoRecentLogging) {
   EnableSBExtendedReporting();
   SetLastTimeSentReport(kDaysBetweenReporterLogsSent + 3);
   RunReporter();
-  TestReporterLaunchCycle({base::FilePath()});
-  ExpectLoggingSwitches(std::set<std::string>(std::begin(kExpectedSwitches),
-                                              std::end(kExpectedSwitches)));
+  ExpectReporterLaunches(0, 1, false);
+  ExpectLoggingSwitches(reporter_launch_parameters_.front(), true);
   ExpectLastReportSentInTheLastHour();
 }
 
@@ -592,8 +533,8 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledRecentlyLogged) {
   SetLastTimeSentReport(kDaysBetweenReporterLogsSent - 1);
   int64_t last_time_sent_logs = GetLastTimeSentReport();
   RunReporter();
-  TestReporterLaunchCycle({base::FilePath()});
-  ExpectLoggingSwitches(std::set<std::string>{/*expect no switches*/});
+  ExpectReporterLaunches(0, 1, false);
+  ExpectLoggingSwitches(reporter_launch_parameters_.front(), false);
   EXPECT_EQ(last_time_sent_logs, GetLastTimeSentReport());
 }
 
@@ -618,19 +559,18 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_MultipleLaunches) {
   // |kDaysBetweenReporterLogsSent| day ago, so we should send logs in this run.
   {
     SCOPED_TRACE("first launch");
-    TestPartialLaunchCycle({path1});
-    ExpectLoggingSwitches(std::set<std::string>(std::begin(kExpectedSwitches),
-                                                std::end(kExpectedSwitches)));
-    ExpectLastReportSentInTheLastHour();
+    first_launch_callback_ =
+        base::BindOnce(&SRTFetcherTest::ExpectLastReportSentInTheLastHour,
+                       base::Unretained(this));
+    ExpectReporterLaunches(0, {path1, path2}, false);
+    ExpectLoggingSwitches(reporter_launch_parameters_[0], true);
   }
 
   // Logs should also be sent for the next run, even though LastTimeSentReport
   // is now recent, because the run is part of the same set of invocations.
   {
     SCOPED_TRACE("second launch");
-    TestReporterLaunchCycle({path2});
-    ExpectLoggingSwitches(std::set<std::string>(std::begin(kExpectedSwitches),
-                                                std::end(kExpectedSwitches)));
+    ExpectLoggingSwitches(reporter_launch_parameters_[1], true);
     ExpectLastReportSentInTheLastHour();
   }
 }