Revert "Removed ProcessLifetimeObserver from TestLauncher."

This reverts commit 58756aea.

Reason for revert: suspected to a cause Linux Tsan failures see
crbug.com/985799

Builders failed on:
- Linux TSan Tests:
  https://ci.chromium.org/p/chromium/builders/ci/Linux%20TSan%20Tests
- Linux TSan Tests:
  https://ci.chromium.org/p/chromium/builders/ci/Linux%20TSan%20Tests
- Linux TSan Tests:
  https://ci.chromium.org/p/chromium/builders/ci/Linux%20TSan%20Tests

Original change's description:
> Removed ProcessLifetimeObserver from TestLauncher.
>
> The goal of this cl is to move all child process logic to TestLauncher.
> This should simplify the gtest launcher structure and clarify
> each class responsibilities.
>
> TestRunner controls running test processes across sequence runners.
>
> TestLauncherDelegate is now limited to provide test specific needs.
> Command line for tests, timeout, result processing, etc.
>
> This allows us to remove the ProcessLifetimeObserver and its extending
> classes
>
> Bug: 936248
> Change-Id: I2165d9f5a295f153bd87e0155aabf9316acabfc6
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677241
> Reviewed-by: Erik Chen <erikchen@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Commit-Queue: Ilia Samsonov <isamsonov@google.com>
> Cr-Commit-Position: refs/heads/master@{#677409}

TBR=sky@chromium.org,erikchen@chromium.org,isamsonov@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 936248, 985799
Change-Id: I169c720169564182e3eae8dcd391ef2eeb33c225
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1719485Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#681221}

base/test/launcher/test_launcher.h

@@ -13,7 +13,6 @@
 #include <string>
 #include <vector>
 
-#include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/process/launch.h"
@@ -27,7 +26,10 @@
 
 namespace base {
 
+class CommandLine;
 struct LaunchOptions;
+class TestLauncher;
+
 // Constants for GTest command-line flags.
 extern const char kGTestFilterFlag[];
 extern const char kGTestFlagfileFlag[];
@@ -55,48 +57,53 @@ class TestLauncherDelegate {
   virtual bool WillRunTest(const std::string& test_case_name,
                            const std::string& test_name) = 0;
 
-  // Invoked after a child process finishes, reporting the process |exit_code|,
-  // child process |elapsed_time|, whether or not the process was terminated as
-  // a result of a timeout, and the output of the child (stdout and stderr
-  // together). NOTE: this method is invoked on the same thread as
-  // LaunchChildGTestProcess.
-  // Returns test results of child process.
-  virtual std::vector<TestResult> ProcessTestResults(
-      const std::vector<std::string>& test_names,
-      const base::FilePath& output_file,
-      const std::string& output,
-      const base::TimeDelta& elapsed_time,
-      int exit_code,
-      bool was_timeout) = 0;
-
-  // Called to get the command line for the specified tests.
-  // |output_file_| is populated with the path to the result file, and must
-  // be inside |temp_dir|.
-  virtual CommandLine GetCommandLine(const std::vector<std::string>& test_names,
-                                     const FilePath& temp_dir,
-                                     FilePath* output_file) = 0;
+  // Called to make the delegate run the specified tests. The delegate must
+  // return the number of actual tests it's going to run (can be smaller,
+  // equal to, or larger than size of |test_names|). It must also call
+  // |test_launcher|'s OnTestFinished method once per every run test,
+  // regardless of its success.
+  // If test_names contains PRE_ chained tests, they must be properly ordered
+  // and consecutive.
+  virtual size_t RunTests(TestLauncher* test_launcher,
+                          const std::vector<std::string>& test_names) = 0;
+
+  // Called to make the delegate retry the specified tests. The delegate must
+  // return the number of actual tests it's going to retry (can be smaller,
+  // equal to, or larger than size of |test_names|). It must also call
+  // |test_launcher|'s OnTestFinished method once per every retried test,
+  // regardless of its success.
+  virtual size_t RetryTests(TestLauncher* test_launcher,
+                            const std::vector<std::string>& test_names) = 0;
+
+  virtual ~TestLauncherDelegate();
+};
+
+// An observer of child process lifetime events generated by
+// LaunchChildGTestProcess.
+class ProcessLifetimeObserver {
+ public:
+  virtual ~ProcessLifetimeObserver() = default;
 
   // Invoked when a test process exceeds its runtime, immediately before it is
   // terminated. |command_line| is the command line used to launch the process.
   // NOTE: this method is invoked on the thread the process is launched on.
-  virtual void OnTestTimedOut(const base::CommandLine& cmd_line) {}
+  virtual void OnTimedOut(const CommandLine& command_line) {}
 
-  // Returns the delegate specific wrapper for command line.
-  // If it is not empty, it is prepended to the final command line.
-  virtual std::string GetWrapper() = 0;
-
-  // Returns the delegate specific flags for launch options.
-  // The flags are specified in LaunchChildGTestProcessFlags.
-  virtual int GetLaunchOptions() = 0;
-
-  // Returns the delegate specific timeout per test.
-  virtual TimeDelta GetTimeout() = 0;
-
-  // Returns the delegate specific batch size.
-  virtual size_t GetBatchSize() = 0;
+  // Invoked after a child process finishes, reporting the process |exit_code|,
+  // child process |elapsed_time|, whether or not the process was terminated as
+  // a result of a timeout, and the output of the child (stdout and stderr
+  // together). NOTE: this method is invoked on the same thread as
+  // LaunchChildGTestProcess.
+  virtual void OnCompleted(int exit_code,
+                           TimeDelta elapsed_time,
+                           bool was_timeout,
+                           const std::string& output) {}
 
  protected:
-  virtual ~TestLauncherDelegate();
+  ProcessLifetimeObserver() = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ProcessLifetimeObserver);
 };
 
 // Launches tests using a TestLauncherDelegate.
@@ -144,16 +151,18 @@ class TestLauncher {
   // if null, uses command line for current process.
   bool Run(CommandLine* command_line = nullptr) WARN_UNUSED_RESULT;
 
-  // Launches a child process (assumed to be gtest-based binary) which runs
-  // tests indicated by |test_names|.
-  // |task_runner| is used to post results back to the launcher
-  // on the main thread. |temp_dir| is used for child process files,
-  // such as user data, result file, and flag_file.
+  // Launches a child process (assumed to be gtest-based binary) using
+  // |command_line|. If |wrapper| is not empty, it is prepended to the final
+  // command line. |observer|, if not null, is used to convey process lifetime
+  // events to the caller. |observer| is destroyed after its OnCompleted
+  // method is invoked.
   // virtual to mock in testing.
   virtual void LaunchChildGTestProcess(
-      scoped_refptr<TaskRunner> task_runner,
-      const std::vector<std::string>& test_names,
-      const FilePath& temp_dir);
+      const CommandLine& command_line,
+      const std::string& wrapper,
+      TimeDelta timeout,
+      const LaunchOptions& options,
+      std::unique_ptr<ProcessLifetimeObserver> observer);
 
   // Called when a test has finished running.
   void OnTestFinished(const TestResult& result);

base/test/launcher/test_launcher_nacl_nonsfi.cc

@@ -80,19 +80,14 @@ class NonSfiUnitTestPlatformDelegate : public base::UnitTestPlatformDelegate {
   }
 
  private:
-  bool CreateResultsFile(const base::FilePath& temp_dir,
-                         base::FilePath* path) override {
-    if (!base::CreateTemporaryDirInDir(temp_dir, base::FilePath::StringType(),
-                                       path))
+  bool CreateResultsFile(base::FilePath* path) override {
+    if (!base::CreateNewTempDirectory(base::FilePath::StringType(), path))
       return false;
     *path = path->AppendASCII("test_results.xml");
     return true;
   }
 
-  bool CreateTemporaryFile(const base::FilePath& temp_dir,
-                           base::FilePath* path) override {
-    return false;
-  }
+  bool CreateTemporaryFile(base::FilePath* path) override { return false; }
 
   bool GetTests(std::vector<base::TestIdentifier>* output) override {
     base::FilePath output_file;

base/test/launcher/unit_test_launcher.h

@@ -60,13 +60,11 @@ class UnitTestPlatformDelegate {
 
   // Called to create a temporary for storing test results. The delegate
   // must put the resulting path in |path| and return true on success.
-  virtual bool CreateResultsFile(const base::FilePath& temp_dir,
-                                 base::FilePath* path) = 0;
+  virtual bool CreateResultsFile(base::FilePath* path) = 0;
 
   // Called to create a new temporary file. The delegate must put the resulting
   // path in |path| and return true on success.
-  virtual bool CreateTemporaryFile(const base::FilePath& temp_dir,
-                                   base::FilePath* path) = 0;
+  virtual bool CreateTemporaryFile(base::FilePath* path) = 0;
 
   // Returns command line for child GTest process based on the command line
   // of current process. |test_names| is a vector of test full names
@@ -97,11 +95,9 @@ class DefaultUnitTestPlatformDelegate : public UnitTestPlatformDelegate {
 
   bool GetTests(std::vector<TestIdentifier>* output) override;
 
-  bool CreateResultsFile(const base::FilePath& temp_dir,
-                         base::FilePath* path) override;
+  bool CreateResultsFile(base::FilePath* path) override;
 
-  bool CreateTemporaryFile(const base::FilePath& temp_dir,
-                           base::FilePath* path) override;
+  bool CreateTemporaryFile(base::FilePath* path) override;
 
   CommandLine GetCommandLineForChildGTestProcess(
       const std::vector<std::string>& test_names,
@@ -115,6 +111,18 @@ class DefaultUnitTestPlatformDelegate : public UnitTestPlatformDelegate {
   DISALLOW_COPY_AND_ASSIGN(DefaultUnitTestPlatformDelegate);
 };
 
+// Runs tests serially, each in its own process.
+void RunUnitTestsSerially(TestLauncher* test_launcher,
+                          UnitTestPlatformDelegate* platform_delegate,
+                          const std::vector<std::string>& test_names,
+                          int launch_flags);
+
+// Runs tests in batches (each batch in its own process).
+void RunUnitTestsBatch(TestLauncher* test_launcher,
+                       UnitTestPlatformDelegate* platform_delegate,
+                       const std::vector<std::string>& test_names,
+                       int launch_flags);
+
 // Test launcher delegate for unit tests (mostly to support batching).
 class UnitTestLauncherDelegate : public TestLauncherDelegate {
  public:
@@ -128,26 +136,10 @@ class UnitTestLauncherDelegate : public TestLauncherDelegate {
   bool GetTests(std::vector<TestIdentifier>* output) override;
   bool WillRunTest(const std::string& test_case_name,
                    const std::string& test_name) override;
-
-  std::vector<TestResult> ProcessTestResults(
-      const std::vector<std::string>& test_names,
-      const base::FilePath& output_file,
-      const std::string& output,
-      const base::TimeDelta& elapsed_time,
-      int exit_code,
-      bool was_timeout) override;
-
-  CommandLine GetCommandLine(const std::vector<std::string>& test_names,
-                             const FilePath& temp_dir,
-                             FilePath* output_file) override;
-
-  std::string GetWrapper() override;
-
-  int GetLaunchOptions() override;
-
-  TimeDelta GetTimeout() override;
-
-  size_t GetBatchSize() override;
+  size_t RunTests(TestLauncher* test_launcher,
+                  const std::vector<std::string>& test_names) override;
+  size_t RetryTests(TestLauncher* test_launcher,
+                    const std::vector<std::string>& test_names) override;
 
   ThreadChecker thread_checker_;
 

base/test/launcher/unit_test_launcher_unittest.cc

@@ -19,6 +19,64 @@
 
 namespace base {
 namespace {
+
+// Unit tests to validate DefaultUnitTestPlatformDelegate implementations.
+class DefaultUnitTestPlatformDelegateTester : public testing::Test {
+ protected:
+  UnitTestPlatformDelegate* platformDelegate;
+  FilePath flag_path;
+  FilePath output_path;
+  std::vector<std::string> test_names;
+
+  void SetUp() override { platformDelegate = &defaultPlatform_; }
+
+ private:
+  DefaultUnitTestPlatformDelegate defaultPlatform_;
+};
+
+// Call fails when flag_file does not exist.
+TEST_F(DefaultUnitTestPlatformDelegateTester, FlagPathCheckFail) {
+  ASSERT_CHECK_DEATH(platformDelegate->GetCommandLineForChildGTestProcess(
+      test_names, output_path, flag_path));
+}
+
+// Validate flags are set correctly in by the delegate.
+TEST_F(DefaultUnitTestPlatformDelegateTester,
+       GetCommandLineForChildGTestProcess) {
+  ASSERT_TRUE(platformDelegate->CreateResultsFile(&output_path));
+  ASSERT_TRUE(platformDelegate->CreateTemporaryFile(&flag_path));
+  CommandLine cmd_line(platformDelegate->GetCommandLineForChildGTestProcess(
+      test_names, output_path, flag_path));
+  EXPECT_EQ(cmd_line.GetSwitchValueASCII("test-launcher-output"),
+            output_path.MaybeAsASCII());
+  EXPECT_EQ(cmd_line.GetSwitchValueASCII("gtest_flagfile"),
+            flag_path.MaybeAsASCII());
+  EXPECT_TRUE(cmd_line.HasSwitch("single-process-tests"));
+}
+
+// Validate the tests are saved correctly in flag file under
+// the "--gtest_filter" flag.
+TEST_F(DefaultUnitTestPlatformDelegateTester, GetCommandLineFilterTest) {
+  test_names.push_back("Test1");
+  test_names.push_back("Test2");
+  ASSERT_TRUE(platformDelegate->CreateResultsFile(&output_path));
+  ASSERT_TRUE(platformDelegate->CreateTemporaryFile(&flag_path));
+  CommandLine cmd_line(platformDelegate->GetCommandLineForChildGTestProcess(
+      test_names, output_path, flag_path));
+
+  const int size = 2048;
+  std::string content;
+  ASSERT_TRUE(ReadFileToStringWithMaxSize(flag_path, &content, size));
+  EXPECT_EQ(content.find("--gtest_filter="), 0u);
+  base::ReplaceSubstringsAfterOffset(&content, 0, "--gtest_filter=", "");
+  std::vector<std::string> gtest_filter_tests =
+      SplitString(content, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+  ASSERT_EQ(gtest_filter_tests.size(), test_names.size());
+  for (unsigned i = 0; i < test_names.size(); i++) {
+    EXPECT_EQ(gtest_filter_tests.at(i), test_names.at(i));
+  }
+}
+
 // Mock TestLauncher to validate LaunchChildGTestProcess
 // is called correctly inside the test launcher delegate.
 class MockTestLauncher : public TestLauncher {
@@ -27,52 +85,81 @@ class MockTestLauncher : public TestLauncher {
                    size_t parallel_jobs)
       : TestLauncher(launcher_delegate, parallel_jobs) {}
 
-  MOCK_METHOD3(LaunchChildGTestProcess,
-               void(scoped_refptr<TaskRunner> task_runner,
-                    const std::vector<std::string>& test_names,
-                    const FilePath& temp_dir));
+  MOCK_METHOD5(LaunchChildGTestProcess,
+               void(const CommandLine& command_line,
+                    const std::string& wrapper,
+                    TimeDelta timeout,
+                    const LaunchOptions& options,
+                    std::unique_ptr<ProcessLifetimeObserver> observer));
 };
 
 // Unit tests to validate UnitTestLauncherDelegateTester implementations.
 class UnitTestLauncherDelegateTester : public testing::Test {
  protected:
+  TestLauncherDelegate* launcherDelegate;
+  MockTestLauncher* launcher;
+  std::vector<std::string> tests;
+
+  void SetUp() override { tests.assign(100, "Test"); }
+
+  // Setup test launcher delegate with a particular batch size.
+  void SetUpLauncherDelegate(size_t batch_size) {
+    launcherDelegate =
+        new UnitTestLauncherDelegate(&defaultPlatform, batch_size, true);
+    launcher = new MockTestLauncher(launcherDelegate, batch_size);
+  }
+
+  // Validate LaunchChildGTestProcess is called x number of times.
+  void ValidateChildGTestProcessCalls(int times_called) {
+    using ::testing::_;
+    EXPECT_CALL(*launcher, LaunchChildGTestProcess(_, _, _, _, _))
+        .Times(times_called);
+  }
+
+  void TearDown() override {
+    delete launcherDelegate;
+    delete launcher;
+  }
+
+ private:
   DefaultUnitTestPlatformDelegate defaultPlatform;
 };
 
-// Validate delegate produces correct command line.
-TEST_F(UnitTestLauncherDelegateTester, GetCommandLine) {
-  UnitTestLauncherDelegate launcher_delegate(&defaultPlatform, 10u, true);
-  TestLauncherDelegate* delegate_ptr = &launcher_delegate;
+// Validate 0 batch size corresponds to unlimited batch size.
+TEST_F(UnitTestLauncherDelegateTester, RunTestsWithUnlimitedBatchSize) {
+  SetUpLauncherDelegate(0);
 
-  std::vector<std::string> test_names(5, "Tests");
-  base::FilePath temp_dir;
-  base::FilePath result_file;
-  CreateNewTempDirectory(FilePath::StringType(), &temp_dir);
+  ValidateChildGTestProcessCalls(1);
+  EXPECT_EQ(launcherDelegate->RunTests(launcher, tests), tests.size());
+}
 
-  CommandLine cmd_line =
-      delegate_ptr->GetCommandLine(test_names, temp_dir, &result_file);
-  EXPECT_TRUE(cmd_line.HasSwitch("single-process-tests"));
-  EXPECT_EQ(cmd_line.GetSwitchValuePath("test-launcher-output"), result_file);
+// Validate edge case, no tests to run.
+TEST_F(UnitTestLauncherDelegateTester, RunTestsWithEmptyTests) {
+  SetUpLauncherDelegate(0);
 
-  const int size = 2048;
-  std::string content;
-  ASSERT_TRUE(ReadFileToStringWithMaxSize(
-      cmd_line.GetSwitchValuePath("gtest_flagfile"), &content, size));
-  EXPECT_EQ(content.find("--gtest_filter="), 0u);
-  base::ReplaceSubstringsAfterOffset(&content, 0, "--gtest_filter=", "");
-  std::vector<std::string> gtest_filter_tests =
-      SplitString(content, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
-  ASSERT_EQ(gtest_filter_tests.size(), test_names.size());
-  for (unsigned i = 0; i < test_names.size(); i++) {
-    EXPECT_EQ(gtest_filter_tests.at(i), test_names.at(i));
-  }
+  ValidateChildGTestProcessCalls(0);
+  tests.clear();
+  EXPECT_EQ(launcherDelegate->RunTests(launcher, tests), tests.size());
+}
+
+// Validate delegate slices batch size correctly.
+TEST_F(UnitTestLauncherDelegateTester, RunTestsBatchSize10) {
+  SetUpLauncherDelegate(10);
+
+  ValidateChildGTestProcessCalls(10);
+  EXPECT_EQ(launcherDelegate->RunTests(launcher, tests), tests.size());
 }
 
-// Validate delegate sets batch size correctly.
-TEST_F(UnitTestLauncherDelegateTester, BatchSize) {
-  UnitTestLauncherDelegate launcher_delegate(&defaultPlatform, 15u, true);
-  TestLauncherDelegate* delegate_ptr = &launcher_delegate;
-  EXPECT_EQ(delegate_ptr->GetBatchSize(), 15u);
+// ValidateRetryTests will only kick-off one run.
+TEST_F(UnitTestLauncherDelegateTester, RetryTests) {
+  // ScopedTaskEviorment is needed since RetryTests uses thread task
+  // runner to start.
+  test::ScopedTaskEnvironment task_environment;
+  SetUpLauncherDelegate(10);
+
+  ValidateChildGTestProcessCalls(1);
+  EXPECT_EQ(launcherDelegate->RetryTests(launcher, tests), tests.size());
+  RunLoop().RunUntilIdle();
 }
 
 }  // namespace