Skip to content

Deflake a few flaky tests #516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jul 8, 2021
Merged

Deflake a few flaky tests #516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6ec335f
Add flaky-retry logic to FirebaseInstallationsTest.TestGettingTokenTw…
jonsimantov Jul 7, 2021
2ce16c9
Deflake messaging test SendMessageToTopic.
jonsimantov Jul 7, 2021
53f79ab
Reformat files.
jonsimantov Jul 7, 2021
0d1f426
Fix bug.
jonsimantov Jul 7, 2021
28a7a18
Fix missing check.
jonsimantov Jul 7, 2021
19ba826
Reformat.
jonsimantov Jul 7, 2021
6077266
Preemptively deflake another test.
jonsimantov Jul 7, 2021
bb3c48c
Format code.
jonsimantov Jul 7, 2021
1720d49
Add FLAKY_EXPECT_* and FLAKY_WAIT_FOR_COMPLETION.
jonsimantov Jul 7, 2021
8ef6dff
Format code.
jonsimantov Jul 7, 2021
ab01860
Rephrase messages.
jonsimantov Jul 7, 2021
3ad69b9
Format code.
jonsimantov Jul 7, 2021
401c258
Slight optimization.
jonsimantov Jul 7, 2021
f8867ea
Fix a bug in a test.
jonsimantov Jul 7, 2021
cacce2b
Fix log whitespace.
jonsimantov Jul 7, 2021
b867958
Format code.
jonsimantov Jul 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 52 additions & 72 deletions installations/integration_test/src/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_)) {
Expand All @@ -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_)) {
Expand All @@ -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) {
Expand Down
87 changes: 56 additions & 31 deletions messaging/integration_test/src/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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_;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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) {
Expand Down
34 changes: 7 additions & 27 deletions storage/integration_test/src/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")) {
Expand Down
Loading