[chrome_elf] blacklist flaky tests

chrome_elf/blacklist/test/blacklist_test.cc

Index: chrome_elf/blacklist/test/blacklist_test.cc
diff --git a/chrome_elf/blacklist/test/blacklist_test.cc b/chrome_elf/blacklist/test/blacklist_test.cc
index ddcff2b328d561e1a6e4e37d660319c4fd8609db..08aa103f0ec059d99f3b45450d5653097896460b 100644
--- a/chrome_elf/blacklist/test/blacklist_test.cc
+++ b/chrome_elf/blacklist/test/blacklist_test.cc
@@ -18,6 +18,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_reg_util_win.h"
+#include "base/test/test_timeouts.h"
 #include "base/win/registry.h"
 #include "chrome/common/chrome_version.h"
 #include "chrome/install_static/install_util.h"
@@ -37,6 +38,12 @@ extern const wchar_t* kEnvVars[];
 
 namespace {
 
+// Timeouts for synchronization.
+#define event_timeout \

[robertshield, 2017/05/05 13:04:43]

nit, mildly prefer: kEventTimeout or EVENT_TIMEOUT

[grt (UTC plus 2), 2017/05/05 13:11:47]

if this can't be a true constant owing to TestTimeouts, then please use a
function -- macros are strongly discouraged:

DWORD SychronizationTimeout() {
  return static_cast<DWORD>(
      TestTimeouts::action_timeout().InMillisecondsRoundedUp());
}

or something along those lines.

+  static_cast<DWORD>((TestTimeouts::action_timeout()).InMillisecondsRoundedUp())
+
+const wchar_t* g_blacklist_test_mutex = L"ChromeElfBlacklistTestMutex";

[grt (UTC plus 2), 2017/05/05 13:11:47]

also from the nit factory:

constexpr wchar_t g_blacklist_test_mutex[] = ...;

is the preferred way to spell this since it ensures that the constant is in
read-only data and it retains the string length at compile in case anyone cares
to know about it.

+
 // Functions we need from blacklist_test_main_dll.dll
 typedef bool (*TestDll_AddDllToBlacklistFunction)(const wchar_t* dll_name);
 typedef int (*TestDll_BlacklistSizeFunction)();
@@ -73,7 +80,7 @@ class BlacklistTest : public testing::Test {
 
   void CheckBlacklistedDllsNotLoaded() {
     base::FilePath current_dir;
-    ASSERT_TRUE(PathService::Get(base::DIR_EXE, &current_dir));
+    EXPECT_TRUE(PathService::Get(base::DIR_EXE, &current_dir));
 
     for (size_t i = 0; i < arraysize(test_data); ++i) {
       // Ensure that the dll has not been loaded both by inspecting the handle
@@ -136,17 +143,17 @@ class BlacklistTest : public testing::Test {
   void IpcOverrides() {
     base::string16 temp = nt::GetTestingOverride(nt::HKCU);
     if (!temp.empty())
-      ASSERT_TRUE(::SetEnvironmentVariableW(L"hkcu_override", temp.c_str()));
+      EXPECT_TRUE(::SetEnvironmentVariableW(L"hkcu_override", temp.c_str()));
     temp = nt::GetTestingOverride(nt::HKLM);
     if (!temp.empty())
-      ASSERT_TRUE(::SetEnvironmentVariableW(L"hklm_override", temp.c_str()));
+      EXPECT_TRUE(::SetEnvironmentVariableW(L"hklm_override", temp.c_str()));
   }
 
   void SetUp() override {
     base::string16 temp;
-    ASSERT_NO_FATAL_FAILURE(
+    EXPECT_NO_FATAL_FAILURE(
         override_manager_.OverrideRegistry(HKEY_CURRENT_USER, &temp));
-    ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, temp));
+    EXPECT_TRUE(nt::SetTestingOverride(nt::HKCU, temp));
 
     // Make the override path available to our test DLL.
     IpcOverrides();
@@ -206,11 +213,16 @@ class BlacklistTest : public testing::Test {
     TestDll_RemoveDllFromBlacklist(kTestDllName2);
     TestDll_RemoveDllFromBlacklist(kTestDllName3);
 
-    ASSERT_TRUE(nt::SetTestingOverride(nt::HKCU, base::string16()));
+    EXPECT_TRUE(nt::SetTestingOverride(nt::HKCU, base::string16()));
   }
 };
 
 TEST_F(BlacklistTest, Beacon) {
+  HANDLE mutex = ::CreateMutexW(NULL, FALSE, g_blacklist_test_mutex);
+  EXPECT_TRUE(mutex != NULL);
+  DWORD timeout = ::IsDebuggerPresent() ? INFINITE : event_timeout;
+  EXPECT_EQ(WAIT_OBJECT_0, ::WaitForSingleObject(mutex, timeout));
+
   // Ensure that the beacon state starts off 'running' for this version.
   LONG result = blacklist_registry_key_->WriteValue(
       blacklist::kBeaconState, blacklist::BLACKLIST_SETUP_RUNNING);
@@ -225,9 +237,17 @@ TEST_F(BlacklistTest, Beacon) {
 
   // First call should succeed as the beacon is enabled.
   EXPECT_TRUE(blacklist::LeaveSetupBeacon());
+
+  EXPECT_TRUE(::ReleaseMutex(mutex));
+  EXPECT_TRUE(::CloseHandle(mutex));
 }
 
 TEST_F(BlacklistTest, AddAndRemoveModules) {
+  HANDLE mutex = ::CreateMutexW(NULL, FALSE, g_blacklist_test_mutex);
+  EXPECT_TRUE(mutex != NULL);
+  DWORD timeout = ::IsDebuggerPresent() ? INFINITE : event_timeout;
+  EXPECT_EQ(WAIT_OBJECT_0, ::WaitForSingleObject(mutex, timeout));
+
   EXPECT_TRUE(TestDll_AddDllToBlacklist(L"foo.dll"));
   // Adding the same item twice should be idempotent.
   EXPECT_TRUE(TestDll_AddDllToBlacklist(L"foo.dll"));
@@ -251,15 +271,23 @@ TEST_F(BlacklistTest, AddAndRemoveModules) {
   EXPECT_FALSE(TestDll_RemoveDllFromBlacklist(added_dlls[0].c_str()));
   EXPECT_FALSE(
       TestDll_RemoveDllFromBlacklist(added_dlls[empty_spaces - 1].c_str()));
+
+  EXPECT_TRUE(::ReleaseMutex(mutex));
+  EXPECT_TRUE(::CloseHandle(mutex));
 }
 
 TEST_F(BlacklistTest, SuccessfullyBlocked) {
+  HANDLE mutex = ::CreateMutexW(NULL, FALSE, g_blacklist_test_mutex);
+  EXPECT_TRUE(mutex != NULL);
+  DWORD timeout = ::IsDebuggerPresent() ? INFINITE : event_timeout;
+  EXPECT_EQ(WAIT_OBJECT_0, ::WaitForSingleObject(mutex, timeout));
+
   // Add 5 news dlls to blacklist.
   const int kDesiredBlacklistSize = 1;
   std::vector<base::string16> dlls_to_block;
   for (int i = 0; i < kDesiredBlacklistSize; ++i) {
     dlls_to_block.push_back(base::IntToString16(i) + L".dll");
-    ASSERT_TRUE(TestDll_AddDllToBlacklist(dlls_to_block[i].c_str()));
+    EXPECT_TRUE(TestDll_AddDllToBlacklist(dlls_to_block[i].c_str()));
   }
 
   // Block the dlls, one at a time, and ensure SuccesfullyBlocked correctly
@@ -269,11 +297,11 @@ TEST_F(BlacklistTest, SuccessfullyBlocked) {
 
     int size = 0;
     TestDll_SuccessfullyBlocked(NULL, &size);
-    ASSERT_EQ(num_initially_blocked_ + i + 1, size);
+    EXPECT_EQ(num_initially_blocked_ + i + 1, size);
 
     std::vector<const wchar_t*> blocked_dlls(size);
     TestDll_SuccessfullyBlocked(&(blocked_dlls[0]), &size);
-    ASSERT_EQ(num_initially_blocked_ + i + 1, size);
+    EXPECT_EQ(num_initially_blocked_ + i + 1, size);
 
     for (int j = 0; j <= i; ++j) {
       EXPECT_STREQ(blocked_dlls[num_initially_blocked_ + j],
@@ -285,15 +313,22 @@ TEST_F(BlacklistTest, SuccessfullyBlocked) {
   for (const auto& dll : dlls_to_block) {
     EXPECT_TRUE(TestDll_RemoveDllFromBlacklist(dll.c_str()));
   }
+
+  EXPECT_TRUE(::ReleaseMutex(mutex));
+  EXPECT_TRUE(::CloseHandle(mutex));
 }
 
-// Disabled due to flakiness.  https://crbug.com/711651.
-TEST_F(BlacklistTest, DISABLED_LoadBlacklistedLibrary) {
+TEST_F(BlacklistTest, LoadBlacklistedLibrary) {
+  HANDLE mutex = ::CreateMutexW(NULL, FALSE, g_blacklist_test_mutex);
+  EXPECT_TRUE(mutex != NULL);
+  DWORD timeout = ::IsDebuggerPresent() ? INFINITE : event_timeout;
+  EXPECT_EQ(WAIT_OBJECT_0, ::WaitForSingleObject(mutex, timeout));
+
   base::FilePath current_dir;
-  ASSERT_TRUE(PathService::Get(base::DIR_EXE, &current_dir));
+  EXPECT_TRUE(PathService::Get(base::DIR_EXE, &current_dir));
 
   // Ensure that the blacklist is loaded.
-  ASSERT_TRUE(TestDll_IsBlacklistInitialized());
+  EXPECT_TRUE(TestDll_IsBlacklistInitialized());
 
   // Test that an un-blacklisted DLL can load correctly.
   base::ScopedNativeLibrary dll1(current_dir.Append(kTestDllName1));
@@ -309,6 +344,9 @@ TEST_F(BlacklistTest, DISABLED_LoadBlacklistedLibrary) {
     EXPECT_TRUE(TestDll_AddDllToBlacklist(test_data[i].dll_name));
   }
   CheckBlacklistedDllsNotLoaded();
+
+  EXPECT_TRUE(::ReleaseMutex(mutex));
+  EXPECT_TRUE(::CloseHandle(mutex));
 }
 
 void TestResetBeacon(std::unique_ptr<base::win::RegKey>& key,
@@ -325,6 +363,11 @@ void TestResetBeacon(std::unique_ptr<base::win::RegKey>& key,
 }
 
 TEST_F(BlacklistTest, ResetBeacon) {
+  HANDLE mutex = ::CreateMutexW(NULL, FALSE, g_blacklist_test_mutex);
+  EXPECT_TRUE(mutex != NULL);
+  DWORD timeout = ::IsDebuggerPresent() ? INFINITE : event_timeout;
+  EXPECT_EQ(WAIT_OBJECT_0, ::WaitForSingleObject(mutex, timeout));
+
   // Ensure that ResetBeacon resets properly on successful runs and not on
   // failed or disabled runs.
   TestResetBeacon(blacklist_registry_key_,
@@ -338,9 +381,17 @@ TEST_F(BlacklistTest, ResetBeacon) {
   TestResetBeacon(blacklist_registry_key_,
                   blacklist::BLACKLIST_DISABLED,
                   blacklist::BLACKLIST_DISABLED);
+
+  EXPECT_TRUE(::ReleaseMutex(mutex));
+  EXPECT_TRUE(::CloseHandle(mutex));
 }
 
 TEST_F(BlacklistTest, SetupFailed) {
+  HANDLE mutex = ::CreateMutexW(NULL, FALSE, g_blacklist_test_mutex);
+  EXPECT_TRUE(mutex != NULL);
+  DWORD timeout = ::IsDebuggerPresent() ? INFINITE : event_timeout;
+  EXPECT_EQ(WAIT_OBJECT_0, ::WaitForSingleObject(mutex, timeout));
+
   // Ensure that when the number of failed tries reaches the maximum allowed,
   // the blacklist state is set to failed.
   LONG result = blacklist_registry_key_->WriteValue(
@@ -365,9 +416,17 @@ TEST_F(BlacklistTest, SetupFailed) {
                                                 &blacklist_state);
   EXPECT_EQ(ERROR_SUCCESS, result);
   EXPECT_EQ(blacklist_state, blacklist::BLACKLIST_SETUP_FAILED);
+
+  EXPECT_TRUE(::ReleaseMutex(mutex));
+  EXPECT_TRUE(::CloseHandle(mutex));
 }
 
 TEST_F(BlacklistTest, SetupSucceeded) {
+  HANDLE mutex = ::CreateMutexW(NULL, FALSE, g_blacklist_test_mutex);
+  EXPECT_TRUE(mutex != NULL);
+  DWORD timeout = ::IsDebuggerPresent() ? INFINITE : event_timeout;
+  EXPECT_EQ(WAIT_OBJECT_0, ::WaitForSingleObject(mutex, timeout));
+
   // Starting with the enabled beacon should result in the setup running state
   // and the attempt counter reset to zero.
   LONG result = blacklist_registry_key_->WriteValue(
@@ -388,6 +447,9 @@ TEST_F(BlacklistTest, SetupSucceeded) {
   blacklist_registry_key_->ReadValueDW(blacklist::kBeaconAttemptCount,
                                        &attempt_count);
   EXPECT_EQ(static_cast<DWORD>(0), attempt_count);
+
+  EXPECT_TRUE(::ReleaseMutex(mutex));
+  EXPECT_TRUE(::CloseHandle(mutex));
 }
 
 }  // namespace