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

mvanbeusekom · 2021-10-12T09:34:08Z

Refactored the iOS native [CameraOrientationTests testOrientationNotifications] unit test to remove the dependency on the XCUIDevice class. The XCUIDevice class can no longer be used in unit tests starting from Xcode 13 (the class will only be available in Xcode UI tests).

The updates made in this PR isolates the [CameraPlugin orientationChanged] adhering to the principles of a unit test. This however has two consequences that would be good to verify:

The previous version of this test functioned as an integration test, also running the [CameraPlugin startOrientationListener] method and relying on the NSNotificationCenter to deliver orientation updates. This version directly calls the [CameraPlugin orientationChanged] method with a mocked NSNotification object and thus no longer tests the integration between the NSNotificationCenter and the CameraPlugin class. Since this is only simple initialisation logic I decided it wouldn't be a big problem, but it is worth mentioning here to make sure everybody agrees;
I have removed the test which makes sure that no message is send when the [CameraPlugin orientationChanged] method is called with the same orientation the device is already in. The reason is that the [CameraPlugin orientationChanged] method doesn't contain any logic which would prevent this. The reason this wasn't a problem earlier is that the NSNotificationCenter will not deliver a notification if the orientation isn't physically different. Since we are now testing the [CameraPlugin orientationChanged] method in isolation and no longer the integration with NSNotificationCenter this test would fail. If it is necessary to ensure this behaviour in the [CameraPlugin orientationChanged] method, the implementation needs to change and I didn't want to do so unless everybody agrees it needs to be changed.

Resolves flutter/flutter#84162

Pre-launch Checklist

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

Refactored the unit test so it no longer relies on the XCUIDevice class which can only be used by UI tests starting from Xcode 13 and higher.

jmagman

Thanks for working on this!

jmagman · 2021-10-12T17:34:25Z

packages/camera/camera/example/ios/RunnerTests/CameraOrientationTests.m

  self.mockMessenger = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
-  OCMStub([self.mockRegistrar messenger]).andReturn(self.mockMessenger);
+  self.cameraPlugin = [[CameraPlugin alloc] initWithRegistry:nil messenger:_mockMessenger];


Nit: Don't use ivars (except in getters, initializers, and dealloc), use the synthesized property.

Suggested change

self.cameraPlugin = [[CameraPlugin alloc] initWithRegistry:nil messenger:_mockMessenger];

self.cameraPlugin = [[CameraPlugin alloc] initWithRegistry:nil messenger:self.mockMessenger];

Fixed, thanks for educating.

jmagman · 2021-10-12T19:28:17Z

packages/camera/camera/example/ios/RunnerTests/CameraOrientationTests.m

+@interface CameraPlugin : NSObject
+- (instancetype)initWithRegistry:(NSObject<FlutterTextureRegistry> *)registry
+                       messenger:(NSObject<FlutterBinaryMessenger> *)messenger;
+
+- (void)orientationChanged:(NSNotification *)note;
+@end


We don't generally want to expose private interfaces like this, and instead expose in a test header like FLTGoogleSignInPlugin_Test.h introduced in #4157. Though that caused a lot of changes, I had to add a module map and umbrella header.
@stuartmorgan what's your take on this? Would you prefer the a CameraPlugin_Test.h header and explicit module map, or is a little SPI for an API under test (and so would fail in the same commit if something changed) preferred?

Having a look at the changes you made in #4157, I feel confident I can also do the same here (have to read up a bit on the .modulemap but it doesn't look to complicated). However I will wait for @stuartmorgan's take on this, but I do see why it would be useful, thanks.

I always prefer explicit test headers; it makes it much easier for someone looking at the non-test code to understand what parts of the internals are being used by tests. (I also like to hope that maybe having to be very deliberate about the fact that the internals of the class are being exposed and used gives people more pause than just tossing a couple of lines at the top of a test file where "it's just test code".)

@mvanbeusekom There were a few gotchas with modulemaps, the podspec, and designated initializers, so I took the liberty of getting it all working in #4430. You can cherry pick that change into this PR, or I can update the pubspec/CHANGELOG and check it in first, up to you.

@jmagman thank you for doing all this work for me ;). I cherry picked your change into my branch and removed the inline interface declaration from the CameraOrientationTests.m file and all looks to be fine. Could you have another look at the PR and see if you agree with the changes?

jmagman

I think this needs a version bump and pubspec change, but other than that, a few very minor nits.

jmagman · 2021-10-13T17:14:09Z

packages/camera/camera/example/ios/RunnerTests/CameraOrientationTests.m

 @import XCTest;
-
+#import <Flutter/Flutter.h>


Suggested change

#import <Flutter/Flutter.h>

@import Flutter;

jmagman · 2021-10-13T17:15:50Z

packages/camera/camera/CHANGELOG.md

@@ -1,6 +1,7 @@
 ## NEXT


I think the modulemap change deserves a patch version bump since it's a change in the framework, not just the example tests.

jmagman · 2021-10-13T17:16:59Z

packages/camera/camera/example/ios/RunnerTests/CameraOrientationTests.m

+  UIDevice *mockDevice = OCMClassMock([UIDevice class]);
+  OCMStub([mockDevice orientation]).andReturn(deviceOrientation);
+
+  return [[NSNotification alloc] initWithName:@"orientation_test" object:mockDevice userInfo:nil];


Suggested change

return [[NSNotification alloc] initWithName:@"orientation_test" object:mockDevice userInfo:nil];

return [NSNotification notificationWithName:@"orientation_test" object:mockDevice];

jmagman

The test failures look like a GitHub connection issue? Might pass on a re-run.

fatal: unable to access 'https://github.com/flutter/plugins.git/': Failed to connect to github.com port 443: Connection refused

LGTM, thanks again!

jmagman · 2021-10-13T21:31:15Z

Might pass on a re-run.

And they did.

…cations] unit test to work on Xcode 13 (flutter/plugins#4426)

… unit test to work on Xcode 13 (flutter#4426)

* master: [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436) [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality (flutter#4389) Bump compileSdkVersion to 31 (flutter#4432) [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426) Update integration_test README (flutter#3824) [webview_flutter] Adjust test URLs again (flutter#4407) [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179) [camera] Add filter for unsupported cameras on Android (flutter#4418)

* master: [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436) [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality (flutter#4389) Bump compileSdkVersion to 31 (flutter#4432) [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426) Update integration_test README (flutter#3824) [webview_flutter] Adjust test URLs again (flutter#4407) [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179) [camera] Add filter for unsupported cameras on Android (flutter#4418) # Conflicts: # packages/webview_flutter/webview_flutter/CHANGELOG.md # packages/webview_flutter/webview_flutter/pubspec.yaml

* master: (364 commits) Use OpenJDK 11 in CI jobs (flutter#4419) [google_sign_in] remove the commented out code in tests (flutter#4442) [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436) [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality (flutter#4389) Bump compileSdkVersion to 31 (flutter#4432) [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426) Update integration_test README (flutter#3824) [webview_flutter] Adjust test URLs again (flutter#4407) [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179) [camera] Add filter for unsupported cameras on Android (flutter#4418) ... # Conflicts: # packages/google_maps_flutter/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

… unit test to work on Xcode 13 (flutter#4426)

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).

mvanbeusekom added 2 commits October 12, 2021 11:07

Refactored unit test to prevent usage of XCUIDevice.

165b15c

Refactored the unit test so it no longer relies on the XCUIDevice class which can only be used by UI tests starting from Xcode 13 and higher.

Add changelog description

d9292ba

mvanbeusekom requested review from stuartmorgan-g and jmagman October 12, 2021 09:34

mvanbeusekom requested a review from bparrishMines as a code owner October 12, 2021 09:34

google-cla bot added the cla: yes label Oct 12, 2021

github-actions bot added p: camera platform-ios labels Oct 12, 2021

jmagman reviewed Oct 12, 2021

View reviewed changes

Replace use of ivar with synthesized property

6083b93

This was referenced Oct 12, 2021

[ci] Update macOS Cirrus image to Xcode 13 #4429

Merged

[camera] Add camera test module map #4430

Closed

[camera] Add camera test module map

8af03a6

mvanbeusekom force-pushed the 84162 branch from 7f11f20 to 8af03a6 Compare October 13, 2021 07:01

mvanbeusekom requested a review from jmagman October 13, 2021 07:28

jmagman reviewed Oct 13, 2021

View reviewed changes

Fixed nits from PR and bump patch number.

30f7c8d

mvanbeusekom requested a review from jmagman October 13, 2021 17:26

jmagman approved these changes Oct 13, 2021

View reviewed changes

jmagman added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Oct 13, 2021

jmagman merged commit 176cfb8 into flutter:master Oct 14, 2021

mvanbeusekom deleted the 84162 branch October 14, 2021 05:29

engine-flutter-autoroll mentioned this pull request Oct 14, 2021

Roll Plugins from 6716d75c08cb to 176cfb8083bc (1 revision) flutter/flutter#91796

Merged

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

176cfb8 [camera] Update [CameraOrientationTests testOrientationNotifi…

aadd727

…cations] unit test to work on Xcode 13 (flutter/plugins#4426)

camsim99 pushed a commit to camsim99/plugins that referenced this pull request Oct 18, 2021

[camera] Update [CameraOrientationTests testOrientationNotifications]…

5cfb441

… unit test to work on Xcode 13 (flutter#4426)

amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021

[camera] Update [CameraOrientationTests testOrientationNotifications]…

28e1dba

… unit test to work on Xcode 13 (flutter#4426)

KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021

[camera] Update [CameraOrientationTests testOrientationNotifications]…

a3d6a77

… unit test to work on Xcode 13 (flutter#4426)

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

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

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

mvanbeusekom commented Oct 12, 2021 •

edited

Loading

jmagman left a comment

jmagman Oct 12, 2021

mvanbeusekom Oct 12, 2021

jmagman Oct 12, 2021

mvanbeusekom Oct 12, 2021

stuartmorgan-g Oct 13, 2021

jmagman Oct 13, 2021

mvanbeusekom Oct 13, 2021

jmagman left a comment

jmagman Oct 13, 2021

mvanbeusekom Oct 13, 2021

jmagman Oct 13, 2021

mvanbeusekom Oct 13, 2021

jmagman Oct 13, 2021

mvanbeusekom Oct 13, 2021

jmagman left a comment

jmagman commented Oct 13, 2021

	self.cameraPlugin = [[CameraPlugin alloc] initWithRegistry:nil messenger:_mockMessenger];
	self.cameraPlugin = [[CameraPlugin alloc] initWithRegistry:nil messenger:self.mockMessenger];

	return [[NSNotification alloc] initWithName:@"orientation_test" object:mockDevice userInfo:nil];
	return [NSNotification notificationWithName:@"orientation_test" object:mockDevice];

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

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

Conversation

mvanbeusekom commented Oct 12, 2021 • edited Loading

Pre-launch Checklist

jmagman 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman left a comment

Choose a reason for hiding this comment

jmagman commented Oct 13, 2021

mvanbeusekom commented Oct 12, 2021 •

edited

Loading