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

The original change has been reverted since result processing
was not thread safe.

Bug: 936248,848465
Change-Id: I2468cf2e92901c7f2c4a6f42838be219397a7b0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1721429
Commit-Queue: Ilia Samsonov <isamsonov@google.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682916}

base/test/launcher/test_launcher.h

@@ -13,6 +13,7 @@
 #include <string>
 #include <vector>
 
+#include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/process/launch.h"
@@ -26,10 +27,6 @@
 
 namespace base {
 
-class CommandLine;
-struct LaunchOptions;
-class TestLauncher;
-
 // Constants for GTest command-line flags.
 extern const char kGTestFilterFlag[];
 extern const char kGTestFlagfileFlag[];
@@ -57,53 +54,47 @@ class TestLauncherDelegate {
   virtual bool WillRunTest(const std::string& test_case_name,
                            const std::string& test_name) = 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 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 main thread.
+  // 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;
 
   // 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 OnTimedOut(const CommandLine& command_line) {}
+  virtual void OnTestTimedOut(const base::CommandLine& cmd_line) {}
 
-  // 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) {}
+  // 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;
 
- protected:
-  ProcessLifetimeObserver() = default;
+  // Returns the delegate specific flags for launch options.
+  // The flags are specified in LaunchChildGTestProcessFlags.
+  virtual int GetLaunchOptions() = 0;
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(ProcessLifetimeObserver);
+  // Returns the delegate specific timeout per test.
+  virtual TimeDelta GetTimeout() = 0;
+
+  // Returns the delegate specific batch size.
+  virtual size_t GetBatchSize() = 0;
+
+ protected:
+  virtual ~TestLauncherDelegate();
 };
 
 // Launches tests using a TestLauncherDelegate.
@@ -151,18 +142,16 @@ 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) 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.
+  // 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.
   // virtual to mock in testing.
   virtual void LaunchChildGTestProcess(
-      const CommandLine& command_line,
-      const std::string& wrapper,
-      TimeDelta timeout,
-      const LaunchOptions& options,
-      std::unique_ptr<ProcessLifetimeObserver> observer);
+      scoped_refptr<TaskRunner> task_runner,
+      const std::vector<std::string>& test_names,
+      const FilePath& temp_dir);
 
   // Called when a test has finished running.
   void OnTestFinished(const TestResult& result);
@@ -220,6 +209,13 @@ class TestLauncher {
   // wait for child processes). virtual to mock in testing.
   virtual void CreateAndStartThreadPool(int num_parallel_jobs);
 
+  void ProcessTestResults(const std::vector<std::string>& test_names,
+                          const base::FilePath& result_file,
+                          const std::string& output,
+                          const base::TimeDelta& elapsed_time,
+                          int exit_code,
+                          bool was_timeout);
+
   // Make sure we don't accidentally call the wrong methods e.g. on the worker
   // pool thread.  Should be the first member so that it's destroyed last: when
   // destroying other members, especially the worker pool, we may check the code

base/test/launcher/test_launcher_nacl_nonsfi.cc

@@ -80,14 +80,19 @@ class NonSfiUnitTestPlatformDelegate : public base::UnitTestPlatformDelegate {
   }
 
  private:
-  bool CreateResultsFile(base::FilePath* path) override {
-    if (!base::CreateNewTempDirectory(base::FilePath::StringType(), path))
+  bool CreateResultsFile(const base::FilePath& temp_dir,
+                         base::FilePath* path) override {
+    if (!base::CreateTemporaryDirInDir(temp_dir, base::FilePath::StringType(),
+                                       path))
       return false;
     *path = path->AppendASCII("test_results.xml");
     return true;
   }
 
-  bool CreateTemporaryFile(base::FilePath* path) override { return false; }
+  bool CreateTemporaryFile(const base::FilePath& temp_dir,
+                           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,11 +60,13 @@ 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(base::FilePath* path) = 0;
+  virtual bool CreateResultsFile(const base::FilePath& temp_dir,
+                                 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(base::FilePath* path) = 0;
+  virtual bool CreateTemporaryFile(const base::FilePath& temp_dir,
+                                   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
@@ -95,9 +97,11 @@ class DefaultUnitTestPlatformDelegate : public UnitTestPlatformDelegate {
 
   bool GetTests(std::vector<TestIdentifier>* output) override;
 
-  bool CreateResultsFile(base::FilePath* path) override;
+  bool CreateResultsFile(const base::FilePath& temp_dir,
+                         base::FilePath* path) override;
 
-  bool CreateTemporaryFile(base::FilePath* path) override;
+  bool CreateTemporaryFile(const base::FilePath& temp_dir,
+                           base::FilePath* path) override;
 
   CommandLine GetCommandLineForChildGTestProcess(
       const std::vector<std::string>& test_names,
@@ -111,18 +115,6 @@ 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:
@@ -136,10 +128,26 @@ 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;
-  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;
+
+  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;
 
   ThreadChecker thread_checker_;
 

base/test/launcher/unit_test_launcher_unittest.cc

@@ -19,64 +19,6 @@
 
 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 {
@@ -85,81 +27,52 @@ class MockTestLauncher : public TestLauncher {
                    size_t parallel_jobs)
       : TestLauncher(launcher_delegate, parallel_jobs) {}
 
-  MOCK_METHOD5(LaunchChildGTestProcess,
-               void(const CommandLine& command_line,
-                    const std::string& wrapper,
-                    TimeDelta timeout,
-                    const LaunchOptions& options,
-                    std::unique_ptr<ProcessLifetimeObserver> observer));
+  MOCK_METHOD3(LaunchChildGTestProcess,
+               void(scoped_refptr<TaskRunner> task_runner,
+                    const std::vector<std::string>& test_names,
+                    const FilePath& temp_dir));
 };
 
 // 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 0 batch size corresponds to unlimited batch size.
-TEST_F(UnitTestLauncherDelegateTester, RunTestsWithUnlimitedBatchSize) {
-  SetUpLauncherDelegate(0);
-
-  ValidateChildGTestProcessCalls(1);
-  EXPECT_EQ(launcherDelegate->RunTests(launcher, tests), tests.size());
-}
-
-// Validate edge case, no tests to run.
-TEST_F(UnitTestLauncherDelegateTester, RunTestsWithEmptyTests) {
-  SetUpLauncherDelegate(0);
+// Validate delegate produces correct command line.
+TEST_F(UnitTestLauncherDelegateTester, GetCommandLine) {
+  UnitTestLauncherDelegate launcher_delegate(&defaultPlatform, 10u, true);
+  TestLauncherDelegate* delegate_ptr = &launcher_delegate;
 
-  ValidateChildGTestProcessCalls(0);
-  tests.clear();
-  EXPECT_EQ(launcherDelegate->RunTests(launcher, tests), tests.size());
-}
+  std::vector<std::string> test_names(5, "Tests");
+  base::FilePath temp_dir;
+  base::FilePath result_file;
+  CreateNewTempDirectory(FilePath::StringType(), &temp_dir);
 
-// Validate delegate slices batch size correctly.
-TEST_F(UnitTestLauncherDelegateTester, RunTestsBatchSize10) {
-  SetUpLauncherDelegate(10);
+  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);
 
-  ValidateChildGTestProcessCalls(10);
-  EXPECT_EQ(launcherDelegate->RunTests(launcher, tests), tests.size());
+  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));
+  }
 }
 
-// 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();
+// 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);
 }
 
 }  // namespace