Skip to content

Commit f618b5c

Browse files
jonsimantovjonsimantov
authored
Re-enable WrongInstance test with fixed logic and debug logs (#1521)
* Re-enable WrongInstance test with fixed logic and debug logs * Add additional logging for when Futures complete. * Add network error retry to RequestConsentInfoUpdate tests.
1 parent 091c949 commit f618b5c

File tree

2 files changed

+137
-86
lines changed

2 files changed

+137
-86
lines changed

‎gma/integration_test/src/integration_test.cc

+96-17
Original file line numberDiff line numberDiff line change
@@ -2599,6 +2599,8 @@ TEST_F(FirebaseGmaUmpTest, TestUmpRequestConsentInfoUpdate) {
25992599
using firebase::gma::ump::ConsentRequestParameters;
26002600
using firebase::gma::ump::ConsentStatus;
26012601

2602+
FLAKY_TEST_SECTION_BEGIN();
2603+
26022604
ConsentRequestParameters params;
26032605
params.tag_for_under_age_of_consent = false;
26042606

@@ -2607,7 +2609,13 @@ TEST_F(FirebaseGmaUmpTest, TestUmpRequestConsentInfoUpdate) {
26072609

26082610
EXPECT_TRUE(future == consent_info_->RequestConsentInfoUpdateLastResult());
26092611

2610-
WaitForCompletion(future, "RequestConsentInfoUpdate");
2612+
WaitForCompletion(future, "RequestConsentInfoUpdate",
2613+
{firebase::gma::ump::kConsentRequestSuccess,
2614+
firebase::gma::ump::kConsentRequestErrorNetwork});
2615+
// Retry only network errors.
2616+
EXPECT_NE(future.error(), firebase::gma::ump::kConsentRequestErrorNetwork);
2617+
2618+
FLAKY_TEST_SECTION_END();
26112619

26122620
EXPECT_NE(consent_info_->GetConsentStatus(),
26132621
firebase::gma::ump::kConsentStatusUnknown);
@@ -2622,6 +2630,8 @@ TEST_F(FirebaseGmaUmpTest, TestUmpRequestConsentInfoUpdateDebugEEA) {
26222630
using firebase::gma::ump::ConsentRequestParameters;
26232631
using firebase::gma::ump::ConsentStatus;
26242632

2633+
FLAKY_TEST_SECTION_BEGIN();
2634+
26252635
ConsentRequestParameters params;
26262636
params.tag_for_under_age_of_consent = false;
26272637
params.debug_settings.debug_geography =
@@ -2632,7 +2642,13 @@ TEST_F(FirebaseGmaUmpTest, TestUmpRequestConsentInfoUpdateDebugEEA) {
26322642
firebase::Future<void> future =
26332643
consent_info_->RequestConsentInfoUpdate(params);
26342644

2635-
WaitForCompletion(future, "RequestConsentInfoUpdate");
2645+
WaitForCompletion(future, "RequestConsentInfoUpdate",
2646+
{firebase::gma::ump::kConsentRequestSuccess,
2647+
firebase::gma::ump::kConsentRequestErrorNetwork});
2648+
// Retry only network errors.
2649+
EXPECT_NE(future.error(), firebase::gma::ump::kConsentRequestErrorNetwork);
2650+
2651+
FLAKY_TEST_SECTION_END();
26362652

26372653
EXPECT_EQ(consent_info_->GetConsentStatus(),
26382654
firebase::gma::ump::kConsentStatusRequired);
@@ -2643,6 +2659,8 @@ TEST_F(FirebaseGmaUmpTest, TestUmpRequestConsentInfoUpdateDebugNonEEA) {
26432659
using firebase::gma::ump::ConsentRequestParameters;
26442660
using firebase::gma::ump::ConsentStatus;
26452661

2662+
FLAKY_TEST_SECTION_BEGIN();
2663+
26462664
ConsentRequestParameters params;
26472665
params.tag_for_under_age_of_consent = false;
26482666
params.debug_settings.debug_geography =
@@ -2653,7 +2671,13 @@ TEST_F(FirebaseGmaUmpTest, TestUmpRequestConsentInfoUpdateDebugNonEEA) {
26532671
firebase::Future<void> future =
26542672
consent_info_->RequestConsentInfoUpdate(params);
26552673

2656-
WaitForCompletion(future, "RequestConsentInfoUpdate");
2674+
WaitForCompletion(future, "RequestConsentInfoUpdate",
2675+
{firebase::gma::ump::kConsentRequestSuccess,
2676+
firebase::gma::ump::kConsentRequestErrorNetwork});
2677+
// Retry only network errors.
2678+
EXPECT_NE(future.error(), firebase::gma::ump::kConsentRequestErrorNetwork);
2679+
2680+
FLAKY_TEST_SECTION_END();
26572681

26582682
EXPECT_THAT(consent_info_->GetConsentStatus(),
26592683
AnyOf(Eq(firebase::gma::ump::kConsentStatusNotRequired),
@@ -2747,15 +2771,25 @@ TEST_F(FirebaseGmaUmpTest, TestUmpLoadFormUnderAgeOfConsent) {
27472771
using firebase::gma::ump::ConsentRequestParameters;
27482772
using firebase::gma::ump::ConsentStatus;
27492773

2774+
FLAKY_TEST_SECTION_BEGIN();
2775+
27502776
ConsentRequestParameters params;
27512777
params.tag_for_under_age_of_consent = true;
27522778
params.debug_settings.debug_geography =
27532779
firebase::gma::ump::kConsentDebugGeographyEEA;
27542780
params.debug_settings.debug_device_ids = kTestDeviceIDs;
27552781
params.debug_settings.debug_device_ids.push_back(GetDebugDeviceId());
27562782

2757-
WaitForCompletion(consent_info_->RequestConsentInfoUpdate(params),
2758-
"RequestConsentInfoUpdate");
2783+
firebase::Future<void> future =
2784+
consent_info_->RequestConsentInfoUpdate(params);
2785+
2786+
WaitForCompletion(future, "RequestConsentInfoUpdate",
2787+
{firebase::gma::ump::kConsentRequestSuccess,
2788+
firebase::gma::ump::kConsentRequestErrorNetwork});
2789+
// Retry only network errors.
2790+
EXPECT_NE(future.error(), firebase::gma::ump::kConsentRequestErrorNetwork);
2791+
2792+
FLAKY_TEST_SECTION_END();
27592793

27602794
firebase::Future<void> load_future = consent_info_->LoadConsentForm();
27612795
WaitForCompletion(load_future, "LoadConsentForm",
@@ -2770,15 +2804,25 @@ TEST_F(FirebaseGmaUmpTest, TestUmpLoadFormUnavailableDebugNonEEA) {
27702804
using firebase::gma::ump::ConsentRequestParameters;
27712805
using firebase::gma::ump::ConsentStatus;
27722806

2807+
FLAKY_TEST_SECTION_BEGIN();
2808+
27732809
ConsentRequestParameters params;
27742810
params.tag_for_under_age_of_consent = false;
27752811
params.debug_settings.debug_geography =
27762812
firebase::gma::ump::kConsentDebugGeographyNonEEA;
27772813
params.debug_settings.debug_device_ids = kTestDeviceIDs;
27782814
params.debug_settings.debug_device_ids.push_back(GetDebugDeviceId());
27792815

2780-
WaitForCompletion(consent_info_->RequestConsentInfoUpdate(params),
2781-
"RequestConsentInfoUpdate");
2816+
firebase::Future<void> future =
2817+
consent_info_->RequestConsentInfoUpdate(params);
2818+
2819+
WaitForCompletion(future, "RequestConsentInfoUpdate",
2820+
{firebase::gma::ump::kConsentRequestSuccess,
2821+
firebase::gma::ump::kConsentRequestErrorNetwork});
2822+
// Retry only network errors.
2823+
EXPECT_NE(future.error(), firebase::gma::ump::kConsentRequestErrorNetwork);
2824+
2825+
FLAKY_TEST_SECTION_END();
27822826

27832827
if (consent_info_->GetConsentStatus() !=
27842828
firebase::gma::ump::kConsentStatusRequired) {
@@ -2792,15 +2836,25 @@ TEST_F(FirebaseGmaUmpTest, TestUmpLoadAndShowIfRequiredDebugNonEEA) {
27922836
using firebase::gma::ump::ConsentRequestParameters;
27932837
using firebase::gma::ump::ConsentStatus;
27942838

2839+
FLAKY_TEST_SECTION_BEGIN();
2840+
27952841
ConsentRequestParameters params;
27962842
params.tag_for_under_age_of_consent = false;
27972843
params.debug_settings.debug_geography =
27982844
firebase::gma::ump::kConsentDebugGeographyNonEEA;
27992845
params.debug_settings.debug_device_ids = kTestDeviceIDs;
28002846
params.debug_settings.debug_device_ids.push_back(GetDebugDeviceId());
28012847

2802-
WaitForCompletion(consent_info_->RequestConsentInfoUpdate(params),
2803-
"RequestConsentInfoUpdate");
2848+
firebase::Future<void> future =
2849+
consent_info_->RequestConsentInfoUpdate(params);
2850+
2851+
WaitForCompletion(future, "RequestConsentInfoUpdate",
2852+
{firebase::gma::ump::kConsentRequestSuccess,
2853+
firebase::gma::ump::kConsentRequestErrorNetwork});
2854+
// Retry only network errors.
2855+
EXPECT_NE(future.error(), firebase::gma::ump::kConsentRequestErrorNetwork);
2856+
2857+
FLAKY_TEST_SECTION_END();
28042858

28052859
EXPECT_THAT(consent_info_->GetConsentStatus(),
28062860
AnyOf(Eq(firebase::gma::ump::kConsentStatusNotRequired),
@@ -3024,7 +3078,7 @@ TEST_F(FirebaseGmaUmpTest, TestUmpCleanupRaceCondition) {
30243078
ProcessEvents(5000);
30253079
}
30263080

3027-
TEST_F(FirebaseGmaUmpTest, TestUmpCallbacksOnWrongInstance_DISABLED) {
3081+
TEST_F(FirebaseGmaUmpTest, TestUmpCallbacksOnWrongInstance) {
30283082
// Ensure that if ConsentInfo is deleted and then recreated, stale
30293083
// callbacks don't call into the new instance and cause crashes.
30303084
using firebase::gma::ump::ConsentFormStatus;
@@ -3038,23 +3092,46 @@ TEST_F(FirebaseGmaUmpTest, TestUmpCallbacksOnWrongInstance_DISABLED) {
30383092
params.debug_settings.debug_device_ids = kTestDeviceIDs;
30393093
params.debug_settings.debug_device_ids.push_back(GetDebugDeviceId());
30403094

3041-
consent_info_->RequestConsentInfoUpdate(params);
3042-
consent_info_->LoadConsentForm();
3095+
LogDebug("RequestConsentInfoUpdate");
3096+
consent_info_->RequestConsentInfoUpdate(params).OnCompletion(
3097+
[](const firebase::Future<void>&) {
3098+
LogDebug("RequestConsentInfoUpdate done");
3099+
});
3100+
LogDebug("LoadConsentForm");
3101+
consent_info_->LoadConsentForm().OnCompletion(
3102+
[](const firebase::Future<void>&) { LogDebug("LoadConsentForm done"); });
30433103
// In automated tests, only check RequestConsentInfoUpdate and LoadConsentForm
30443104
// as the rest may show UI.
30453105
if (ShouldRunUITests()) {
3046-
consent_info_->ShowConsentForm(app_framework::GetWindowController());
3047-
consent_info_->LoadAndShowConsentFormIfRequired(
3048-
app_framework::GetWindowController());
3049-
consent_info_->ShowPrivacyOptionsForm(app_framework::GetWindowController());
3106+
LogDebug("ShowConsentForm");
3107+
consent_info_->ShowConsentForm(app_framework::GetWindowController())
3108+
.OnCompletion([](const firebase::Future<void>&) {
3109+
LogDebug("ShowConsentForm done");
3110+
});
3111+
LogDebug("LoadAndShowConsentFormIfRequired");
3112+
consent_info_
3113+
->LoadAndShowConsentFormIfRequired(app_framework::GetWindowController())
3114+
.OnCompletion([](const firebase::Future<void>&) {
3115+
LogDebug("LoadAndShowConsentFormIfRequired done");
3116+
});
3117+
LogDebug("ShowPrivacyOptionsForm");
3118+
consent_info_->ShowPrivacyOptionsForm(app_framework::GetWindowController())
3119+
.OnCompletion([](const firebase::Future<void>&) {
3120+
LogDebug("ShowPrivacyOptionsForm done");
3121+
});
30503122
}
30513123

3124+
LogDebug("Terminate");
30523125
TerminateUmp(kNoReset);
30533126

3127+
LogDebug("Initialize");
30543128
InitializeUmp(kNoReset);
30553129

30563130
// Give the operations time to complete.
3131+
LogDebug("Wait");
30573132
ProcessEvents(5000);
3133+
3134+
LogDebug("Done");
30583135
}
30593136

30603137
TEST_F(FirebaseGmaUmpTest, TestUmpMethodsReturnOperationInProgress) {
@@ -3086,7 +3163,9 @@ TEST_F(FirebaseGmaUmpTest, TestUmpMethodsReturnOperationInProgress) {
30863163
WaitForCompletion(
30873164
future_request_2, "RequestConsentInfoUpdate second",
30883165
firebase::gma::ump::kConsentRequestErrorOperationInProgress);
3089-
WaitForCompletion(future_request_1, "RequestConsentInfoUpdate first");
3166+
WaitForCompletion(future_request_1, "RequestConsentInfoUpdate first",
3167+
{firebase::gma::ump::kConsentRequestSuccess,
3168+
firebase::gma::ump::kConsentRequestErrorNetwork});
30903169

30913170
consent_info_->Reset();
30923171

‎gma/src/ios/ump/consent_info_internal_ios.mm

+41-69
Original file line numberDiff line numberDiff line change
@@ -125,24 +125,19 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) {
125125
callback_instance_tag = s_instance_tag;
126126

127127
util::DispatchAsyncSafeMainQueue(^{
128-
{
129-
MutexLock lock(s_instance_mutex);
130-
if (!s_instance || s_instance_tag != callback_instance_tag) {
131-
// Instance changed or was invalidated, don't call the iOS method any more.
132-
return;
133-
}
128+
MutexLock lock(s_instance_mutex);
129+
if (!s_instance || s_instance_tag != callback_instance_tag) {
130+
// Instance changed or was invalidated, don't call the iOS method any more.
131+
return;
134132
}
135133
[UMPConsentInformation.sharedInstance
136134
requestConsentInfoUpdateWithParameters:ios_parameters
137135
completionHandler:^(NSError* _Nullable error) {
138-
if (!error) {
139-
MutexLock lock(s_instance_mutex);
140-
if (s_instance && s_instance_tag == callback_instance_tag) {
136+
MutexLock lock(s_instance_mutex);
137+
if (s_instance && s_instance_tag == callback_instance_tag) {
138+
if (!error) {
141139
CompleteFuture(handle, kConsentRequestSuccess);
142-
}
143-
} else {
144-
MutexLock lock(s_instance_mutex);
145-
if (s_instance && s_instance_tag == callback_instance_tag) {
140+
} else {
146141
CompleteFuture(handle,
147142
CppRequestErrorFromIosRequestError(error.code),
148143
error.localizedDescription.UTF8String);
@@ -203,30 +198,22 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) {
203198
callback_instance_tag = s_instance_tag;
204199

205200
util::DispatchAsyncSafeMainQueue(^{
206-
{
207-
MutexLock lock(s_instance_mutex);
208-
if (!s_instance || s_instance_tag != callback_instance_tag) {
209-
// Instance changed or was invalidated, don't call the iOS method any more.
210-
return;
211-
}
201+
MutexLock lock(s_instance_mutex);
202+
if (!s_instance || s_instance_tag != callback_instance_tag) {
203+
// Instance changed or was invalidated, don't call the iOS method any more.
204+
return;
212205
}
213206
[UMPConsentForm
214207
loadWithCompletionHandler:^(UMPConsentForm* _Nullable form, NSError* _Nullable error) {
215-
if (form) {
216-
MutexLock lock(s_instance_mutex);
217-
if (s_instance && s_instance_tag == callback_instance_tag) {
208+
MutexLock lock(s_instance_mutex);
209+
if (s_instance && s_instance_tag == callback_instance_tag) {
210+
if (form) {
218211
SetLoadedForm(form);
219212
CompleteFuture(handle, kConsentFormSuccess, "Success");
220-
}
221-
} else if (error) {
222-
MutexLock lock(s_instance_mutex);
223-
if (s_instance && s_instance_tag == callback_instance_tag) {
213+
} else if (error) {
224214
CompleteFuture(handle, CppFormErrorFromIosFormError(error.code),
225215
error.localizedDescription.UTF8String);
226-
}
227-
} else {
228-
MutexLock lock(s_instance_mutex);
229-
if (s_instance && s_instance_tag == callback_instance_tag) {
216+
} else {
230217
CompleteFuture(handle, kConsentFormErrorUnknown, "An unknown error occurred.");
231218
}
232219
}
@@ -254,23 +241,18 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) {
254241
callback_instance_tag = s_instance_tag;
255242

256243
util::DispatchAsyncSafeMainQueue(^{
257-
{
258-
MutexLock lock(s_instance_mutex);
259-
if (!s_instance || s_instance_tag != callback_instance_tag) {
260-
// Instance changed or was invalidated, don't call the iOS method any more.
261-
return;
262-
}
244+
MutexLock lock(s_instance_mutex);
245+
if (!s_instance || s_instance_tag != callback_instance_tag) {
246+
// Instance changed or was invalidated, don't call the iOS method any more.
247+
return;
263248
}
264249
[loaded_form_ presentFromViewController:parent
265250
completionHandler:^(NSError* _Nullable error) {
266-
if (!error) {
267-
MutexLock lock(s_instance_mutex);
268-
if (s_instance && s_instance_tag == callback_instance_tag) {
251+
MutexLock lock(s_instance_mutex);
252+
if (s_instance && s_instance_tag == callback_instance_tag) {
253+
if (!error) {
269254
CompleteFuture(handle, kConsentRequestSuccess);
270-
}
271-
} else {
272-
MutexLock lock(s_instance_mutex);
273-
if (s_instance && s_instance_tag == callback_instance_tag) {
255+
} else {
274256
CompleteFuture(handle, CppFormErrorFromIosFormError(error.code),
275257
error.localizedDescription.UTF8String);
276258
}
@@ -297,24 +279,19 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) {
297279
callback_instance_tag = s_instance_tag;
298280

299281
util::DispatchAsyncSafeMainQueue(^{
300-
{
301-
MutexLock lock(s_instance_mutex);
302-
if (!s_instance || s_instance_tag != callback_instance_tag) {
303-
// Instance changed or was invalidated, don't call the iOS method any more.
304-
return;
305-
}
282+
MutexLock lock(s_instance_mutex);
283+
if (!s_instance || s_instance_tag != callback_instance_tag) {
284+
// Instance changed or was invalidated, don't call the iOS method any more.
285+
return;
306286
}
307287
[UMPConsentForm
308288
loadAndPresentIfRequiredFromViewController:parent
309289
completionHandler:^(NSError* _Nullable error) {
310-
if (!error) {
311-
MutexLock lock(s_instance_mutex);
312-
if (s_instance && s_instance_tag == callback_instance_tag) {
290+
MutexLock lock(s_instance_mutex);
291+
if (s_instance && s_instance_tag == callback_instance_tag) {
292+
if (!error) {
313293
CompleteFuture(handle, kConsentRequestSuccess);
314-
}
315-
} else {
316-
MutexLock lock(s_instance_mutex);
317-
if (s_instance && s_instance_tag == callback_instance_tag) {
294+
} else {
318295
CompleteFuture(handle,
319296
CppFormErrorFromIosFormError(error.code),
320297
error.localizedDescription.UTF8String);
@@ -357,24 +334,19 @@ static ConsentFormError CppFormErrorFromIosFormError(NSInteger code) {
357334
callback_instance_tag = s_instance_tag;
358335

359336
util::DispatchAsyncSafeMainQueue(^{
360-
{
361-
MutexLock lock(s_instance_mutex);
362-
if (!s_instance || s_instance_tag != callback_instance_tag) {
363-
// Instance changed or was invalidated, don't call the iOS method any more.
364-
return;
365-
}
337+
MutexLock lock(s_instance_mutex);
338+
if (!s_instance || s_instance_tag != callback_instance_tag) {
339+
// Instance changed or was invalidated, don't call the iOS method any more.
340+
return;
366341
}
367342
[UMPConsentForm
368343
presentPrivacyOptionsFormFromViewController:parent
369344
completionHandler:^(NSError* _Nullable error) {
370-
if (!error) {
371-
MutexLock lock(s_instance_mutex);
372-
if (s_instance && s_instance_tag == callback_instance_tag) {
345+
MutexLock lock(s_instance_mutex);
346+
if (s_instance && s_instance_tag == callback_instance_tag) {
347+
if (!error) {
373348
CompleteFuture(handle, kConsentRequestSuccess);
374-
}
375-
} else {
376-
MutexLock lock(s_instance_mutex);
377-
if (s_instance && s_instance_tag == callback_instance_tag) {
349+
} else {
378350
CompleteFuture(handle,
379351
CppFormErrorFromIosFormError(error.code),
380352
error.localizedDescription.UTF8String);

0 commit comments

Comments
 (0)