Skip to content

Commit c02e173

Browse files
jonsimantovjonsimantov
authored
Deflake tests by adding RunWithRetry and changing test timeouts (#281)
Allows you to retry Futures more than once, for handling transient failures. Implemented in Remote Config integration test (for Fetch) and IID integration test (for DeleteId). Also (not related to RunWithRetry), increase message timeout in Messaging test to help avoid flakes, and add a small delay in Storage's pause callback.
1 parent 34f1ad1 commit c02e173

6 files changed

Lines changed: 151 additions & 8 deletions

File tree

‎instance_id/integration_test/src/integration_test.cc‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,14 +152,18 @@ TEST_F(FirebaseInstanceIdTest, TestGettingIdTwiceMatches) {
152152
WaitForCompletion(id, "GetId 2");
153153
EXPECT_EQ(*id.result(), first_id);
154154
}
155-
156155
TEST_F(FirebaseInstanceIdTest, TestDeleteIdGivesNewIdNextTime) {
157156
firebase::Future<std::string> id = instance_id_->GetId();
158157
WaitForCompletion(id, "GetId");
159158
EXPECT_NE(*id.result(), "");
160159
std::string first_id = *id.result();
161160

162-
firebase::Future<void> del = instance_id_->DeleteId();
161+
// Try deleting the IID, but it can sometimes fail due to
162+
// sporadic network issues, so allow retrying.
163+
firebase::Future<void> del =
164+
RunWithRetry<void>([](firebase::instance_id::InstanceId* iid) {
165+
return iid->DeleteId();
166+
}, instance_id_);
163167
WaitForCompletion(del, "DeleteId");
164168

165169
// Ensure that we now get a different IID.

‎messaging/integration_test/src/integration_test.cc‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,8 @@ const char kRestEndpoint[] = "https://fcm.googleapis.com/fcm/send";
5151
const char kNotificationLinkKey[] = "gcm.n.link";
5252
const char kTestLink[] = "https://this-is-a-test-link/";
5353

54-
// Give each operation approximately 60 seconds before failing.
55-
const int kTimeoutSeconds = 60;
54+
// Give each operation approximately 120 seconds before failing.
55+
const int kTimeoutSeconds = 120;
5656
const char kTestingNotificationKey[] = "fcm_testing_notification";
5757

5858
using app_framework::LogDebug;
@@ -546,7 +546,9 @@ TEST_F(FirebaseMessagingTest, TestChangingListener) {
546546
// WaitForMessage() uses whatever shared_listener_ is set to.
547547
shared_listener_ = new firebase::messaging::PollableListener();
548548
firebase::messaging::SetListener(shared_listener_);
549-
549+
// Pause a moment to make sure old listeners are deleted.
550+
ProcessEvents(1000);
551+
550552
std::string unique_id = GetUniqueMessageId();
551553
const char kNotificationTitle[] = "New Listener Test";
552554
const char kNotificationBody[] = "New Listener Test notification body";

‎remote_config/integration_test/src/integration_test.cc‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,9 @@ TEST_F(FirebaseRemoteConfigTest, TestGetAll) {
484484
ASSERT_NE(rc_, nullptr);
485485

486486
EXPECT_TRUE(WaitForCompletion(SetDefaultsV2(rc_), "SetDefaultsV2"));
487-
EXPECT_TRUE(WaitForCompletion(rc_->Fetch(), "Fetch"));
487+
EXPECT_TRUE(WaitForCompletion(RunWithRetry([](RemoteConfig* rc) {
488+
return rc->Fetch();
489+
}, rc_), "Fetch"));
488490
EXPECT_TRUE(WaitForCompletion(rc_->Activate(), "Activate"));
489491
std::map<std::string, firebase::Variant> key_values = rc_->GetAll();
490492
EXPECT_EQ(key_values.size(), 6);
@@ -508,8 +510,9 @@ TEST_F(FirebaseRemoteConfigTest, TestFetchV2) {
508510
ASSERT_NE(rc_, nullptr);
509511

510512
EXPECT_TRUE(WaitForCompletion(SetDefaultsV2(rc_), "SetDefaultsV2"));
511-
512-
EXPECT_TRUE(WaitForCompletion(rc_->Fetch(), "Fetch"));
513+
EXPECT_TRUE(WaitForCompletion(RunWithRetry([](RemoteConfig* rc) {
514+
return rc->Fetch();
515+
}, rc_), "Fetch"));
513516
EXPECT_TRUE(WaitForCompletion(rc_->Activate(), "Activate"));
514517
LogDebug("Fetch time: %lld", rc_->GetInfo().fetch_time);
515518
firebase::remote_config::ValueInfo value_info;

‎storage/integration_test/src/integration_test.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -623,6 +623,8 @@ class StorageListener : public firebase::storage::Listener {
623623

624624
// Tracks whether OnPaused was ever called and resumes the transfer.
625625
void OnPaused(firebase::storage::Controller* controller) override {
626+
// Let things be paused for a moment.
627+
ProcessEvents(1000);
626628
on_paused_was_called_ = true;
627629
controller->Resume();
628630
}

‎testing/test_framework/src/firebase_test_framework.cc‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,65 @@ void FirebaseTest::TerminateApp() {
9696
app_ = nullptr;
9797
}
9898

99+
firebase::FutureBase FirebaseTest::RunWithRetryBase(
100+
firebase::FutureBase (*run_future)(void* context),
101+
void* context, const char* name, int expected_error) {
102+
// Run run_future(context), which returns a Future, then wait for that Future
103+
// to complete. If the Future returns Invalid, or if its error() does
104+
// not match expected_error, pause a moment and try again.
105+
//
106+
// In most cases, this will return the Future once it's been completed.
107+
// However, if it reaches the last attempt, it will return immediately once
108+
// the operation begins. This is because at this point we want to return the
109+
// results whether or not the operation succeeds.
110+
const int kRetryDelaysMs[] = {
111+
// Roughly exponential backoff for the retries.
112+
100, 1000, 5000, 10000, 30000
113+
};
114+
const int kNumAttempts = 1+(sizeof(kRetryDelaysMs) / sizeof(kRetryDelaysMs[0]));
115+
116+
int attempt = 0;
117+
firebase::FutureBase future;
118+
119+
while (attempt < kNumAttempts) {
120+
future = run_future(context);
121+
if (attempt == kNumAttempts-1) {
122+
// This is the last attempt, return immediately.
123+
break;
124+
}
125+
126+
// Wait for completion, then check status and error.
127+
while (future.status() == firebase::kFutureStatusPending) {
128+
app_framework::ProcessEvents(100);
129+
}
130+
if (future.status() != firebase::kFutureStatusComplete) {
131+
app_framework::LogDebug(
132+
"RunWithRetry%s%s: Attempt %d returned invalid status",
133+
*name?" ":"",
134+
name, attempt+1);
135+
}
136+
else if (future.error() != expected_error) {
137+
app_framework::LogDebug(
138+
"RunWithRetry%s%s: Attempt %d returned error %d, expected %d",
139+
*name?" ":"",
140+
name, attempt+1, future.error(), expected_error);
141+
}
142+
else {
143+
// Future is completed and the error matches what's expected, no need to
144+
// retry further.
145+
break;
146+
}
147+
int delay_ms = kRetryDelaysMs[attempt];
148+
app_framework::LogDebug(
149+
"RunWithRetry%s%s: Pause %d milliseconds before retrying.",
150+
*name?" ":"",
151+
name, delay_ms);
152+
app_framework::ProcessEvents(delay_ms);
153+
attempt++;
154+
}
155+
return future;
156+
}
157+
99158
bool FirebaseTest::WaitForCompletion(const firebase::FutureBase& future,
100159
const char* name, int expected_error) {
101160
app_framework::LogDebug("WaitForCompletion %s", name);

‎testing/test_framework/src/firebase_test_framework.h‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,69 @@ class FirebaseTest : public testing::Test {
138138
static bool WaitForCompletion(const firebase::FutureBase& future,
139139
const char* name, int expected_error = 0);
140140

141+
// Run an operation that returns a Future (via a callback), retrying with
142+
// exponential backoff if the operation fails.
143+
//
144+
// Blocks until the operation succeeds (the Future completes, with error
145+
// matching expected_error) or if the final attempt is started (in which case
146+
// the Future returned may still be in progress). You should use
147+
// WaitForCompletion to await the results of this function in any case.
148+
//
149+
// For example, to add retry, you would change:
150+
//
151+
// bool success = WaitForCompletion(
152+
// auth_->DeleteUser(auth->current_user()),
153+
// "DeleteUser");
154+
// To this:
155+
//
156+
// bool success = WaitForCompletion(RunWithRetry(
157+
// [](Auth* auth) {
158+
// return auth->DeleteUser(auth->current_user());
159+
// }, auth_), "DeleteUser"));
160+
template<class CallbackType, class ContextType>
161+
static firebase::FutureBase RunWithRetry(
162+
CallbackType run_future_typed,
163+
ContextType* context_typed,
164+
const char* name = "",
165+
int expected_error = 0) {
166+
struct RunData { CallbackType callback; ContextType* context; };
167+
RunData run_data = { run_future_typed, context_typed };
168+
return RunWithRetryBase(
169+
[](void*ctx) {
170+
CallbackType callback = static_cast<RunData*>(ctx)->callback;
171+
ContextType* context = static_cast<RunData*>(ctx)->context;
172+
return static_cast<firebase::FutureBase>(callback(context));
173+
},
174+
static_cast<void*>(&run_data),name, expected_error);
175+
}
176+
177+
// Same as RunWithRetry, but templated to return a Future<ResultType>
178+
// rather than a FutureBase, in case you want to use the result data
179+
// of the Future. You need to explicitly provide the template parameter,
180+
// e.g. RunWithRetry<int>(..) to return a Future<int>.
181+
template<class ResultType, class CallbackType, class ContextType>
182+
static firebase::Future<ResultType> RunWithRetry(
183+
CallbackType run_future_typed,
184+
ContextType* context_typed,
185+
const char* name = "",
186+
int expected_error = 0) {
187+
struct RunData { CallbackType callback; ContextType* context; };
188+
RunData run_data = { run_future_typed, context_typed };
189+
firebase::FutureBase result_base = RunWithRetryBase(
190+
[](void*ctx) {
191+
CallbackType callback = static_cast<RunData*>(ctx)->callback;
192+
ContextType* context = static_cast<RunData*>(ctx)->context;
193+
// The following line checks that CallbackType actually returns a
194+
// Future<ResultType>. If it returns any other type, the compiler will
195+
// complain about implicit conversion to Future<ResultType> here.
196+
firebase::Future<ResultType> future_result = callback(context);
197+
return static_cast<firebase::FutureBase>(future_result);
198+
},
199+
static_cast<void*>(&run_data),name, expected_error);
200+
// Future<T> and FutureBase are reinterpret_cast-compatible, by design.
201+
return *reinterpret_cast<firebase::Future<ResultType>*>(&result_base);
202+
}
203+
141204
// Blocking HTTP request helper function, for testing only.
142205
static bool SendHttpGetRequest(
143206
const char* url, const std::map<std::string, std::string>& headers,
@@ -161,10 +224,20 @@ class FirebaseTest : public testing::Test {
161224
// false if it failed.
162225
static bool Base64Decode(const std::string& input, std::string* output);
163226

227+
164228
firebase::App* app_;
165229
static int argc_;
166230
static char** argv_;
167231
static bool found_config_;
232+
233+
private:
234+
// Untyped version of RunWithRetry, with implementation.
235+
// This is kept private because the templated version should be used instead,
236+
// for type safety.
237+
static firebase::FutureBase RunWithRetryBase(
238+
firebase::FutureBase (*run_future)(void* context),
239+
void* context,
240+
const char* name, int expected_error);
168241
};
169242

170243
} // namespace firebase_test_framework

0 commit comments

Comments
 (0)