Checking for gtest name duplication.

Fail TestLauncher if disabled tests shares a name with another test.

Added unit tests for TestLauncher disabled tests logic.

Changed TestResultTracker unit test to better reflect disabled test tags.

Changed problematic disabled tests name that caused CQ to fail.
DISABLED_BrowserCloseInfiniteBeforeUnload was removed since
it is a copy of BrowserCloseInfiniteBeforeUnload.

Bug: 961424,960619
Change-Id: I064a6d5c06e5dfccab2e54d2d5e54ff3903e81f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1603483
Commit-Queue: Ilia Samsonov <isamsonov@google.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658846}

base/test/launcher/test_launcher.cc

@@ -87,6 +87,12 @@ const char kTestTotalShards[] = "GTEST_TOTAL_SHARDS";
 // The environment variable name for the test shard index.
 const char kTestShardIndex[] = "GTEST_SHARD_INDEX";
 
+// Prefix indicating test has to run prior to the other test.
+const char kPreTestPrefix[] = "PRE_";
+
+// Prefix indicating test is disabled, will not run unless specified.
+const char kDisabledTestPrefix[] = "DISABLED_";
+
 namespace {
 
 // Global tag for test runs where the results are unreliable for any reason.
@@ -570,6 +576,69 @@ const char kIsolatedScriptRunDisabledTestsFlag[] =
 const char kIsolatedScriptTestFilterFlag[] = "isolated-script-test-filter";
 const char kIsolatedScriptTestRepeatFlag[] = "isolated-script-test-repeat";
 
+class TestLauncher::TestInfo {
+ public:
+  TestInfo() = default;
+  TestInfo(const TestInfo& other) = default;
+  TestInfo(const TestIdentifier& test_id);
+  ~TestInfo() = default;
+
+  // Returns test name excluding DISABLE_ prefix.
+  std::string GetDisabledStrippedName() const;
+
+  // Returns full test name.
+  std::string GetFullName() const;
+
+  // Returns test name with PRE_ prefix added, excluding DISABLE_ prefix.
+  std::string GetPreName() const;
+
+  const std::string& test_case_name() const { return test_case_name_; }
+  const std::string& test_name() const { return test_name_; }
+  const std::string& file() const { return file_; }
+  int line() const { return line_; }
+  bool disabled() const { return disabled_; }
+  bool pre_test() const { return pre_test_; }
+
+ private:
+  std::string test_case_name_;
+  std::string test_name_;
+  std::string file_;
+  int line_;
+  bool disabled_;
+  bool pre_test_;
+};
+
+TestLauncher::TestInfo::TestInfo(const TestIdentifier& test_id)
+    : test_case_name_(test_id.test_case_name),
+      test_name_(test_id.test_name),
+      file_(test_id.file),
+      line_(test_id.line),
+      disabled_(false),
+      pre_test_(false) {
+  disabled_ = GetFullName().find(kDisabledTestPrefix) != std::string::npos;
+  pre_test_ = test_name_.find(kPreTestPrefix) != std::string::npos;
+}
+
+std::string TestLauncher::TestInfo::GetDisabledStrippedName() const {
+  std::string test_name = GetFullName();
+  ReplaceSubstringsAfterOffset(&test_name, 0, kDisabledTestPrefix,
+                               std::string());
+  return test_name;
+}
+
+std::string TestLauncher::TestInfo::GetFullName() const {
+  return FormatFullTestName(test_case_name_, test_name_);
+}
+
+std::string TestLauncher::TestInfo::GetPreName() const {
+  std::string name = test_name_;
+  ReplaceSubstringsAfterOffset(&name, 0, kDisabledTestPrefix, std::string());
+  std::string case_name = test_case_name_;
+  ReplaceSubstringsAfterOffset(&case_name, 0, kDisabledTestPrefix,
+                               std::string());
+  return FormatFullTestName(case_name, kPreTestPrefix + name);
+}
+
 TestLauncherDelegate::~TestLauncherDelegate() = default;
 
 TestLauncher::LaunchOptions::LaunchOptions() = default;
@@ -1038,10 +1107,11 @@ bool TestLauncher::Init(CommandLine* command_line) {
     }
   }
 
-  if (!launcher_delegate_->GetTests(&tests_)) {
-    LOG(ERROR) << "Failed to get list of tests.";
+  if (!InitTests())
+    return false;
+
+  if (!ValidateTests())
     return false;
-  }
 
   if (command_line->HasSwitch(switches::kTestLauncherPrintTestStdio)) {
     std::string print_test_stdio = command_line->GetSwitchValueASCII(
@@ -1157,6 +1227,51 @@ bool TestLauncher::Init(CommandLine* command_line) {
   return true;
 }
 
+bool TestLauncher::InitTests() {
+  std::vector<TestIdentifier> tests;
+  if (!launcher_delegate_->GetTests(&tests)) {
+    LOG(ERROR) << "Failed to get list of tests.";
+    return false;
+  }
+  for (const TestIdentifier& test_id : tests) {
+    TestInfo test_info(test_id);
+    tests_.push_back(test_info);
+  }
+  return true;
+}
+
+bool TestLauncher::ValidateTests() {
+  bool result = true;
+  std::unordered_set<std::string> disabled_tests;
+  std::unordered_set<std::string> pre_tests;
+
+  // Find disabled and pre tests
+  for (const TestInfo& test_info : tests_) {
+    if (test_info.disabled())
+      disabled_tests.insert(test_info.GetDisabledStrippedName());
+    if (test_info.pre_test())
+      pre_tests.insert(test_info.GetDisabledStrippedName());
+  }
+
+  for (const TestInfo& test_info : tests_) {
+    // If any test has a matching disabled test, fail and log for audit.
+    if (base::ContainsKey(disabled_tests, test_info.GetFullName())) {
+      LOG(ERROR) << test_info.GetFullName()
+                 << " duplicated by a DISABLED_ test";
+      result = false;
+    }
+    // Remove pre tests that have a matching subsequent test.
+    pre_tests.erase(test_info.GetPreName());
+  }
+
+  // If any tests remain in pre_tests set, fail and log for audit.
+  for (const std::string& pre_test : pre_tests) {
+    LOG(ERROR) << pre_test << " is an orphaned pre test";
+    result = false;
+  }
+  return result;
+}
+
 void TestLauncher::CreateAndStartThreadPool(int num_parallel_jobs) {
   // These values are taken from ThreadPool::StartWithDefaultParams(), which
   // is not used directly to allow a custom number of threads in the foreground
@@ -1182,7 +1297,7 @@ void TestLauncher::CombinePositiveTestFilters(
   // in both filters.
   if (!filter_a.empty() && !filter_b.empty()) {
     for (const auto& i : tests_) {
-      std::string test_name = FormatFullTestName(i.test_case_name, i.test_name);
+      std::string test_name = i.GetFullName();
       bool found_a = false;
       bool found_b = false;
       for (const auto& k : filter_a) {
@@ -1204,30 +1319,25 @@ void TestLauncher::CombinePositiveTestFilters(
 
 void TestLauncher::RunTests() {
   std::vector<std::string> test_names;
-  for (const TestIdentifier& test_id : tests_) {
-    std::string test_name =
-        FormatFullTestName(test_id.test_case_name, test_id.test_name);
-
+  for (const TestInfo& test_info : tests_) {
+    std::string test_name = test_info.GetFullName();
     results_tracker_.AddTest(test_name);
-
-    if (test_name.find("DISABLED") != std::string::npos) {
+    // Skip disabled tests unless explicitly requested.
+    if (test_info.disabled()) {
       results_tracker_.AddDisabledTest(test_name);
-
-      // Skip disabled tests unless explicitly requested.
       if (skip_diabled_tests_)
         continue;
     }
 
-    bool will_run_test = launcher_delegate_->WillRunTest(test_id.test_case_name,
-                                                         test_id.test_name);
+    bool will_run_test = launcher_delegate_->WillRunTest(
+        test_info.test_case_name(), test_info.test_name());
     if (!will_run_test)
       continue;
 
     // Count tests in the binary, before we apply filter and sharding.
     test_found_count_++;
 
-    std::string test_name_no_disabled =
-        TestNameWithoutDisabledPrefix(test_name);
+    std::string test_name_no_disabled = test_info.GetDisabledStrippedName();
 
     // Skip the test that doesn't match the filter (if given).
     if (has_at_least_one_positive_filter_) {
@@ -1263,8 +1373,8 @@ void TestLauncher::RunTests() {
     size_t index_of_first_period = test_name_to_bucket.find(".");
     if (index_of_first_period == std::string::npos)
       index_of_first_period = 0;
-    base::ReplaceSubstringsAfterOffset(&test_name_to_bucket,
-                                       index_of_first_period, "PRE_", "");
+    base::ReplaceSubstringsAfterOffset(
+        &test_name_to_bucket, index_of_first_period, "PRE_", std::string());
 
     if (Hash(test_name_to_bucket) % total_shards_ !=
         static_cast<uint32_t>(shard_index_)) {
@@ -1273,10 +1383,11 @@ void TestLauncher::RunTests() {
 
     // Report test locations after applying all filters, so that we report test
     // locations only for those tests that were run as part of this shard.
-    results_tracker_.AddTestLocation(test_name, test_id.file, test_id.line);
+    results_tracker_.AddTestLocation(test_name, test_info.file(),
+                                     test_info.line());
 
     bool should_run_test = launcher_delegate_->ShouldRunTest(
-        test_id.test_case_name, test_id.test_name);
+        test_info.test_case_name(), test_info.test_name());
     if (should_run_test) {
       // Only a subset of tests that are run require placeholders -- namely,
       // those that will output results.

base/test/launcher/test_launcher.h

@@ -180,6 +180,15 @@ class TestLauncher {
  private:
   bool Init(CommandLine* command_line) WARN_UNUSED_RESULT;
 
+  // Gets tests from the delegate, and converts to TestInfo objects.
+  // Returns false if delegate fails to return tests.
+  bool InitTests();
+
+  // 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();
+
   // Runs all tests in current iteration.
   void RunTests();
 
@@ -226,8 +235,11 @@ class TestLauncher {
   std::vector<std::string> positive_test_filter_;
   std::vector<std::string> negative_test_filter_;
 
+  // Class to encapsulate gtest information.
+  class TestInfo;
+
   // Tests to use (cached result of TestLauncherDelegate::GetTests).
-  std::vector<TestIdentifier> tests_;
+  std::vector<TestInfo> tests_;
 
   // Number of tests found in this binary.
   size_t test_found_count_;

base/test/launcher/test_launcher_unittest.cc

@@ -61,21 +61,25 @@ class TestLauncherTest : public testing::Test {
         FROM_HERE, BindOnce(&RunLoop::QuitCurrentWhenIdleDeprecated));
   }
 
-  // Setup expected delegate calls, and which tests the delegate will return.
-  void SetUpExpectCalls(std::vector<const std::string> test_names) {
-    std::vector<TestIdentifier> tests;
+  // Adds tests to be returned by the delegate.
+  void AddMockedTests(std::string test_case_name,
+                      const std::vector<std::string>& test_names) {
     for (const std::string& test_name : test_names) {
       TestIdentifier test_data;
-      test_data.test_case_name = "Test";
+      test_data.test_case_name = test_case_name;
       test_data.test_name = test_name;
       test_data.file = "File";
       test_data.line = 100;
-      tests.push_back(test_data);
+      tests_.push_back(test_data);
     }
+  }
+
+  // Setup expected delegate calls, and which tests the delegate will return.
+  void SetUpExpectCalls() {
     using ::testing::_;
     EXPECT_CALL(delegate, GetTests(_))
-        .WillOnce(testing::DoAll(testing::SetArgPointee<0>(tests),
-                                 testing::Return(true)));
+        .WillOnce(::testing::DoAll(testing::SetArgPointee<0>(tests_),
+                                   testing::Return(true)));
     EXPECT_CALL(delegate, WillRunTest(_, _))
         .WillRepeatedly(testing::Return(true));
     EXPECT_CALL(delegate, ShouldRunTest(_, _))
@@ -86,11 +90,39 @@ class TestLauncherTest : public testing::Test {
   MockTestLauncher test_launcher;
   MockTestLauncherDelegate delegate;
   base::test::ScopedTaskEnvironment scoped_task_environment;
+
+ private:
+  std::vector<TestIdentifier> tests_;
 };
 
+// A test and a disabled test cannot share a name.
+TEST_F(TestLauncherTest, TestNameSharedWithDisabledTest) {
+  AddMockedTests("Test", {"firstTest", "DISABLED_firstTest"});
+  SetUpExpectCalls();
+  EXPECT_FALSE(test_launcher.Run(command_line.get()));
+}
+
+// A test case and a disabled test case cannot share a name.
+TEST_F(TestLauncherTest, TestNameSharedWithDisabledTestCase) {
+  AddMockedTests("DISABLED_Test", {"firstTest"});
+  AddMockedTests("Test", {"firstTest"});
+  SetUpExpectCalls();
+  EXPECT_FALSE(test_launcher.Run(command_line.get()));
+}
+
+// Compiled tests should not contain an orphaned pre test.
+TEST_F(TestLauncherTest, OrphanePreTest) {
+  AddMockedTests("Test", {"firstTest", "PRE_firstTestOrphane"});
+  SetUpExpectCalls();
+  EXPECT_FALSE(test_launcher.Run(command_line.get()));
+}
+
 // Test TestLauncher filters DISABLED tests by default.
 TEST_F(TestLauncherTest, FilterDisabledTestByDefault) {
-  SetUpExpectCalls({"firstTest", "secondTest", "DISABLED_firstTest"});
+  AddMockedTests("DISABLED_TestDisabled", {"firstTest"});
+  AddMockedTests("Test",
+                 {"firstTest", "secondTest", "DISABLED_firstTestDisabled"});
+  SetUpExpectCalls();
   using ::testing::_;
   std::vector<std::string> tests_names = {"Test.firstTest", "Test.secondTest"};
   EXPECT_CALL(delegate,
@@ -102,8 +134,10 @@ TEST_F(TestLauncherTest, FilterDisabledTestByDefault) {
 
 // Test TestLauncher "gtest_filter" switch.
 TEST_F(TestLauncherTest, UsingCommandLineSwitch) {
-  SetUpExpectCalls({"firstTest", "secondTest", "DISABLED_firstTest"});
-  command_line->AppendSwitchASCII("gtest_filter", "Test.first*");
+  AddMockedTests("Test",
+                 {"firstTest", "secondTest", "DISABLED_firstTestDisabled"});
+  SetUpExpectCalls();
+  command_line->AppendSwitchASCII("gtest_filter", "Test*.first*");
   using ::testing::_;
   std::vector<std::string> tests_names = {"Test.firstTest"};
   EXPECT_CALL(delegate,
@@ -115,12 +149,16 @@ TEST_F(TestLauncherTest, UsingCommandLineSwitch) {
 
 // Test TestLauncher run disabled unit tests switch.
 TEST_F(TestLauncherTest, RunDisabledTests) {
-  SetUpExpectCalls({"firstTest", "secondTest", "DISABLED_firstTest"});
+  AddMockedTests("DISABLED_TestDisabled", {"firstTest"});
+  AddMockedTests("Test",
+                 {"firstTest", "secondTest", "DISABLED_firstTestDisabled"});
+  SetUpExpectCalls();
   command_line->AppendSwitch("gtest_also_run_disabled_tests");
-  command_line->AppendSwitchASCII("gtest_filter", "Test.first*");
+  command_line->AppendSwitchASCII("gtest_filter", "Test*.first*");
   using ::testing::_;
-  std::vector<std::string> tests_names = {"Test.firstTest",
-                                          "Test.DISABLED_firstTest"};
+  std::vector<std::string> tests_names = {"DISABLED_TestDisabled.firstTest",
+                                          "Test.firstTest",
+                                          "Test.DISABLED_firstTestDisabled"};
   EXPECT_CALL(delegate,
               RunTests(_, testing::ElementsAreArray(tests_names.cbegin(),
                                                     tests_names.cend())))
@@ -139,7 +177,7 @@ TEST_F(TestLauncherTest, FaultyShardSetup) {
 
 // Shard index must be lesser than total shards
 TEST_F(TestLauncherTest, RedirectStdio) {
-  SetUpExpectCalls({});
+  SetUpExpectCalls();
   command_line->AppendSwitchASCII("test-launcher-print-test-stdio", "always");
   using ::testing::_;
   EXPECT_CALL(delegate, RunTests(_, _)).Times(1);

base/test/launcher/test_results_tracker_unittest.cc

@@ -167,9 +167,10 @@ TEST_F(TestResultsTrackerTester, JsonSummaryRootTags) {
   tracker.OnTestIterationStarting();
   tracker.AddTest("Test2");  // Test should appear in alphabetical order.
   tracker.AddTest("Test1");
-  tracker.AddTest("Test3");
   tracker.AddTest("Test1");  // Test should only appear once.
-  tracker.AddDisabledTest("Test3");
+  // DISABLED_ prefix should be removed.
+  tracker.AddTest("DISABLED_Test3");
+  tracker.AddDisabledTest("DISABLED_Test3");
   tracker.AddGlobalTag("global1");
 
   Optional<Value> root = SaveAndReadSummary();
@@ -274,6 +275,31 @@ TEST_F(TestResultsTrackerTester, JsonSummaryTestPlaceholderOrder) {
   ValidateTestResult(iteration_val, test_result);
 }
 
+// Disabled tests results are not saved in json summary.
+TEST_F(TestResultsTrackerTester, JsonSummaryDisabledTestResults) {
+  tracker.AddDisabledTest("Test");
+  TestResult test_result =
+      GenerateTestResult("Test", TestResult::TEST_SUCCESS,
+                         TimeDelta::FromMilliseconds(30), "output");
+  test_result.test_result_parts.push_back(GenerateTestResultPart(
+      TestResultPart::kSuccess, "TestFile", 110, "summary", "message"));
+  tracker.AddTestPlaceholder("Test");
+
+  tracker.OnTestIterationStarting();
+  tracker.GeneratePlaceholderIteration();
+  tracker.AddTestResult(test_result);
+
+  Optional<Value> root = SaveAndReadSummary();
+
+  Value* val = root->FindListKey("per_iteration_data");
+  ASSERT_TRUE(val);
+  ASSERT_EQ(1u, val->GetList().size());
+
+  Value* iteration_val = &(val->GetList().at(0));
+  // No result is saved since a placeholder was not specified.
+  EXPECT_EQ(0u, iteration_val->DictSize());
+}
+
 }  // namespace
 
 }  // namespace base

chrome/browser/extensions/api/system_indicator/system_indicator_apitest.cc

@@ -49,16 +49,17 @@ class SystemIndicatorApiTest : public ExtensionApiTest {
 
 // https://crbug.com/960363: Test crashes on ChromeOS.
 #if defined(OS_CHROMEOS)
-#define MAYBE_SystemIndicator DISABLED_SystemIndicator_CrOS
+#define MAYBE_SystemIndicatorBasic DISABLED_SystemIndicatorBasic
 #else
-#define MAYBE_SystemIndicator SystemIndicator
+#define MAYBE_SystemIndicatorBasic SystemIndicatorBasic
 #endif
-IN_PROC_BROWSER_TEST_F(SystemIndicatorApiTest, MAYBE_SystemIndicator) {
+IN_PROC_BROWSER_TEST_F(SystemIndicatorApiTest, MAYBE_SystemIndicatorBasic) {
   ASSERT_TRUE(RunExtensionTest("system_indicator/basics")) << message_;
 }
 
 // Failing on 10.6, flaky elsewhere http://crbug.com/497643
-IN_PROC_BROWSER_TEST_F(SystemIndicatorApiTest, DISABLED_SystemIndicator) {
+IN_PROC_BROWSER_TEST_F(SystemIndicatorApiTest,
+                       DISABLED_SystemIndicatorUnloaded) {
   ResultCatcher catcher;
 
   const Extension* extension =

chrome/browser/extensions/api/system_indicator/system_indicator_manager.h

@@ -22,7 +22,7 @@ class Profile;
 class StatusTray;
 
 namespace extensions {
-FORWARD_DECLARE_TEST(SystemIndicatorApiTest, SystemIndicator);
+FORWARD_DECLARE_TEST(SystemIndicatorApiTest, SystemIndicatorUnloaded);
 
 class ExtensionIndicatorIcon;
 class ExtensionRegistry;
@@ -48,7 +48,7 @@ class SystemIndicatorManager : public ExtensionRegistryObserver,
   void Shutdown() override;
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(SystemIndicatorApiTest, SystemIndicator);
+  FRIEND_TEST_ALL_PREFIXES(SystemIndicatorApiTest, SystemIndicatorUnloaded);
 
   // A structure representing the system indicator for an extension.
   struct SystemIndicator {

chrome/browser/unload_browsertest.cc

@@ -566,17 +566,6 @@ IN_PROC_BROWSER_TEST_F(UnloadTest, BrowserCloseInfiniteUnload) {
   LoadUrlAndQuitBrowser(INFINITE_UNLOAD_HTML, "infiniteunload");
 }
 
-// Tests closing the browser with a beforeunload handler that hangs.
-// If this flakes, use http://crbug.com/78803 and http://crbug.com/86469
-IN_PROC_BROWSER_TEST_F(UnloadTest, DISABLED_BrowserCloseInfiniteBeforeUnload) {
-  // Tests makes no sense in single-process mode since the renderer is hung.
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSingleProcess))
-    return;
-
-  LoadUrlAndQuitBrowser(INFINITE_BEFORE_UNLOAD_HTML, "infinitebeforeunload");
-}
-
 // Tests closing the browser on a page with an unload listener registered where
 // the unload handler has an infinite loop followed by an alert.
 // If this flakes, use http://crbug.com/86469

device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc

@@ -882,7 +882,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 #define MAYBE_WriteRemoteCharacteristic_Twice WriteRemoteCharacteristic_Twice
 #else
 #define MAYBE_WriteRemoteCharacteristic_Twice \
-  DISABLEDWriteRemoteCharacteristic_Twice
+  DISABLED_WriteRemoteCharacteristic_Twice
 #endif
 // Tests WriteRemoteCharacteristic multiple times.
 #if defined(OS_WIN)

ui/ozone/gl/gl_image_ozone_native_pixmap_unittest.cc

@@ -82,7 +82,7 @@ using GLImageScanoutType = testing::Types<
     GLImageNativePixmapTestDelegate<gfx::BufferUsage::SCANOUT,
                                     gfx::BufferFormat::BGRA_8888>>;
 
-INSTANTIATE_TYPED_TEST_SUITE_P(GLImageNativePixmapScanout,
+INSTANTIATE_TYPED_TEST_SUITE_P(GLImageNativePixmapScanoutBGRA,
                                GLImageTest,
                                GLImageScanoutType);
 
@@ -92,7 +92,7 @@ using GLImageScanoutTypeDisabled = testing::Types<
 
 // This test is disabled since we need mesa support for XR30/XB30 that is not
 // available on many boards yet.
-INSTANTIATE_TYPED_TEST_SUITE_P(DISABLED_GLImageNativePixmapScanout,
+INSTANTIATE_TYPED_TEST_SUITE_P(DISABLED_GLImageNativePixmapScanoutRGBX,
                                GLImageTest,
                                GLImageScanoutTypeDisabled);