[google_sign_in] Increase iOS coverage tests - #12484
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive suite of Swift unit tests for the iOS Google Sign-In plugin, covering URL handling, error mapping, disconnect scenarios, user data parsing, and top view controller resolution. The review feedback suggests passing nil instead of a scene instance in the sceneOpenURLContexts test to prevent potential test failures in headless CI environments where connected scenes may be empty.
| @Test func sceneOpenURLContexts() throws { | ||
| let (plugin, fakeSignIn) = createTestPlugin() | ||
| let url = URL(string: "com.googleusercontent.apps.test:/oauthredirect")! | ||
| let scene = try #require(UIApplication.shared.connectedScenes.first) | ||
| let fakeContext = FakeOpenURLContext(url: url) | ||
|
|
||
| plugin.perform( | ||
| NSSelectorFromString("scene:openURLContexts:"), | ||
| with: scene, | ||
| with: NSSet(object: fakeContext)) | ||
|
|
||
| #expect(fakeSignIn.handledURLs == [url]) | ||
| } |
There was a problem hiding this comment.
In headless unit test environments (such as CI runners), UIApplication.shared.connectedScenes can be empty, which causes try #require(UIApplication.shared.connectedScenes.first) to throw and fail the test. Since the scene parameter is completely unused in the plugin's implementation of scene:openURLContexts:, we can pass nil instead and make the test more robust and independent of the application's scene state.
@Test func sceneOpenURLContexts() {
let (plugin, fakeSignIn) = createTestPlugin()
let url = URL(string: "com.googleusercontent.apps.test:/oauthredirect")!
let fakeContext = FakeOpenURLContext(url: url)
plugin.perform(
NSSelectorFromString("scene:openURLContexts:"),
with: nil,
with: NSSet(object: fakeContext))
#expect(fakeSignIn.handledURLs == [url])
}There was a problem hiding this comment.
please address gemini comments. It will make review easier.
Add URL handling, error mapping, disconnect, and topViewController cases so FLTGoogleSignInPlugin.m is covered before the Obj-C to Swift migration.
e37d0b6 to
2dccfa6
Compare
allowSignInPermissions looked up the system permission alert's confirmation button by the literal title "Continue", so testSignInPopUp failed on any simulator whose language is not English; the button is titled in the simulator's language (for example "Continuar" in Spanish). Tap the last of the alert's buttons instead, which is the confirmation button in every localization.
167fe18 to
59fbe19
Compare
hellohuanlin
left a comment
There was a problem hiding this comment.
Overall looks good. Just some nits.
| let fakeContext = FakeOpenURLContext(url: url) | ||
|
|
||
| plugin.perform( | ||
| NSSelectorFromString("scene:openURLContexts:"), |
There was a problem hiding this comment.
This loses compilation safety. let's avoid using objc hacks
There was a problem hiding this comment.
Done! Dropped scene:openURLContexts: because UIOpenURLContext has no public initializer, so the only way to call it was performSelector. Left a TODO to add a typed test after the Obj-C → Swift migration (flutter/flutter#119103).
| @Test func sceneOpenURLContexts() throws { | ||
| let (plugin, fakeSignIn) = createTestPlugin() | ||
| let url = URL(string: "com.googleusercontent.apps.test:/oauthredirect")! | ||
| let scene = try #require(UIApplication.shared.connectedScenes.first) | ||
| let fakeContext = FakeOpenURLContext(url: url) | ||
|
|
||
| plugin.perform( | ||
| NSSelectorFromString("scene:openURLContexts:"), | ||
| with: scene, | ||
| with: NSSet(object: fakeContext)) | ||
|
|
||
| #expect(fakeSignIn.handledURLs == [url]) | ||
| } |
There was a problem hiding this comment.
please address gemini comments. It will make review easier.
…swift, with a note to re-add after Obj-C plugin migration to Swift.
| // the Obj-C plugin is migrated to Swift. The test was dropped because | ||
| // UIOpenURLContext has no public initializer, so invoking the Obj-C | ||
| // method required performSelector. See | ||
| // https://github.com/flutter/flutter/issues/119103 |
There was a problem hiding this comment.
This issue doesn't describe the problem that you are describing above though
There was a problem hiding this comment.
You’re right, #119103 is the Swift migration tracker, not this test gap. I linked it because that is the issue we are using for this migration effort, and the typed scene:openURLContexts: test is meant to come back as part of that work. Should I drop the #119103 link from the TODO?
|
|
||
| // TODO(victogomez-cs): Re-add a typed scene:openURLContexts: test after | ||
| // the Obj-C plugin is migrated to Swift. The test was dropped because | ||
| // UIOpenURLContext has no public initializer, so invoking the Obj-C |
There was a problem hiding this comment.
How is the previous objc hack related to UIOpenURLContext's public initializer? Can you add more info?
There was a problem hiding this comment.
A typed call needs NSSet<UIOpenURLContext *>. UIKit marks UIOpenURLContext init/new unavailable, so tests cannot build a real context. The old test used performSelector with an NSObject stand-in, which isn’t type-checked. I expanded the TODO to spell that out, still pointing at #119103 as the migration tracker
…d unit test for scene:openURLContexts:. The previous test was removed due to limitations in constructing UIOpenURLContext in Swift.
Adds native unit tests for previously untested paths in
FLTGoogleSignInPlugin.mbefore the Objective-C → Swift migration. Production code is unchanged.This is a tests-only change, so it does not bump the package version or CHANGELOG.
Native unit tests go from 21 tests / 6 suites to 31 tests / 11 suites.
New test cases
urlHandling(iOS / Mac Catalyst only) — these APIs do not exist on macOS (handleOpenURLs:is the macOS path):applicationOpenURL—application:openURL:options:forwards the URL to GID and returnstrueapplicationOpenURLReturnsHandleResult— same path returnsfalsewhen GID doessceneOpenURLContexts—scene:openURLContexts:forwards the URL from aUIOpenURLContextstand-inerrorMapping:mapsRemainingGIDSignInErrorCodes(parameterized):GIDSignInError.keychain→FSIGoogleSignInErrorCode.keychainErrorGIDSignInError.EMM→FSIGoogleSignInErrorCode.eemErrorGIDSignInError.unknown→FSIGoogleSignInErrorCode.unknown12345) →FSIGoogleSignInErrorCode.unknownsanitizesComplexUserInfoInFlutterError—FSISanitizedUserInfokeeps strings/numbers/URLs/arrays/dicts/nested errors and stringifies unsupported types (e.g.Date)disconnect:disconnectReturnsFlutterErrorOnFailure— a GID disconnect failure is returned as a Flutter error with domain/code and userInfouserData:signInWithoutProfileImageOmitsPhotoUrl— a user with no profile photo getsphotoUrl == niland still mapsdisplayNametopViewController(iOS / Mac Catalyst only) — macOS presents withNSWindow, not aUIViewControllerhierarchy:usesNavigationControllerVisibleController— presents from the nav stack’s top VCusesTabBarControllerSelectedController— presents from the selected tab VCusesPresentedViewController— presents from a presented VCAlso records URLs passed to
TestSignIn.handle(_:)so the URL-handling cases can assert the fake was called.First part of Swift migration to backfill code coverage for flutter/flutter#119103
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2