Skip to content

Commit 27ed5d8

Browse files
Address review comments: Refine UseUserAccessGroup integration test
This commit incorporates further feedback on the integration test for `UseUserAccessGroup`: - Removed the verbose explanatory comment block from `TestUseUserAccessGroupDoesNotCrash` in `auth/integration_test/src/integration_test.cc`. - Reinstated platform-specific error checking within the test: - On iOS, the test now uses `EXPECT_THAT` to allow either `kAuthErrorNone` or `kAuthErrorKeychainError`. This handles potential keychain configuration issues in test environments while ensuring the call does not crash. - On other platforms (Android/Desktop), the test continues to expect `kAuthErrorNone`, verifying the stub implementation.
1 parent 96b61f1 commit 27ed5d8

File tree

2 files changed

+14
-19
lines changed

2 files changed

+14
-19
lines changed

auth/integration_test/src/integration_test.cc

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,30 +1044,26 @@ TEST_F(FirebaseAuthTest, TestWithCustomEmailAndPassword) {
10441044
}
10451045

10461046
TEST_F(FirebaseAuthTest, TestUseUserAccessGroupDoesNotCrash) {
1047-
// This test primarily ensures that calling UseUserAccessGroup doesn't crash
1048-
// on any platform and that stubs return kAuthErrorNone.
10491047
firebase::auth::AuthError error =
10501048
auth_->UseUserAccessGroup("com.google.firebase.test.accessgroup");
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.
1049+
#if TARGET_OS_IPHONE
1050+
// On iOS, this might return an error if keychain sharing isn't set up
1051+
// for the test app, but it shouldn't crash. We accept kAuthErrorNone or
1052+
// kAuthErrorKeychainError.
1053+
EXPECT_THAT(error, AnyOf(firebase::auth::kAuthErrorNone,
1054+
firebase::auth::kAuthErrorKeychainError));
1055+
#else
1056+
// On other platforms, it should be a no-op and return kAuthErrorNone.
10671057
EXPECT_EQ(error, firebase::auth::kAuthErrorNone);
1058+
#endif
10681059

10691060
error = auth_->UseUserAccessGroup(nullptr);
1061+
#if TARGET_OS_IPHONE
1062+
EXPECT_THAT(error, AnyOf(firebase::auth::kAuthErrorNone,
1063+
firebase::auth::kAuthErrorKeychainError));
1064+
#else
10701065
EXPECT_EQ(error, firebase::auth::kAuthErrorNone);
1066+
#endif
10711067
}
10721068

10731069
TEST_F(FirebaseAuthTest, TestAuthPersistenceWithAnonymousSignin) {

auth/src/include/firebase/auth.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
stray text causing a build error
21
/*
32
* Copyright 2016 Google LLC
43
*

0 commit comments

Comments
 (0)