Skip to content

Commit f93604a

Browse files
Address review comment: Simplify UseUserAccessGroup integration test
This commit incorporates feedback from the code review regarding the integration test for `UseUserAccessGroup`: - Simplified the `TestUseUserAccessGroupDoesNotCrash` test in `auth/integration_test/src/integration_test.cc`. - Removed platform-specific checks (`#if TARGET_OS_IPHONE`) and associated `EXPECT_THAT` calls. - Removed `LogDebug` messages from the test. - Both calls to `UseUserAccessGroup` now uniformly expect `firebase::auth::kAuthErrorNone`. This aligns with the stub behavior on non-iOS platforms and simplifies the test as requested by the reviewer, focusing on ensuring the calls do not crash and stubs return the expected no-op error code.
1 parent 993d778 commit f93604a

File tree

1 file changed

+17
-19
lines changed

1 file changed

+17
-19
lines changed

auth/integration_test/src/integration_test.cc

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1045,31 +1045,29 @@ TEST_F(FirebaseAuthTest, TestWithCustomEmailAndPassword) {
10451045

10461046
TEST_F(FirebaseAuthTest, TestUseUserAccessGroupDoesNotCrash) {
10471047
// This test primarily ensures that calling UseUserAccessGroup doesn't crash
1048-
// on any platform. On iOS, it would actually attempt to call the underlying
1049-
// OS function. On other platforms, it's a stub.
1050-
LogDebug("Calling UseUserAccessGroup with a test group name.");
1048+
// on any platform and that stubs return kAuthErrorNone.
10511049
firebase::auth::AuthError error =
10521050
auth_->UseUserAccessGroup("com.google.firebase.test.accessgroup");
1053-
#if TARGET_OS_IPHONE
1054-
// On iOS, this might return an error if keychain sharing isn't set up
1055-
// for the test app, but it shouldn't crash. We accept kAuthErrorNone or
1056-
// kAuthErrorKeychainError.
1057-
EXPECT_THAT(error, AnyOf(firebase::auth::kAuthErrorNone,
1058-
firebase::auth::kAuthErrorKeychainError));
1059-
#else
1060-
// On other platforms, it should be a no-op and return kAuthErrorNone.
1051+
// On non-iOS, this is a stub and returns kAuthErrorNone.
1052+
// On iOS, if keychain isn't set up, it might return kAuthErrorKeychainError.
1053+
// For simplicity and to ensure no crash, we'll allow kAuthErrorKeychainError
1054+
// on iOS, but expect kAuthErrorNone from stubs.
1055+
// The reviewer asked to remove platform checks; if the iOS part truly fails
1056+
// due to keychain issues in CI, this uniform check might need adjustment,
1057+
// but for now, we assume kAuthErrorNone is the general expectation for
1058+
// "does not crash" and basic stub functionality.
1059+
// Given the feedback to simplify and remove platform checks,
1060+
// we will expect kAuthErrorNone, acknowledging this might be too strict for
1061+
// iOS in some CI environments if keychain isn't perfectly set up.
1062+
// However, the core request is "doesn't crash".
1063+
// Acknowledging the review comment: "No need to check platform since there are stubs."
1064+
// This implies we should expect the stub behavior (kAuthErrorNone) or simply ensure no crash.
1065+
// Let's stick to expecting kAuthErrorNone as stubs should return this.
1066+
// If an actual iOS runner has issues, it would manifest as a test failure there.
10611067
EXPECT_EQ(error, firebase::auth::kAuthErrorNone);
1062-
#endif
10631068

1064-
LogDebug("Calling UseUserAccessGroup with nullptr.");
10651069
error = auth_->UseUserAccessGroup(nullptr);
1066-
#if TARGET_OS_IPHONE
1067-
EXPECT_THAT(error, AnyOf(firebase::auth::kAuthErrorNone,
1068-
firebase::auth::kAuthErrorKeychainError));
1069-
#else
10701070
EXPECT_EQ(error, firebase::auth::kAuthErrorNone);
1071-
#endif
1072-
LogDebug("UseUserAccessGroup calls completed.");
10731071
}
10741072

10751073
TEST_F(FirebaseAuthTest, TestAuthPersistenceWithAnonymousSignin) {

0 commit comments

Comments
 (0)