Moved pre test logic to test launcher.

First, we make sure we reorder all tests in test launcher,
we guarantee that "PRE_test" will precede "test" for the delegate.

Test shuffeling was moved before re-ordering test for dependent test

Finally, content test launcher delegate was simplified.
It still has some logic to group dependent together
so they run consecutively in parallel launch.

Bug: 936248
Change-Id: I7d34e45eee1c2c0c88ded8e5cc054327fe27d7b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1638139
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@{#666365}

base/test/launcher/test_launcher.h

@@ -57,17 +57,13 @@ class TestLauncherDelegate {
   virtual bool WillRunTest(const std::string& test_case_name,
                            const std::string& test_name) = 0;
 
-  // Called before a test is considered for running. This method must return
-  // true if the TestLauncher is expected to run the test, provided it is part
-  // of the current shard.
-  virtual bool ShouldRunTest(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;
 
@@ -178,10 +174,18 @@ class TestLauncher {
   // Returns false if delegate fails to return tests.
   bool InitTests();
 
+  // Some of the TestLauncherDelegate implementations don't call into gtest
+  // until they've already split into test-specific processes. This results
+  // in gtest's native shuffle implementation attempting to shuffle one test.
+  // Shuffling the list of tests in the test launcher (before the delegate
+  // gets involved) ensures that the entire shard is shuffled.
+  bool ShuffleTests(CommandLine* command_line);
+
+  // Move all PRE_ tests prior to the final test in order.
   // Validate tests names. This includes no name conflict between tests
   // without DISABLED_ prefix, and orphaned PRE_ tests.
   // Returns false if any violation is found.
-  bool ValidateTests();
+  bool ReorderAndValidateTests();
 
   // Runs all tests in current iteration.
   void RunTests();
@@ -263,11 +267,7 @@ class TestLauncher {
   bool force_run_broken_tests_;
 
   // Tests to retry in this iteration.
-  std::set<std::string> tests_to_retry_;
-
-  // Support for test shuffling, just like gtest does.
-  bool shuffle_;
-  uint32_t shuffle_seed_;
+  std::unordered_set<std::string> tests_to_retry_;
 
   TestResultsTracker results_tracker_;
 

base/test/launcher/test_launcher_unittest.cc

@@ -34,9 +34,6 @@ class MockTestLauncherDelegate : public TestLauncherDelegate {
   MOCK_METHOD2(WillRunTest,
                bool(const std::string& test_case_name,
                     const std::string& test_name));
-  MOCK_METHOD2(ShouldRunTest,
-               bool(const std::string& test_case_name,
-                    const std::string& test_name));
   MOCK_METHOD2(RunTests,
                size_t(TestLauncher* test_launcher,
                       const std::vector<std::string>& test_names));
@@ -77,8 +74,6 @@ class TestLauncherTest : public testing::Test {
                                    testing::Return(true)));
     EXPECT_CALL(delegate, WillRunTest(_, _))
         .WillRepeatedly(testing::Return(true));
-    EXPECT_CALL(delegate, ShouldRunTest(_, _))
-        .WillRepeatedly(testing::Return(true));
   }
 
   std::unique_ptr<CommandLine> command_line;
@@ -148,6 +143,20 @@ TEST_F(TestLauncherTest, FilterDisabledTestByDefault) {
   EXPECT_TRUE(test_launcher.Run(command_line.get()));
 }
 
+// Test TestLauncher should reorder PRE_ tests before delegate
+TEST_F(TestLauncherTest, ReorderPreTests) {
+  AddMockedTests("Test", {"firstTest", "PRE_PRE_firstTest", "PRE_firstTest"});
+  SetUpExpectCalls();
+  using ::testing::_;
+  std::vector<std::string> tests_names = {
+      "Test.PRE_PRE_firstTest", "Test.PRE_firstTest", "Test.firstTest"};
+  EXPECT_CALL(delegate,
+              RunTests(_, testing::ElementsAreArray(tests_names.cbegin(),
+                                                    tests_names.cend())))
+      .WillOnce(testing::Return(0));
+  EXPECT_TRUE(test_launcher.Run(command_line.get()));
+}
+
 // Test TestLauncher "gtest_filter" switch.
 TEST_F(TestLauncherTest, UsingCommandLineFilter) {
   AddMockedTests("Test",
@@ -165,6 +174,21 @@ TEST_F(TestLauncherTest, UsingCommandLineFilter) {
   EXPECT_TRUE(test_launcher.Run(command_line.get()));
 }
 
+// Test TestLauncher gtest filter will include pre tests
+TEST_F(TestLauncherTest, FilterIncludePreTest) {
+  AddMockedTests("Test", {"firstTest", "secondTest", "PRE_firstTest"});
+  SetUpExpectCalls();
+  command_line->AppendSwitchASCII("gtest_filter", "Test.firstTest");
+  using ::testing::_;
+  std::vector<std::string> tests_names = {"Test.PRE_firstTest",
+                                          "Test.firstTest"};
+  EXPECT_CALL(delegate,
+              RunTests(_, testing::ElementsAreArray(tests_names.cbegin(),
+                                                    tests_names.cend())))
+      .WillOnce(testing::Return(0));
+  EXPECT_TRUE(test_launcher.Run(command_line.get()));
+}
+
 // Test TestLauncher "gtest_repeat" switch.
 TEST_F(TestLauncherTest, RunningMultipleIterations) {
   AddMockedTests("Test", {"firstTest"});
@@ -221,6 +245,30 @@ TEST_F(TestLauncherTest, FailOnRetryTests) {
   EXPECT_FALSE(test_launcher.Run(command_line.get()));
 }
 
+// Test TestLauncher should retry all PRE_ chained tests
+TEST_F(TestLauncherTest, RetryPreTests) {
+  AddMockedTests("Test", {"firstTest", "PRE_PRE_firstTest", "PRE_firstTest"});
+  SetUpExpectCalls();
+  using ::testing::_;
+  EXPECT_CALL(delegate, RunTests(_, _))
+      .WillOnce(::testing::DoAll(
+          OnTestResult("Test.PRE_PRE_firstTest", TestResult::TEST_SUCCESS),
+          OnTestResult("Test.PRE_firstTest", TestResult::TEST_FAILURE),
+          OnTestResult("Test.firstTest", TestResult::TEST_SUCCESS),
+          testing::Return(3)));
+  std::vector<std::string> tests_names = {
+      "Test.PRE_PRE_firstTest", "Test.PRE_firstTest", "Test.firstTest"};
+  EXPECT_CALL(delegate,
+              RetryTests(_, testing::ElementsAreArray(tests_names.cbegin(),
+                                                      tests_names.cend())))
+      .WillOnce(::testing::DoAll(
+          OnTestResult("Test.PRE_PRE_firstTest", TestResult::TEST_SUCCESS),
+          OnTestResult("Test.PRE_firstTest", TestResult::TEST_SUCCESS),
+          OnTestResult("Test.firstTest", TestResult::TEST_SUCCESS),
+          testing::Return(3)));
+  EXPECT_TRUE(test_launcher.Run(command_line.get()));
+}
+
 // Test TestLauncher run disabled unit tests switch.
 TEST_F(TestLauncherTest, RunDisabledTests) {
   AddMockedTests("DISABLED_TestDisabled", {"firstTest"});

base/test/launcher/unit_test_launcher.cc

@@ -676,14 +676,6 @@ bool UnitTestLauncherDelegate::WillRunTest(const std::string& test_case_name,
   return true;
 }
 
-bool UnitTestLauncherDelegate::ShouldRunTest(const std::string& test_case_name,
-                                             const std::string& test_name) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  // There is no additional logic to disable specific tests.
-  return true;
-}
-
 size_t UnitTestLauncherDelegate::RunTests(
     TestLauncher* test_launcher,
     const std::vector<std::string>& test_names) {

base/test/launcher/unit_test_launcher.h

@@ -136,8 +136,6 @@ 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;
-  bool ShouldRunTest(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,