[google_sign_in] Add iOS unit tests #4157

jmagman · 2021-07-15T02:23:32Z

Add google_sign_in iOS unit tests to validate we don't break (much) in preparation for [google_sign_in] Update GoogleSignIn dependency to 6.2 flutter#86436, which included SDK removals and signature changes without deprecations. Native code plugin coverage increased from 41.4% to 78.4%.
Add -[FLTGoogleSignInPlugin initWithSignIn:] initializer to allow easy injection of GIDSignIn into unit tests. Expose in a new google_sign_in.Test explicit clang module. Include new modulemap and umbrella header to support this.
Add lightweight generics to the plugin.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jmagman · 2021-07-15T03:45:17Z

packages/google_sign_in/google_sign_in/example/ios/Flutter/AppFrameworkInfo.plist

@@ -25,6 +25,6 @@
    <string>arm64</string>
  </array>
  <key>MinimumOSVersion</key>
-  <string>8.0</string>
+  <string>9.0</string>


Automatic from flutter/flutter#85174

jmagman · 2021-07-15T03:45:21Z

packages/google_sign_in/google_sign_in/example/ios/Runner.xcodeproj/project.pbxproj

@@ -539,7 +539,7 @@
 				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
 				GCC_WARN_UNUSED_FUNCTION = YES;
 				GCC_WARN_UNUSED_VARIABLE = YES;
-				IPHONEOS_DEPLOYMENT_TARGET = 8.0;
+				IPHONEOS_DEPLOYMENT_TARGET = 9.0;


Automatic from flutter/flutter#85174

stuartmorgan-g

Nice :)

The description says "Turn on code coverage", but I'm not seeing project changes other than deployment target.

packages/google_sign_in/google_sign_in/example/ios/RunnerTests/GoogleSignInTests.m

packages/google_sign_in/google_sign_in/ios/Classes/FLTGoogleSignInPlugin.m

jmagman · 2021-07-15T21:23:49Z

The description says "Turn on code coverage", but I'm not seeing project changes other than deployment target.

Ah sorry I removed that in an intermediate commit since it's not being checked anywhere, so not worth the overhead in CI. Fixed the description.

jmagman · 2021-07-15T22:33:49Z

packages/google_sign_in/google_sign_in/ios/Classes/FLTGoogleSignInPlugin_Private.h

@@ -0,0 +1,13 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


Unfortunately I had to make this a public header to make it accessible to the RunnerTest.

Maybe I should rename to FLTGoogleSignInPlugin_Test.h

Just renamed it.

It would be kind of ugly, but couldn't the test do a path-based include of the private header instead of getting it via the module import?

Alternately, you could do the thing that I almost always tell people not to do (in favor of making an internal header like you did) and re-declare the method you need at the top of the test file. I've never liked it, but it seems better than actually making the private header public in the module.

How about a Test module so it needs to be explicitly imported?

@import google_sign_in; @import google_sign_in.Test;

stuartmorgan-g

LGTM with the private header thing adrressed

packages/google_sign_in/google_sign_in/ios/Classes/FLTGoogleSignInPlugin_Test.h

jmagman · 2021-07-19T19:39:54Z

packages/google_sign_in/google_sign_in/pubspec.yaml

@@ -3,7 +3,7 @@ description: Flutter plugin for Google Sign-In, a secure authentication system
  for signing in with a Google account on Android and iOS.
 repository: https://github.com/flutter/plugins/tree/master/packages/google_sign_in/google_sign_in
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+google_sign_in%22
-version: 5.0.4
+version: 5.0.5


@stuartmorgan with the module change this seems different enough to warrant a patch bump.

jmagman · 2021-07-19T19:40:21Z

packages/google_sign_in/google_sign_in/ios/google_sign_in.podspec

@@ -12,8 +12,9 @@ Enables Google Sign-In in Flutter apps.
  s.license          = { :type => 'BSD', :file => '../LICENSE' }
  s.author           = { 'Flutter Team' => '[email protected]' }
  s.source           = { :http => 'https://github.com/flutter/plugins/tree/master/packages/google_sign_in' }
-  s.source_files = 'Classes/**/*'
+  s.source_files = 'Classes/**/*.{h,m}'


modulemap shouldn't be included in the source files.

jmagman · 2021-07-19T19:41:08Z

packages/google_sign_in/google_sign_in/ios/Classes/google_sign_in-umbrella.h

@@ -0,0 +1,10 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


The file was previously generated by CocoaPods with the same name. Now we need to manage it manually and check it into the repo because we're defining a module map, which takes us out of the magical auto module world.

jmagman · 2021-07-19T19:44:07Z

packages/google_sign_in/google_sign_in/ios/Classes/FLTGoogleSignInPlugin.modulemap

+  export *
+  module * { export * }
+
+  explicit module Test {


This isn't a private module since there are some complications for Swift files not being able to import those.

Instead, this explicit module requires that google_sign_in.Test be imported to access FLTGoogleSignInPlugin_Test. So initWithSignIn: won't be available in the normal import google_sign_in case, won't auto-complete, etc.

stuartmorgan-g

LGTM

Refactored the existing unit tests to expose private interfaces in a separate test header instead of an inline interface. This was first introduced in the google_sign_in plugin (flutter#4157) and after that also applied in the camera plugin (flutter#4426) and the webview_flutter plugin (flutter#4480). This commit also removed several obsolete `OCMStub` declarations.

Refactored the existing unit tests to expose private interfaces in a separate test header instead of an inline interface. This was first introduced in the google_sign_in plugin (flutter#4157) and after that also applied in the camera plugin (flutter#4426) and the webview_flutter plugin (flutter#4480).

[google_sign_in] Add iOS unit tests

b2d5f81

jmagman added p: google_sign_in platform-ios labels Jul 15, 2021

jmagman self-assigned this Jul 15, 2021

google-cla bot added the cla: yes label Jul 15, 2021

jmagman commented Jul 15, 2021

View reviewed changes

Format

4d90826

jmagman force-pushed the gsi-tests branch from b6704d8 to 4d90826 Compare July 15, 2021 03:48

stuartmorgan-g reviewed Jul 15, 2021

View reviewed changes

Injected GIDSignIn

cd4a6c4

jmagman force-pushed the gsi-tests branch from 9fc8976 to cd4a6c4 Compare July 15, 2021 22:16

jmagman added 2 commits July 15, 2021 15:27

valueForKey

9a67228

Angle imports

6cc702f

jmagman commented Jul 15, 2021

View reviewed changes

FLTGoogleSignInPlugin_Test

fab5aa0

jmagman force-pushed the gsi-tests branch from 75e5efc to fab5aa0 Compare July 15, 2021 22:47

stuartmorgan-g approved these changes Jul 16, 2021

View reviewed changes

packages/google_sign_in/google_sign_in/ios/Classes/FLTGoogleSignInPlugin_Test.h Show resolved Hide resolved

packages/google_sign_in/google_sign_in/ios/Classes/FLTGoogleSignInPlugin_Test.h Show resolved Hide resolved

google_sign_in.Test module

6d48338

jmagman commented Jul 19, 2021

View reviewed changes

Format

72fba46

stuartmorgan-g approved these changes Jul 19, 2021

View reviewed changes

jmagman merged commit 45a1ba6 into flutter:master Jul 20, 2021

jmagman deleted the gsi-tests branch July 20, 2021 00:05

engine-flutter-autoroll mentioned this pull request Jul 20, 2021

Roll Plugins from 027db9039c03 to 45a1ba6d91af (1 revision) flutter/flutter#86687

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 20, 2021

45a1ba6 [google_sign_in] Add iOS unit tests (flutter/plugins#4157)

c728796

engine-flutter-autoroll mentioned this pull request Jul 27, 2021

Roll Plugins from 027db9039c03 to 530a18705f74 (21 revisions) flutter/flutter#87080

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 27, 2021

45a1ba6 [google_sign_in] Add iOS unit tests (flutter/plugins#4157)

0ed2eee

engine-flutter-autoroll mentioned this pull request Jul 27, 2021

Roll Plugins from 027db9039c03 to 5f42c6920cc6 (22 revisions) flutter/flutter#87088

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 27, 2021

45a1ba6 [google_sign_in] Add iOS unit tests (flutter/plugins#4157)

8e50a86

fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021

[google_sign_in] Add iOS unit tests (flutter#4157)

56a07e2

amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021

[google_sign_in] Add iOS unit tests (flutter#4157)

fc4132c

jmagman mentioned this pull request Oct 12, 2021

[camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 #4426

Merged

11 tasks

jmagman mentioned this pull request Jan 26, 2022

[camera]refactor flash mode and add unit test #4695

Merged

11 tasks

mvanbeusekom mentioned this pull request Feb 2, 2022

[image_picker] Refactored tests to expose private interface in separate test header. #4722

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_sign_in] Add iOS unit tests #4157

[google_sign_in] Add iOS unit tests #4157

jmagman commented Jul 15, 2021 •

edited

Loading

jmagman Jul 15, 2021

jmagman Jul 15, 2021

stuartmorgan-g left a comment

jmagman commented Jul 15, 2021

jmagman Jul 15, 2021

jmagman Jul 15, 2021

jmagman Jul 15, 2021

stuartmorgan-g Jul 16, 2021

stuartmorgan-g Jul 16, 2021

jmagman Jul 19, 2021

stuartmorgan-g left a comment

jmagman Jul 19, 2021

jmagman Jul 19, 2021

jmagman Jul 19, 2021

jmagman Jul 19, 2021

stuartmorgan-g left a comment

		@@ -0,0 +1,13 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

		@@ -0,0 +1,10 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

[google_sign_in] Add iOS unit tests #4157

[google_sign_in] Add iOS unit tests #4157

Conversation

jmagman commented Jul 15, 2021 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan-g left a comment

Choose a reason for hiding this comment

jmagman commented Jul 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan-g left a comment

Choose a reason for hiding this comment

jmagman commented Jul 15, 2021 •

edited

Loading