Deflake a few flaky tests (#516)

jonsimantov · web-flow · commit 501e5fa5c22e · 2021-07-08T12:59:59.000-04:00
Refactor a few tests to use [`RunFlakyBlock`](https://github.com/firebase/firebase-cpp-sdk/blob/acac90f7939f30a48c24ba5aa8d69fffe1ef6927/testing/test_framework/src/firebase_test_framework.h#L204): - FirebaseInstallationsTest.TestGettingTokenTwiceMatches (copied logic from TestGettingIdTwiceMatches) - FirebaseInstallationsTest.TestDeleteGivesNewTokenNextTime (copied logic from TestDeleteGivesNewIdNextTime) - FirebaseMessagingTest.TestSendMessageToTopic Additionally, to make the RunFlakyBlock tests more readable, this PR adds some FLAKY_EXPECT_* macros for equality and boolean values, as well as FLAKY_WAIT_FOR_COMPLETION. Refactored a Storage test to use these.
diff --git a/installations/integration_test/src/integration_test.cc b/installations/integration_test/src/integration_test.cc
@@ -147,30 +147,15 @@ TEST_F(FirebaseInstallationsTest, TestGettingIdTwiceMatches) {
   if (!RunFlakyBlock(
           [](firebase::installations::Installations* installations) {
             firebase::Future<std::string> id = installations->GetId();
-            WaitForCompletionAnyResult(id, "GetId");
-            if (id.error() != 0) {
-              LogError("GetId returned error %d: %s", id.error(),
-                       id.error_message());
-              return false;
-            }
-            if (*id.result() == "") {
-              LogError("GetId returned blank");
-              return false;
-            }
+            FLAKY_WAIT_FOR_COMPLETION(id, "GetId");
+            FLAKY_EXPECT_NE(*id.result(), "");  // ensure non-blank
             std::string first_id = *id.result();
             id = installations->GetId();
-            WaitForCompletionAnyResult(id, "GetId 2");
-            if (id.error() != 0) {
-              LogError("GetId 2 returned error %d: %s", id.error(),
-                       id.error_message());
-              return false;
-            }
-            if (*id.result() != first_id) {
-              LogError(
-                  "GetId 2 returned non-matching ID: first(%s) vs second(%s)",
-                  first_id.c_str(), id.result()->c_str());
-              return false;
-            }
+            FLAKY_WAIT_FOR_COMPLETION(id, "GetId 2");
+            FLAKY_EXPECT_NE(*id.result(), "");  // ensure non-blank
+
+            // Ensure the second ID returned is the same as the first.
+            FLAKY_EXPECT_EQ(*id.result(), first_id);
             return true;
           },
           installations_)) {
@@ -182,47 +167,24 @@ TEST_F(FirebaseInstallationsTest, TestDeleteGivesNewIdNextTime) {
   if (!RunFlakyBlock(
           [](firebase::installations::Installations* installations) {
             firebase::Future<std::string> id = installations->GetId();
-            WaitForCompletionAnyResult(id, "GetId");
-            if (id.error() != 0) {
-              LogError("GetId returned error %d: %s", id.error(),
-                       id.error_message());
-              return false;
-            }
-            if (*id.result() == "") {
-              LogError("GetId returned blank");
-              return false;
-            }
+            FLAKY_WAIT_FOR_COMPLETION(id, "GetId");
+            FLAKY_EXPECT_NE(*id.result(), "");  // ensure non-blank
             std::string first_id = *id.result();
 
             firebase::Future<void> del = installations->Delete();
-            WaitForCompletionAnyResult(del, "Delete");
-            if (del.error() != 0) {
-              LogError("Delete returned error %d: %s", id.error(),
-                       id.error_message());
-              return false;
-            }
+            FLAKY_WAIT_FOR_COMPLETION(del, "Delete");
 
             // Ensure that we now get a different installations id.
             id = installations->GetId();
-            WaitForCompletionAnyResult(id, "GetId 2");
-            if (id.error() != 0) {
-              LogError("GetId 2 returned error %d: %s", id.error(),
-                       id.error_message());
-              return false;
-            }
-            if (*id.result() == "") {
-              LogError("GetId 2 returned blank");
-              return false;
-            }
+            FLAKY_WAIT_FOR_COMPLETION(id, "GetId 2");
+            FLAKY_EXPECT_NE(*id.result(), "");  // ensure non-blank
 #if defined(__ANDROID__) || (defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE)
             // Desktop is a stub and returns the same ID, but on mobile it
             // should return a new ID.
-            if (*id.result() == first_id) {
-              LogError("IDs match (should be different): %s", first_id.c_str());
-              return false;
-            }
+            FLAKY_EXPECT_NE(*id.result(), first_id);
 #endif  // defined(__ANDROID__) || (defined(TARGET_OS_IPHONE) &&
         // TARGET_OS_IPHONE)
+
             return true;
           },
           installations_)) {
@@ -237,34 +199,52 @@ TEST_F(FirebaseInstallationsTest, TestCanGetToken) {
 }
 
 TEST_F(FirebaseInstallationsTest, TestGettingTokenTwiceMatches) {
-  firebase::Future<std::string> token = installations_->GetToken(false);
-  WaitForCompletion(token, "GetToken");
-  EXPECT_NE(*token.result(), "");
-  std::string first_token = *token.result();
-  token = installations_->GetToken(false);
-  WaitForCompletion(token, "GetToken 2");
-  EXPECT_EQ(*token.result(), first_token);
+  if (!RunFlakyBlock(
+          [](firebase::installations::Installations* installations) {
+            firebase::Future<std::string> token =
+                installations->GetToken(false);
+            FLAKY_WAIT_FOR_COMPLETION(token, "GetToken");
+            FLAKY_EXPECT_NE(*token.result(), "");  // ensure non-blank
+            std::string first_token = *token.result();
+            token = installations->GetToken(false);
+            FLAKY_WAIT_FOR_COMPLETION(token, "GetToken 2");
+            FLAKY_EXPECT_NE(*token.result(), "");  // ensure non-blank
+            FLAKY_EXPECT_EQ(*token.result(), first_token);
+            return true;
+          },
+          installations_)) {
+    FAIL() << "Test failed, check error log for details.";
+  }
 }
 
 TEST_F(FirebaseInstallationsTest, TestDeleteGivesNewTokenNextTime) {
-  firebase::Future<std::string> token = installations_->GetToken(false);
-  WaitForCompletion(token, "GetToken");
-  EXPECT_NE(*token.result(), "");
-  std::string first_token = *token.result();
+  if (!RunFlakyBlock(
+          [](firebase::installations::Installations* installations) {
+            firebase::Future<std::string> token =
+                installations->GetToken(false);
+            FLAKY_WAIT_FOR_COMPLETION(token, "GetToken");
+            FLAKY_EXPECT_NE(*token.result(), "");  // ensure non-blank
+            std::string first_token = *token.result();
 
-  firebase::Future<void> del = installations_->Delete();
-  WaitForCompletion(del, "Delete");
+            firebase::Future<void> del = installations->Delete();
+            FLAKY_WAIT_FOR_COMPLETION(del, "Delete");
 
-  // Ensure that we now get a different installations token.
-  token = installations_->GetToken(false);
-  WaitForCompletion(token, "GetToken 2");
-  EXPECT_NE(*token.result(), "");
+            // Ensure that we now get a different installations token.
+            token = installations->GetToken(false);
+            FLAKY_WAIT_FOR_COMPLETION(token, "GetToken 2");
+            FLAKY_EXPECT_NE(*token.result(), "");  // ensure non-blank
 #if defined(__ANDROID__) || (defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE)
-  // Desktop is a stub and returns the same token, but on mobile it should
-  // return a new token.
-  EXPECT_NE(*token.result(), first_token);
+            // Desktop is a stub and returns the same token, but on mobile it
+            // should return a new token.
+            FLAKY_EXPECT_NE(*token.result(), first_token);
 #endif  // defined(__ANDROID__) || (defined(TARGET_OS_IPHONE) &&
         // TARGET_OS_IPHONE)
+
+            return true;
+          },
+          installations_)) {
+    FAIL() << "Test failed, check error log for details.";
+  }
 }
 
 TEST_F(FirebaseInstallationsTest, TestCanGetIdAndTokenTogether) {
diff --git a/messaging/integration_test/src/integration_test.cc b/messaging/integration_test/src/integration_test.cc
@@ -55,7 +55,9 @@ const int kTimeoutSeconds = 120;
 const char kTestingNotificationKey[] = "fcm_testing_notification";
 
 using app_framework::LogDebug;
+using app_framework::LogError;
 using app_framework::LogInfo;
+using app_framework::LogWarning;
 
 using app_framework::GetCurrentTimeInMicroseconds;
 using app_framework::PathForResource;
@@ -90,7 +92,6 @@ class FirebaseMessagingTest : public FirebaseTest {
       const char* notification_body,
       const std::map<std::string, std::string>& message_fields);
 
- protected:
   // Get a unique message ID so we can confirm the correct message is being
   // received.
   std::string GetUniqueMessageId();
@@ -106,6 +107,7 @@ class FirebaseMessagingTest : public FirebaseTest {
   bool WaitForMessage(firebase::messaging::Message* message_out,
                       int timeout = kTimeoutSeconds);
 
+ protected:
   static firebase::App* shared_app_;
   static firebase::messaging::PollableListener* shared_listener_;
   static std::string* shared_token_;
@@ -218,6 +220,12 @@ bool FirebaseMessagingTest::CreateTestMessage(
     // Don't send HTTP requests in stub mode.
     return false;
   }
+  if (strcmp(kFcmServerKey, "REPLACE_WITH_YOUR_SERVER_KEY") == 0) {
+    LogWarning(
+        "Please put your Firebase Cloud Messaging server key in "
+        "kFcmServerKey.");
+    LogWarning("Without a server key, most of these tests will fail.");
+  }
   std::map<std::string, std::string> headers;
   headers.insert(std::make_pair("Content-type", "application/json"));
   headers.insert(
@@ -504,37 +512,54 @@ TEST_F(FirebaseMessagingTest, TestSendMessageToTopic) {
 
   EXPECT_TRUE(RequestPermission());
   EXPECT_TRUE(WaitForToken());
-  std::string unique_id = GetUniqueMessageId();
-  const char kNotificationTitle[] = "Topic Test";
-  const char kNotificationBody[] = "Topic Test notification body";
-  // Create a somewhat unique topic name using 2 digits near the end of
-  // unique_id (but not the LAST 2 digits, due to timestamp resolution on some
-  // platforms).
-  std::string unique_id_tag =
-      (unique_id.length() >= 7 ? unique_id.substr(unique_id.length() - 5, 2)
-                               : "00");
-  std::string topic = "FCMTestTopic" + unique_id_tag;
-  EXPECT_TRUE(WaitForCompletion(firebase::messaging::Subscribe(topic.c_str()),
-                                "Subscribe"));
-  SendTestMessage(("/topics/" + topic).c_str(), kNotificationTitle,
-                  kNotificationBody,
-                  {{"message", "Hello, world!"}, {"unique_id", unique_id}});
-  firebase::messaging::Message message;
-  EXPECT_TRUE(WaitForMessage(&message));
-  EXPECT_EQ(message.data["unique_id"], unique_id);
-  if (message.notification) {
-    EXPECT_EQ(message.notification->title, kNotificationTitle);
-    EXPECT_EQ(message.notification->body, kNotificationBody);
-  }
-
-  EXPECT_TRUE(WaitForCompletion(firebase::messaging::Unsubscribe(topic.c_str()),
-                                "Unsubscribe"));
 
-  // Ensure that we *don't* receive a message now.
-  unique_id = GetUniqueMessageId();
-  SendTestMessage(("/topics/" + topic).c_str(), "Topic Title 2", "Topic Body 2",
-                  {{"message", "Hello, world!"}, {"unique_id", unique_id}});
-  EXPECT_FALSE(WaitForMessage(&message, 5));
+  if (!RunFlakyBlock(
+          [](FirebaseMessagingTest* this_) {
+            std::string unique_id = this_->GetUniqueMessageId();
+            const char kNotificationTitle[] = "Topic Test";
+            const char kNotificationBody[] = "Topic Test notification body";
+            // Create a somewhat unique topic name using 2 digits near the end
+            // of unique_id (but not the LAST 2 digits, due to timestamp
+            // resolution on some platforms).
+            std::string unique_id_tag =
+                (unique_id.length() >= 7
+                     ? unique_id.substr(unique_id.length() - 5, 2)
+                     : "00");
+            std::string topic = "FCMTestTopic" + unique_id_tag;
+            firebase::Future<void> sub =
+                firebase::messaging::Subscribe(topic.c_str());
+            FLAKY_WAIT_FOR_COMPLETION(sub, "Subscribe");
+            this_->SendTestMessage(
+                ("/topics/" + topic).c_str(), kNotificationTitle,
+                kNotificationBody,
+                {{"message", "Hello, world!"}, {"unique_id", unique_id}});
+            firebase::messaging::Message message;
+            FLAKY_EXPECT_TRUE(this_->WaitForMessage(&message));
+
+            FLAKY_EXPECT_EQ(message.data["unique_id"], unique_id);
+            if (message.notification) {
+              FLAKY_EXPECT_EQ(message.notification->title, kNotificationTitle);
+              FLAKY_EXPECT_EQ(message.notification->body, kNotificationBody);
+            }
+            firebase::Future<void> unsub =
+                firebase::messaging::Unsubscribe(topic.c_str());
+            FLAKY_WAIT_FOR_COMPLETION(unsub, "Unsubscribe");
+
+            // Ensure that we *don't* receive a message now.
+            unique_id = this_->GetUniqueMessageId();
+            this_->SendTestMessage(
+                ("/topics/" + topic).c_str(), "Topic Title 2", "Topic Body 2",
+                {{"message", "Hello, world!"}, {"unique_id", unique_id}});
+
+            // If this returns true, it means we received a message but
+            // shouldn't have.
+            FLAKY_EXPECT_FALSE(this_->WaitForMessage(&message, 5));
+
+            return true;
+          },
+          this)) {
+    FAIL() << "Test failed, check error log for details.";
+  }
 }
 
 TEST_F(FirebaseMessagingTest, TestChangingListener) {
diff --git a/storage/integration_test/src/integration_test.cc b/storage/integration_test/src/integration_test.cc
@@ -711,10 +711,7 @@ TEST_F(FirebaseStorageTest, TestLargeFilePauseResumeAndDownloadCancel) {
 
             // Ensure the Controller is valid now that we have associated it
             // with an operation.
-            if (!controller.is_valid()) {
-              LogError("Controller is invalid");
-              return false;
-            }
+            FLAKY_EXPECT_TRUE(controller.is_valid());
 
             while (controller.bytes_transferred() == 0) {
 #if FIREBASE_PLATFORM_DESKTOP
@@ -740,34 +737,17 @@ TEST_F(FirebaseStorageTest, TestLargeFilePauseResumeAndDownloadCancel) {
             // The StorageListener's OnPaused will call Resume().
 
             LogDebug("Waiting for future.");
-            WaitForCompletionAnyResult(future, "WriteLargeFile");
-            if (future.error() != firebase::storage::kErrorNone) {
-              LogError("PutBytes returned error %d: %s", future.error(),
-                       future.error_message());
-              return false;
-            }
+            FLAKY_WAIT_FOR_COMPLETION(future, "WriteLargeFile");
             LogDebug("Upload complete.");
 
             // Ensure the various callbacks were called.
-            if (!listener.on_paused_was_called()) {
-              LogError("Listener::OnPaused was not called");
-              return false;
-            }
-            if (!listener.on_progress_was_called()) {
-              LogError("Listener::OnProgress was not called");
-              return false;
-            }
-            if (!listener.resume_succeeded()) {
-              LogError("Resume failed");
-              return false;
-            }
+            FLAKY_EXPECT_TRUE(listener.on_paused_was_called());
+            FLAKY_EXPECT_TRUE(listener.on_progress_was_called());
+            FLAKY_EXPECT_TRUE(listener.resume_succeeded());
 
             auto metadata = future.result();
-            if (metadata->size_bytes() != test_file_size) {
-              LogError(
-                  "Metadata reports incorrect size, file failed to upload.");
-              return false;
-            }
+            // If metadata reports incorrect size, file failed to upload.
+            FLAKY_EXPECT_EQ(metadata->size_bytes(), test_file_size);
             return true;
           },
           &context, "PutBytes")) {
diff --git a/testing/test_framework/src/firebase_test_framework.h b/testing/test_framework/src/firebase_test_framework.h
