[google_sign_in] Remove use of OCMock #10290

stuartmorgan-g · 2025-10-24T14:05:44Z

Wraps Google Sign In SDK classes with protocols, so that we can inject fakes that implement the protocols instead of using OCMock to mock the actual classes. This will allow conversion of the tests to Swift, in preparation for migrating the entire plugin to Swift.

For now the protocols are implemented in production with passthrough wrappers, so this adds a fair amount of boilerplate code to the plugin, but almost all of it will be removed when the plugin implementation is converted to Swift since we will be able to use extensions to make the SDK classes conform to the protocol instead of maintaining wrappers.

Unblocks flutter/flutter#119103

Pre-Review Checklist

Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

gemini-code-assist

Code Review

This pull request is a significant and well-executed refactoring to remove the OCMock dependency from the iOS Google Sign-In plugin tests. By introducing a protocol-based abstraction layer for the Google Sign-In SDK, you've greatly improved the testability of the native code, which is a crucial step for the planned migration to Swift. The use of wrapper classes for production and hand-crafted fakes for testing is a clean and effective pattern. The changes are extensive but logical and consistent throughout the codebase. I have a few suggestions to further improve the code's robustness and adherence to Objective-C best practices.

packages/google_sign_in/google_sign_in_ios/darwin/Tests/GoogleSignInTests.m

gemini-code-assist · 2025-10-24T14:09:16Z

packages/google_sign_in/google_sign_in_ios/darwin/Tests/GoogleSignInTests.m

+@property(strong, nonatomic) NSString *email;
+@property(strong, nonatomic) NSString *name;
+/// A URL to return from imageURLWithDimension:.
+@property(strong, nonatomic) NSURL *imageURL;


Properties for objects that have mutable counterparts (like NSString, NSArray, NSDictionary, NSURL, NSDate) should be declared with copy instead of strong. This prevents the internal state of the object from being changed unexpectedly by another object that holds a reference to a mutable version. This is a standard Objective-C best practice.

This applies to email, name, and imageURL here, and also to properties in other test fake classes:

TestToken: tokenString, expirationDate

TestSignInResult: serverAuthCode

TestGoogleUser: userID, grantedScopes, requestedScopes

Suggested change

@property(strong, nonatomic) NSString *email;

@property(strong, nonatomic) NSString *name;

/// A URL to return from imageURLWithDimension:.

@property(strong, nonatomic) NSURL *imageURL;

@property(copy, nonatomic) NSString *email;

@property(copy, nonatomic) NSString *name;

/// A URL to return from imageURLWithDimension:.

@property(copy, nonatomic) NSURL *imageURL;

Unfortunately the compiler gets angry if you re-declare a public readonly property as copy, even if it's specifically being changed to readwrite. That seems like a bug, but we're stuck with it.

I don't want to make the protocols use copy because they are duplicated exactly from the SDK, in anticipation of the switch to Swift class extensions to add protocol conformance.

(I could instead make these ivars, add manual implementations of the read-only properties, and declare an initWith... to set them all, but that's a bunch more test code to fix something that should not actually matter in tests.)

...gle_sign_in_ios/darwin/google_sign_in_ios/Sources/google_sign_in_ios/FLTGoogleSignInPlugin.m

stuartmorgan-g · 2025-10-24T14:09:10Z

...sign_in_ios/Sources/google_sign_in_ios/include/google_sign_in_ios/FSIGoogleSignInProtocols.h

+/// to GIDSignIn.
+@protocol FSIGIDSignIn <NSObject>
+
+@property(nonatomic, nullable) GIDConfiguration *configuration;


The reason this uses GIDConfiguration directly, instead of introducing an FSIGIDConfiguration protocol like everything else, is that for whatever reason the SDK exposes this as a data class with public initializers, unlike the other classes that look data-class-y (GIDSignInResult, GIDToken, etc.) which don't have public initializers.

Since we can make instances of this class with arbitrary data in tests, we don't need to wrap it to allow for that.

stuartmorgan-g · 2025-10-24T14:11:17Z

...gle_sign_in_ios/darwin/google_sign_in_ios/Sources/google_sign_in_ios/FLTGoogleSignInPlugin.m

-  // https://github.com/flutter/flutter/issues/104117
-  return [self topViewControllerFromViewController:[UIApplication sharedApplication]
-                                                       .keyWindow.rootViewController];
-#pragma clang diagnostic pop


I went ahead and moved this code into the view provider abstraction, since that's where the non-deprecated replacement codepath will live, it's what I have already done in at least one other plugin where I introduced a view provider, and it means we could write tests for code that calls this without hitting issues with trying to use the actual UIApplication in a unit test.

stuartmorgan-g · 2025-10-24T14:12:28Z

packages/google_sign_in/google_sign_in_ios/darwin/Tests/GoogleSignInTests.m

-  OCMStub(self.mockPluginRegistrar.messenger).andReturn(self.mockBinaryMessenger);
-  self.plugin = [[FLTGoogleSignInPlugin alloc] initWithSignIn:mockSignIn
-                                                    registrar:self.mockPluginRegistrar];
-  [FLTGoogleSignInPlugin registerWithRegistrar:self.mockPluginRegistrar];


I have no idea why this line was here; it was making an instance without any test DI that we were never using in tests. That's generally not something we want to do in tests, so I removed it while I was adjusting the DI around it.

stuartmorgan-g · 2025-10-24T14:21:40Z

packages/google_sign_in/google_sign_in_ios/darwin/Tests/GoogleSignInTests.m

  XCTAssertNil(error);
+  // No configuration should be set, allowing the SDK to use its default behavior
+  // (which is to load configuration information from Info.plist).
+  XCTAssertNil(self.fakeSignIn.configuration);


This is the only place where an existing test was changed in a way that wasn't just adapting the OCMock logic to the new approach. That's because this looked like an actual test, but it wasn't: the OCMExpect call was never actually triggered, so none of the XCTAsserts were being executed. Reading from the Info.plist is logic that is in the SDK, not in the plugin. That's why line 186 was making a different plugin instance that didn't inject a mock Google Sign In instance... but that also meant that an expectation on self.mockSignIn never fired.

So basically this test was just testing that the SDK didn't crash, not that it actually had the expected behavior. I considered fixing this to read the values from the actual config, but having a plugin unit test that is asserting SDK behavior rather than plugin behavior is weird, so I changed it to instead assert the plugin behavior we want, which is to not set a config so that the SDK uses the one it creates by default from the Info.plist.

stuartmorgan-g · 2025-10-24T14:22:26Z

packages/google_sign_in/google_sign_in_ios/darwin/Tests/GoogleSignInTests.m


 #pragma mark - addScopes

+- (void)testRequestScopesPassesScopes {


Mostly I didn't do test backfill in this PR, but I noticed we had a lot of tests of error conditions for addScopes:... but nothing that tested that we actually requested the right scopes, so added that quickly while I was here.

...gle_sign_in_ios/darwin/google_sign_in_ios/Sources/google_sign_in_ios/FLTGoogleSignInPlugin.m

LongCatIsLooong

Would it be possible to make the new header files private, or are they intended to be public so people can fake GID objects themselves?

LongCatIsLooong · 2025-11-03T18:21:38Z

...in_ios/darwin/google_sign_in_ios/Sources/google_sign_in_ios/WrapperProtocolImplementations.m

+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+  // TODO(stuartmorgan) Provide a non-deprecated codepath. See
+  // https://github.com/flutter/flutter/issues/104117
+  return UIApplication.sharedApplication.keyWindow.rootViewController;


I thought we added a view controller getter on the iOS registrar. Because that change hasn't landed in stable?

Correct, it's not in stable yet.

LongCatIsLooong · 2025-11-03T19:04:36Z

...n_ios/Sources/google_sign_in_ios/include/google_sign_in_ios/WrapperProtocolImplementations.h

+
+NS_ASSUME_NONNULL_BEGIN
+
+// TODO(stuarmorgan): Replace these with protocol extensions when migrating to Swift.


Maybe I'm reading this wrong but I thought in objective-c protocols can't have default implementations? For example if FSIViewProvider needs to be exposed to objective-c then each conformer has to implement all the required properties and methods manually (instead of getting them for free via protocol extensions).

if FSIViewProvider needs to be exposed to objective-c

It doesn't, it's an internal implementation detail. It just doesn't look that way because of the issues mentioned in my other comment. When we migrate to Swift we'll make all of it Swift. There is nothing in google_sign_in's native code that we actually need to expose publicly.

stuartmorgan-g · 2025-11-03T19:23:31Z

Would it be possible to make the new header files private, or are they intended to be public so people can fake GID objects themselves?

We've had a bunch of issues in the past with trying to make Obj-C headers private and how that interacts with modules and umbrella headers. In general our solution has been to just ignore it; in practice essentially nobody uses any of the native headers anyway, so it's largely moot. And the problem will go away as we migrate to Swift, so the incentive to put more effort into trying to massage the built into working with private headers is low.

stuartmorgan-g · 2025-11-03T19:34:01Z

We've had a bunch of issues in the past with trying to make Obj-C headers private and how that interacts with modules and umbrella headers.

Ah, and to helpfully remind me what this looks like, CI is failing because I didn't expose one of these internal headers enough:

xcodebuild: Headers/Private/google_sign_in_ios/google_sign_in_ios-umbrella.h:10:1: warning: umbrella header for module 'google_sign_in_ios' does not include header 'WrapperProtocolImplementations.h' [-Wincomplete-umbrella]

flutter/packages@1a7075b...3d926aa 2025-11-04 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.squareup.okhttp3:okhttp from 5.1.0 to 5.3.0 in /packages/espresso/android (flutter/packages#10348) 2025-11-04 engine-flutter-autoroll@skia.org Roll Flutter from 6f8abdd to 027f2e4 (26 revisions) (flutter/packages#10335) 2025-11-03 stuartmorgan@google.com [google_sign_in] Remove use of OCMock (flutter/packages#10290) 2025-11-03 stuartmorgan@google.com [interactive_media_ads] Pin iOS dependency maximum (flutter/packages#10349) 2025-10-30 magder@google.com [webview_flutter_wkwebview] Remove specialization of 'map' to fix Swift warning (flutter/packages#9810) 2025-10-30 engine-flutter-autoroll@skia.org Roll Flutter from df72035 to 6f8abdd (16 revisions) (flutter/packages#10327) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

* 'main' of https://github.com/CaoGiaHieu-dev/packages: Update packages/go_router/CHANGELOG.md [tool] Remove use of FETCH_HEAD (flutter#10357) Roll Flutter from 027f2e410241 to e5d5c01850f2 (73 revisions) (flutter#10362) [camera_platform_interface] Adds support for video stabilization to camera_platform_interface (flutter#10337) [google_maps_flutter] Raise `MapUsedAfterWidgetDisposedError` when map controller used after map disposed (flutter#9242) [pigeon] Replace containsKey with contains in Kotlin generator (flutter#10274) [video_player] Remove `package` in example `AndroidManifest.xml` file (flutter#10245) [flutter_svg] Fixes typo of `allowDrawingOutsideViewBox` in doc comments. (flutter#10256) [in_app_purchase] Remove use of Pigeon's Dart test generator (flutter#10328) [dependabot]: Bump com.squareup.okhttp3:okhttp from 5.1.0 to 5.3.0 in /packages/espresso/android (flutter#10348) Roll Flutter from 6f8abdd77820 to 027f2e410241 (26 revisions) (flutter#10335) [google_sign_in] Remove use of OCMock (flutter#10290) [interactive_media_ads] Pin iOS dependency maximum (flutter#10349)

stuartmorgan-g added 14 commits October 22, 2025 14:57

Initial GIDSignIn wrapper

6749e92

Implement the passthrough wrapper

a5f4da5

Switch to using the wrapper

d39fa4e

Make a test stub

a080384

Update unit tests, fix issues

c236ab4

Add wrappers for the rest of the GID types

e241969

Update tests, and add a missing test

f1017aa

Remove some unnecessary mock setup

c9f3b09

Add view provider protocol, remove OCMock

1acc901

Add missing configuration passthrough

6159304

macOS test compilation

29f6178

Remove OCMock from macOS project

10f9367

Version bump

005533e

Revert SwiftPM files

c3d03b1

stuartmorgan-g requested a review from LongCatIsLooong as a code owner October 24, 2025 14:05

github-actions bot added p: google_sign_in platform-ios platform-macos labels Oct 24, 2025

stuartmorgan-g mentioned this pull request Oct 24, 2025

google_sign_in is not working on ios 13 simulators. Throws 'presentingViewController must be set.' flutter/flutter#101700

Open

gemini-code-assist bot reviewed Oct 24, 2025

View reviewed changes

stuartmorgan-g commented Oct 24, 2025

View reviewed changes

stuartmorgan-g added 2 commits October 24, 2025 10:43

Property cleaunp

5bae394

Designated initializers

265c7ef

stuartmorgan-g added the triage-ios Should be looked at in iOS triage label Oct 30, 2025

vashworth self-requested a review October 30, 2025 21:22

vashworth reviewed Oct 31, 2025

View reviewed changes

...gle_sign_in_ios/darwin/google_sign_in_ios/Sources/google_sign_in_ios/FLTGoogleSignInPlugin.m Show resolved Hide resolved

...gle_sign_in_ios/darwin/google_sign_in_ios/Sources/google_sign_in_ios/FLTGoogleSignInPlugin.m Outdated Show resolved Hide resolved

stuartmorgan-g added 3 commits November 3, 2025 07:58

Add issue link

9024198

Split out a file of wrappers

f66dbb4

Merge branch 'main' into google-sign-in-ios-protocols

a822e9c

stuartmorgan-g requested a review from vashworth November 3, 2025 14:09

LongCatIsLooong approved these changes Nov 3, 2025

View reviewed changes

stuartmorgan-g added 2 commits November 3, 2025 14:43

Add to umbrella header

f26e4d4

Missed autoformat change

51da92a

vashworth approved these changes Nov 3, 2025

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 3, 2025

auto-submit bot merged commit 3b6170f into flutter:main Nov 3, 2025
80 checks passed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2025

3b6170fbc [google_sign_in] Remove use of OCMock (flutter/packages#10290)

705aff6

engine-flutter-autoroll mentioned this pull request Nov 4, 2025

Roll Packages from 1a7075b09ef1 to 8c13c08ca683 (5 revisions) flutter/flutter#177990

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 4, 2025

3b6170fbc [google_sign_in] Remove use of OCMock (flutter/packages#10290)

d170763

engine-flutter-autoroll mentioned this pull request Nov 4, 2025

Roll Packages from 1a7075b09ef1 to 3d926aa495d3 (6 revisions) flutter/flutter#177998

Merged


		#pragma mark - addScopes

		- (void)testRequestScopesPassesScopes {


		NS_ASSUME_NONNULL_BEGIN

		// TODO(stuarmorgan): Replace these with protocol extensions when migrating to Swift.

[google_sign_in] Remove use of OCMock #10290

[google_sign_in] Remove use of OCMock #10290

Uh oh!

Conversation

stuartmorgan-g commented Oct 24, 2025

Pre-Review Checklist

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Nov 3, 2025

Uh oh!

stuartmorgan-g commented Nov 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stuartmorgan-g Oct 24, 2025 •

edited

Loading

stuartmorgan-g Nov 3, 2025 •

edited

Loading