GTTF: Fix handling of PRE_ tests

- skip dependent tests if a PRE_ test failed
- simplify ParallelTestLauncher code now that we don't need sequencing

BUG=236893
R=jam@chromium.org

Review URL: https://codereview.chromium.org/24892002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@225741 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 74 insertions, 94 deletions

diff --git a/base/test/parallel_test_launcher.cc b/base/test/parallel_test_launcher.cc
index 0883946..34cf995 100644
--- a/base/test/parallel_test_launcher.cc
+++ b/base/test/parallel_test_launcher.cc
@@ -154,43 +154,6 @@ void ParallelTestLauncher::LaunchChildGTestProcess(
     const LaunchChildGTestProcessCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  LaunchSequencedChildGTestProcess(
-      worker_pool_owner_->pool()->GetSequenceToken(),
-      command_line,
-      wrapper,
-      timeout,
-      callback);
-}
-
-void ParallelTestLauncher::LaunchNamedSequencedChildGTestProcess(
-    const std::string& token_name,
-    const CommandLine& command_line,
-    const std::string& wrapper,
-    base::TimeDelta timeout,
-    const LaunchChildGTestProcessCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  LaunchSequencedChildGTestProcess(
-      worker_pool_owner_->pool()->GetNamedSequenceToken(token_name),
-      command_line,
-      wrapper,
-      timeout,
-      callback);
-}
-
-void ParallelTestLauncher::ResetOutputWatchdog() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  timer_.Reset();
-}
-
-void ParallelTestLauncher::LaunchSequencedChildGTestProcess(
-    SequencedWorkerPool::SequenceToken sequence_token,
-    const CommandLine& command_line,
-    const std::string& wrapper,
-    base::TimeDelta timeout,
-    const LaunchChildGTestProcessCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   // Record the exact command line used to launch the child.
   CommandLine new_command_line(
       PrepareCommandLineForGTest(command_line, wrapper));
@@ -198,8 +161,7 @@ void ParallelTestLauncher::LaunchSequencedChildGTestProcess(
   running_processes_map_.insert(
       std::make_pair(launch_sequence_number_, new_command_line));
 
-  worker_pool_owner_->pool()->PostSequencedWorkerTask(
-      sequence_token,
+  worker_pool_owner_->pool()->PostWorkerTask(
       FROM_HERE,
       Bind(&DoLaunchChildTestProcess,
            new_command_line,
@@ -211,6 +173,11 @@ void ParallelTestLauncher::LaunchSequencedChildGTestProcess(
                 callback)));
 }
 
+void ParallelTestLauncher::ResetOutputWatchdog() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  timer_.Reset();
+}
+
 void ParallelTestLauncher::OnLaunchTestProcessFinished(
     size_t sequence_number,
     const LaunchChildGTestProcessCallback& callback,
diff --git a/base/test/parallel_test_launcher.h b/base/test/parallel_test_launcher.h
index 6d9d228..af73c01 100644
--- a/base/test/parallel_test_launcher.h
+++ b/base/test/parallel_test_launcher.h
@@ -9,7 +9,6 @@
 #include <string>
 
 #include "base/callback.h"
-#include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
@@ -19,7 +18,6 @@ class CommandLine;
 namespace base {
 
 class SequencedWorkerPoolOwner;
-class Timer;
 
 // Launches child gtest process in parallel. Keeps track of running processes,
 // prints a message in case no output is produced for a while.
@@ -46,28 +44,12 @@ class ParallelTestLauncher {
                                base::TimeDelta timeout,
                                const LaunchChildGTestProcessCallback& callback);
 
-  // Similar to above, but with processes sharing the same value of |token_name|
-  // being serialized, with order matching order of calls of this method.
-  void LaunchNamedSequencedChildGTestProcess(
-      const std::string& token_name,
-      const CommandLine& command_line,
-      const std::string& wrapper,
-      base::TimeDelta timeout,
-      const LaunchChildGTestProcessCallback& callback);
-
   // Resets the output watchdog, indicating some test results have been printed
   // out. If a pause between the calls exceeds an internal treshold, a message
   // will be printed listing all child processes we're still waiting for.
   void ResetOutputWatchdog();
 
  private:
-  void LaunchSequencedChildGTestProcess(
-      SequencedWorkerPool::SequenceToken sequence_token,
-      const CommandLine& command_line,
-      const std::string& wrapper,
-      base::TimeDelta timeout,
-      const LaunchChildGTestProcessCallback& callback);
-
   // Called on a worker thread after a child process finishes.
   void OnLaunchTestProcessFinished(
       size_t sequence_number,
diff --git a/base/test/test_launcher.cc b/base/test/test_launcher.cc
index 23fe904..462f4d4 100644
--- a/base/test/test_launcher.cc
+++ b/base/test/test_launcher.cc
@@ -351,6 +351,8 @@ void ResultsPrinter::AddTestResult(const TestResult& result) {
     status_line.append("(TIMED OUT)");
   } else if (result.status == TestResult::TEST_CRASH) {
     status_line.append("(CRASHED)");
+  } else if (result.status == TestResult::TEST_SKIPPED) {
+    status_line.append("(SKIPPED)");
   } else if (result.status == TestResult::TEST_UNKNOWN) {
     status_line.append("(UNKNOWN)");
   } else {
@@ -371,6 +373,7 @@ void ResultsPrinter::AddTestResult(const TestResult& result) {
     PrintTestsByStatus(TestResult::TEST_FAILURE, "failed");
     PrintTestsByStatus(TestResult::TEST_TIMEOUT, "timed out");
     PrintTestsByStatus(TestResult::TEST_CRASH, "crashed");
+    PrintTestsByStatus(TestResult::TEST_SKIPPED, "skipped");
     PrintTestsByStatus(TestResult::TEST_UNKNOWN, "had unknown result");
 
     callback_.Run(
diff --git a/base/test/test_launcher.h b/base/test/test_launcher.h
index fac7297..0e8faea 100644
--- a/base/test/test_launcher.h
+++ b/base/test/test_launcher.h
@@ -39,6 +39,7 @@ struct TestResult {
     TEST_FAILURE,  // Assertion failure (think EXPECT_TRUE, not DCHECK).
     TEST_TIMEOUT,  // Test timed out and was killed.
     TEST_CRASH,    // Test crashed (includes CHECK/DCHECK failures).
+    TEST_SKIPPED,  // Test skipped (not run at all).
   };
 
   TestResult();
diff --git a/content/public/test/test_launcher.cc b/content/public/test/test_launcher.cc
index 5964d55f..1237451 100644
--- a/content/public/test/test_launcher.cc
+++ b/content/public/test/test_launcher.cc
@@ -4,6 +4,7 @@
 
 #include "content/public/test/test_launcher.h"
 
+#include <map>
 #include <string>
 #include <vector>
 
@@ -109,16 +110,22 @@ class WrapperTestLauncherDelegate : public base::TestLauncherDelegate {
 
  private:
   struct TestInfo {
+    std::string GetFullName() const { return test_case_name + "." + test_name; }
+
     std::string test_case_name;
     std::string test_name;
-    base::TestLauncherDelegate::TestResultCallback callback;
+    std::vector<base::TestLauncherDelegate::TestResultCallback> callbacks;
   };
 
-  friend bool CompareTestInfo(const TestInfo& a, const TestInfo& b);
-
   // Launches test from |test_info| using |command_line| and parallel launcher.
   void DoRunTest(const TestInfo& test_info, const CommandLine& command_line);
 
+  // Launches test named |test_name| using |command_line| and parallel launcher,
+  // given result of PRE_ test |pre_test_result|.
+  void RunDependentTest(const std::string test_name,
+                        const CommandLine& command_line,
+                        const base::TestResult& pre_test_result);
+
   // Callback to receive result of a test.
   void GTestCallback(
       const TestInfo& test_info,
@@ -139,8 +146,9 @@ class WrapperTestLauncherDelegate : public base::TestLauncherDelegate {
   base::ParallelTestLauncher parallel_launcher_;
 
   // Store all tests to run before running any of them to properly
-  // handle PRE_ tests.
-  std::vector<TestInfo> tests_to_run_;
+  // handle PRE_ tests. The map is indexed by test full name (e.g. "A.B").
+  typedef std::map<std::string, TestInfo> TestInfoMap;
+  TestInfoMap tests_to_run_;
 
   // Temporary directory for user data directories.
   base::ScopedTempDir temp_dir_;
@@ -182,34 +190,13 @@ void WrapperTestLauncherDelegate::RunTest(
   TestInfo run_test_info;
   run_test_info.test_case_name = test_case->name();
   run_test_info.test_name = test_info->name();
-  run_test_info.callback = callback;
-  tests_to_run_.push_back(run_test_info);
-}
+  run_test_info.callbacks.push_back(callback);
 
-bool CompareTestInfo(const WrapperTestLauncherDelegate::TestInfo& a,
-                     const WrapperTestLauncherDelegate::TestInfo& b) {
-  if (a.test_case_name == b.test_case_name) {
-    // Put PRE_ tests before tests that depend on them (e.g. PRE_Foo before Foo,
-    // and PRE_PRE_Foo before PRE_Foo).
-    if (std::string(kPreTestPrefix) + a.test_name == b.test_name)
-      return false;
-    if (a.test_name == std::string(kPreTestPrefix) + b.test_name)
-      return true;
-  }
-
-  // Otherwise sort by full names, disregarding PRE_ completely so that
-  // this can still be Strict Weak Ordering.
-  std::string a_full(
-      RemoveAnyPrePrefixes(a.test_case_name + "." + a.test_name));
-  std::string b_full(
-      RemoveAnyPrePrefixes(b.test_case_name + "." + b.test_name));
-
-  return a_full < b_full;
+  DCHECK(!ContainsKey(tests_to_run_, run_test_info.GetFullName()));
+  tests_to_run_[run_test_info.GetFullName()] = run_test_info;
 }
 
 void WrapperTestLauncherDelegate::RunRemainingTests() {
-  std::sort(tests_to_run_.begin(), tests_to_run_.end(), CompareTestInfo);
-
   // PRE_ tests and tests that depend on them must share the same
   // data directory. Using test name as directory name leads to too long
   // names (exceeding UNIX_PATH_MAX, which creates a problem with
@@ -217,13 +204,17 @@ void WrapperTestLauncherDelegate::RunRemainingTests() {
   // and keep track of the names so that PRE_ tests can still re-use them.
   std::map<std::string, base::FilePath> temp_directories;
 
-  for (size_t i = 0; i < tests_to_run_.size(); i++) {
-    TestInfo test_info(tests_to_run_[i]);
+  // List of tests we can kick off right now, depending on no other tests.
+  std::vector<std::pair<std::string, CommandLine> > tests_to_run_now;
+
+  for (TestInfoMap::iterator i = tests_to_run_.begin();
+       i != tests_to_run_.end();
+       ++i) {
+    const TestInfo& test_info = i->second;
 
     // Make sure PRE_ tests and tests that depend on them share the same
     // data directory - based it on the test name without prefixes.
-    std::string test_name_no_pre = RemoveAnyPrePrefixes(
-        test_info.test_case_name + "." + test_info.test_name);
+    std::string test_name_no_pre(RemoveAnyPrePrefixes(test_info.GetFullName()));
     if (!ContainsKey(temp_directories, test_name_no_pre)) {
       base::FilePath temp_dir;
       CHECK(file_util::CreateTemporaryDirInDir(
@@ -235,7 +226,24 @@ void WrapperTestLauncherDelegate::RunRemainingTests() {
     CHECK(launcher_delegate_->AdjustChildProcessCommandLine(
               &new_cmd_line, temp_directories[test_name_no_pre]));
 
-    DoRunTest(test_info, new_cmd_line);
+    std::string pre_test_name(
+        test_info.test_case_name + "." + kPreTestPrefix + test_info.test_name);
+    if (ContainsKey(tests_to_run_, pre_test_name)) {
+      tests_to_run_[pre_test_name].callbacks.push_back(
+          base::Bind(&WrapperTestLauncherDelegate::RunDependentTest,
+                     base::Unretained(this),
+                     test_info.GetFullName(),
+                     new_cmd_line));
+    } else {
+      tests_to_run_now.push_back(
+          std::make_pair(test_info.GetFullName(), new_cmd_line));
+    }
+  }
+
+  for (size_t i = 0; i < tests_to_run_now.size(); i++) {
+    const TestInfo& test_info = tests_to_run_[tests_to_run_now[i].first];
+    const CommandLine& cmd_line = tests_to_run_now[i].second;
+    DoRunTest(test_info, cmd_line);
   }
 }
 
@@ -268,8 +276,7 @@ void WrapperTestLauncherDelegate::DoRunTest(const TestInfo& test_info,
   std::string test_name_no_pre = RemoveAnyPrePrefixes(
       test_info.test_case_name + "." + test_info.test_name);
 
-  parallel_launcher_.LaunchNamedSequencedChildGTestProcess(
-      test_name_no_pre,
+  parallel_launcher_.LaunchChildGTestProcess(
       new_cmd_line,
       browser_wrapper ? browser_wrapper : std::string(),
       TestTimeouts::action_max_timeout(),
@@ -278,6 +285,25 @@ void WrapperTestLauncherDelegate::DoRunTest(const TestInfo& test_info,
                  test_info));
 }
 
+void WrapperTestLauncherDelegate::RunDependentTest(
+    const std::string test_name,
+    const CommandLine& command_line,
+    const base::TestResult& pre_test_result) {
+  const TestInfo& test_info = tests_to_run_[test_name];
+  if (pre_test_result.status == base::TestResult::TEST_SUCCESS) {
+    // Only run the dependent test if PRE_ test succeeded.
+    DoRunTest(test_info, command_line);
+  } else {
+    // Otherwise skip the test.
+    base::TestResult test_result;
+    test_result.test_case_name = test_info.test_case_name;
+    test_result.test_name = test_info.test_name;
+    test_result.status = base::TestResult::TEST_SKIPPED;
+    for (size_t i = 0; i < test_info.callbacks.size(); i++)
+      test_info.callbacks[i].Run(test_result);
+  }
+}
+
 void WrapperTestLauncherDelegate::GTestCallback(
     const TestInfo& test_info,
     int exit_code,
@@ -303,7 +329,8 @@ void WrapperTestLauncherDelegate::GTestCallback(
   fprintf(stdout, "%s", output.c_str());
   fflush(stdout);
 
-  test_info.callback.Run(result);
+  for (size_t i = 0; i < test_info.callbacks.size(); i++)
+    test_info.callbacks[i].Run(result);
   parallel_launcher_.ResetOutputWatchdog();
 }