Remove tmp files used to pass command line to tests after they finish

Right now the file used to pass command line parameters to test-
processes is not removed after the test finished. In cases where tests
are run in repeat mode those flag files just keep accumulating until
the system runs out of space in it's $TMPDIR.

To solve this, the patch makes sure the path to the file containing the
flags is stored in UnitTestProcessLifetimeObserver and removed once
the OnCompleted method is called.

Bug: 814687
Change-Id: Ifb0ba6dba860d9297fbc2c939e0cc10f620907fa
Reviewed-on: https://chromium-review.googlesource.com/931341
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539104}

base/test/launcher/test_launcher_nacl_nonsfi.cc

@@ -80,13 +80,15 @@ class NonSfiUnitTestPlatformDelegate : public base::UnitTestPlatformDelegate {
   }
 
  private:
-  bool CreateTemporaryFile(base::FilePath* path) override {
+  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(base::FilePath* path) override { return false; }
+
   bool GetTests(std::vector<base::TestIdentifier>* output) override {
     base::FilePath output_file;
     if (!base::CreateTemporaryFile(&output_file)) {
@@ -112,7 +114,8 @@ class NonSfiUnitTestPlatformDelegate : public base::UnitTestPlatformDelegate {
 
   base::CommandLine GetCommandLineForChildGTestProcess(
       const std::vector<std::string>& test_names,
-      const base::FilePath& output_file) override {
+      const base::FilePath& output_file,
+      const base::FilePath& flag_file) override {
     base::CommandLine cmd_line(test_path_);
     cmd_line.AppendSwitchPath(
         switches::kTestLauncherOutput, output_file);

base/test/launcher/unit_test_launcher.cc

@@ -121,31 +121,36 @@ class DefaultUnitTestPlatformDelegate : public UnitTestPlatformDelegate {
     return true;
   }
 
-  bool CreateTemporaryFile(base::FilePath* path) override {
+  bool CreateResultsFile(base::FilePath* path) override {
     if (!CreateNewTempDirectory(FilePath::StringType(), path))
       return false;
     *path = path->AppendASCII("test_results.xml");
     return true;
   }
 
+  bool CreateTemporaryFile(base::FilePath* path) override {
+    if (!temp_dir_.IsValid() && !temp_dir_.CreateUniqueTempDir())
+      return false;
+    return CreateTemporaryFileInDir(temp_dir_.GetPath(), path);
+  }
+
   CommandLine GetCommandLineForChildGTestProcess(
       const std::vector<std::string>& test_names,
-      const base::FilePath& output_file) override {
+      const base::FilePath& output_file,
+      const base::FilePath& flag_file) override {
     CommandLine new_cmd_line(*CommandLine::ForCurrentProcess());
 
-    CHECK(temp_dir_.IsValid() || temp_dir_.CreateUniqueTempDir());
-    FilePath temp_file;
-    CHECK(CreateTemporaryFileInDir(temp_dir_.GetPath(), &temp_file));
+    CHECK(base::PathExists(flag_file));
+
     std::string long_flags(
         std::string("--") + kGTestFilterFlag + "=" +
         JoinString(test_names, ":"));
     CHECK_EQ(static_cast<int>(long_flags.size()),
-             WriteFile(temp_file,
-                       long_flags.data(),
+             WriteFile(flag_file, long_flags.data(),
                        static_cast<int>(long_flags.size())));
 
     new_cmd_line.AppendSwitchPath(switches::kTestLauncherOutput, output_file);
-    new_cmd_line.AppendSwitchPath(kGTestFlagfileFlag, temp_file);
+    new_cmd_line.AppendSwitchPath(kGTestFlagfileFlag, flag_file);
     new_cmd_line.AppendSwitch(kSingleProcessTestsFlag);
 
     return new_cmd_line;
@@ -417,19 +422,22 @@ class UnitTestProcessLifetimeObserver : public ProcessLifetimeObserver {
   const std::vector<std::string>& test_names() { return test_names_; }
   int launch_flags() { return launch_flags_; }
   const FilePath& output_file() { return output_file_; }
+  const FilePath& flag_file() { return flag_file_; }
 
  protected:
   UnitTestProcessLifetimeObserver(TestLauncher* test_launcher,
                                   UnitTestPlatformDelegate* platform_delegate,
                                   const std::vector<std::string>& test_names,
                                   int launch_flags,
-                                  const FilePath& output_file)
+                                  const FilePath& output_file,
+                                  const FilePath& flag_file)
       : ProcessLifetimeObserver(),
         test_launcher_(test_launcher),
         platform_delegate_(platform_delegate),
         test_names_(test_names),
         launch_flags_(launch_flags),
-        output_file_(output_file) {}
+        output_file_(output_file),
+        flag_file_(flag_file) {}
 
   SEQUENCE_CHECKER(sequence_checker_);
 
@@ -439,6 +447,7 @@ class UnitTestProcessLifetimeObserver : public ProcessLifetimeObserver {
   const std::vector<std::string> test_names_;
   const int launch_flags_;
   const FilePath output_file_;
+  const FilePath flag_file_;
 
   DISALLOW_COPY_AND_ASSIGN(UnitTestProcessLifetimeObserver);
 };
@@ -451,12 +460,14 @@ class ParallelUnitTestProcessLifetimeObserver
       UnitTestPlatformDelegate* platform_delegate,
       const std::vector<std::string>& test_names,
       int launch_flags,
-      const FilePath& output_file)
+      const FilePath& output_file,
+      const FilePath& flag_file)
       : UnitTestProcessLifetimeObserver(test_launcher,
                                         platform_delegate,
                                         test_names,
                                         launch_flags,
-                                        output_file) {}
+                                        output_file,
+                                        flag_file) {}
   ~ParallelUnitTestProcessLifetimeObserver() override = default;
 
  private:
@@ -486,6 +497,8 @@ void ParallelUnitTestProcessLifetimeObserver::OnCompleted(
 
   // The temporary file's directory is also temporary.
   DeleteFile(output_file().DirName(), true);
+  if (!flag_file().empty())
+    DeleteFile(flag_file(), false);
 }
 
 class SerialUnitTestProcessLifetimeObserver
@@ -497,12 +510,14 @@ class SerialUnitTestProcessLifetimeObserver
       const std::vector<std::string>& test_names,
       int launch_flags,
       const FilePath& output_file,
+      const FilePath& flag_file,
       std::vector<std::string>&& next_test_names)
       : UnitTestProcessLifetimeObserver(test_launcher,
                                         platform_delegate,
                                         test_names,
                                         launch_flags,
-                                        output_file),
+                                        output_file,
+                                        flag_file),
         next_test_names_(std::move(next_test_names)) {}
   ~SerialUnitTestProcessLifetimeObserver() override = default;
 
@@ -539,6 +554,9 @@ void SerialUnitTestProcessLifetimeObserver::OnCompleted(
   // The temporary file's directory is also temporary.
   DeleteFile(output_file().DirName(), true);
 
+  if (!flag_file().empty())
+    DeleteFile(flag_file(), false);
+
   ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       BindOnce(&RunUnitTestsSerially, test_launcher(), platform_delegate(),
@@ -609,15 +627,18 @@ void RunUnitTestsSerially(
   // per run to ensure clean state and make it possible to launch multiple
   // processes in parallel.
   FilePath output_file;
-  CHECK(platform_delegate->CreateTemporaryFile(&output_file));
+  CHECK(platform_delegate->CreateResultsFile(&output_file));
+  FilePath flag_file;
+  platform_delegate->CreateTemporaryFile(&flag_file);
 
   auto observer = std::make_unique<SerialUnitTestProcessLifetimeObserver>(
       test_launcher, platform_delegate,
       std::vector<std::string>(1, test_names.back()), launch_flags, output_file,
+      flag_file,
       std::vector<std::string>(test_names.begin(), test_names.end() - 1));
 
   CommandLine cmd_line(platform_delegate->GetCommandLineForChildGTestProcess(
-      observer->test_names(), output_file));
+      observer->test_names(), output_file, flag_file));
 
   TestLauncher::LaunchOptions launch_options;
   launch_options.flags = launch_flags;
@@ -639,13 +660,16 @@ void RunUnitTestsBatch(
   // per run to ensure clean state and make it possible to launch multiple
   // processes in parallel.
   FilePath output_file;
-  CHECK(platform_delegate->CreateTemporaryFile(&output_file));
+  CHECK(platform_delegate->CreateResultsFile(&output_file));
+  FilePath flag_file;
+  platform_delegate->CreateTemporaryFile(&flag_file);
 
   auto observer = std::make_unique<ParallelUnitTestProcessLifetimeObserver>(
-      test_launcher, platform_delegate, test_names, launch_flags, output_file);
+      test_launcher, platform_delegate, test_names, launch_flags, output_file,
+      flag_file);
 
   CommandLine cmd_line(platform_delegate->GetCommandLineForChildGTestProcess(
-      test_names, output_file));
+      test_names, output_file, flag_file));
 
   // Adjust the timeout depending on how many tests we're running
   // (note that e.g. the last batch of tests will be smaller).

base/test/launcher/unit_test_launcher.h

@@ -59,7 +59,11 @@ class UnitTestPlatformDelegate {
   // must put the result in |output| and return true on success.
   virtual bool GetTests(std::vector<TestIdentifier>* output) = 0;
 
-  // Called to create a temporary file. The delegate must put the resulting
+  // 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;
+
+  // 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;
 
@@ -68,7 +72,8 @@ class UnitTestPlatformDelegate {
   // (e.g. "A.B"), |output_file| is path to the GTest XML output file.
   virtual CommandLine GetCommandLineForChildGTestProcess(
       const std::vector<std::string>& test_names,
-      const base::FilePath& output_file) = 0;
+      const base::FilePath& output_file,
+      const base::FilePath& flag_file) = 0;
 
   // Returns wrapper to use for child GTest process. Empty string means
   // no wrapper.