Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

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

Merged
merged 5 commits into from
Oct 14, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
165b15c
Refactored unit test to prevent usage of XCUIDevice.
mvanbeusekom Oct 12, 2021
d9292ba
Add changelog description
mvanbeusekom Oct 12, 2021
6083b93
Replace use of ivar with synthesized property
mvanbeusekom Oct 12, 2021
8af03a6
[camera] Add camera test module map
jmagman Oct 13, 2021
30f7c8d
Fixed nits from PR and bump patch number.
mvanbeusekom Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/camera/camera/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## NEXT
Copy link
Member

@jmagman jmagman Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mvanbeusekom mvanbeusekom Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


* Updated package description.
* Updated package description;
* Refactor unit test on iOS to make it compatible with new restrictions in Xcode 13 which only supports the use of the `XCUIDevice` in Xcode UI tests.

## 0.9.4+1

Expand Down
48 changes: 30 additions & 18 deletions packages/camera/camera/example/ios/RunnerTests/CameraOrientationTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,66 +2,78 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

@import camera;
@import XCTest;

#import <Flutter/Flutter.h>
Copy link
Member

@jmagman jmagman Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#import <Flutter/Flutter.h>
@import Flutter;

Copy link
Contributor Author

@mvanbeusekom mvanbeusekom Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#import <OCMock/OCMock.h>

@interface CameraPlugin : NSObject
- (instancetype)initWithRegistry:(NSObject<FlutterTextureRegistry> *)registry
messenger:(NSObject<FlutterBinaryMessenger> *)messenger;

- (void)orientationChanged:(NSNotification *)note;
@end
Copy link
Member

@jmagman jmagman Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mvanbeusekom mvanbeusekom Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jmagman jmagman Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mvanbeusekom mvanbeusekom Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?


@interface CameraOrientationTests : XCTestCase
@property(strong, nonatomic) id mockRegistrar;
@property(strong, nonatomic) id mockMessenger;
@property(strong, nonatomic) CameraPlugin *cameraPlugin;
@end

@implementation CameraOrientationTests

- (void)setUp {
[super setUp];
self.mockRegistrar = OCMProtocolMock(@protocol(FlutterPluginRegistrar));

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

@jmagman jmagman Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

Copy link
Contributor Author

@mvanbeusekom mvanbeusekom Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks for educating.

}

- (void)testOrientationNotifications {
id mockMessenger = self.mockMessenger;
[mockMessenger setExpectationOrderMatters:YES];
XCUIDevice.sharedDevice.orientation = UIDeviceOrientationPortrait;

[CameraPlugin registerWithRegistrar:self.mockRegistrar];

[self rotate:UIDeviceOrientationPortraitUpsideDown expectedChannelOrientation:@"portraitDown"];
[self rotate:UIDeviceOrientationPortrait expectedChannelOrientation:@"portraitUp"];
[self rotate:UIDeviceOrientationLandscapeRight expectedChannelOrientation:@"landscapeLeft"];
[self rotate:UIDeviceOrientationLandscapeLeft expectedChannelOrientation:@"landscapeRight"];

OCMReject([mockMessenger sendOnChannel:[OCMArg any] message:[OCMArg any]]);
// No notification when orientation doesn't change.
XCUIDevice.sharedDevice.orientation = UIDeviceOrientationLandscapeLeft;

// No notification when flat.
XCUIDevice.sharedDevice.orientation = UIDeviceOrientationFaceUp;
[self.cameraPlugin
orientationChanged:[self createMockNotificationForOrientation:UIDeviceOrientationFaceUp]];
// No notification when facedown.
XCUIDevice.sharedDevice.orientation = UIDeviceOrientationFaceDown;
[self.cameraPlugin
orientationChanged:[self createMockNotificationForOrientation:UIDeviceOrientationFaceDown]];

OCMVerifyAll(mockMessenger);
}

- (void)rotate:(UIDeviceOrientation)deviceOrientation
expectedChannelOrientation:(NSString*)channelOrientation {
expectedChannelOrientation:(NSString *)channelOrientation {
id mockMessenger = self.mockMessenger;
XCTestExpectation* orientationExpectation = [self expectationWithDescription:channelOrientation];
XCTestExpectation *orientationExpectation = [self expectationWithDescription:channelOrientation];

OCMExpect([mockMessenger
sendOnChannel:[OCMArg any]
message:[OCMArg checkWithBlock:^BOOL(NSData* data) {
NSObject<FlutterMethodCodec>* codec = [FlutterStandardMethodCodec sharedInstance];
FlutterMethodCall* methodCall = [codec decodeMethodCall:data];
message:[OCMArg checkWithBlock:^BOOL(NSData *data) {
NSObject<FlutterMethodCodec> *codec = [FlutterStandardMethodCodec sharedInstance];
FlutterMethodCall *methodCall = [codec decodeMethodCall:data];
[orientationExpectation fulfill];
return
[methodCall.method isEqualToString:@"orientation_changed"] &&
[methodCall.arguments isEqualToDictionary:@{@"orientation" : channelOrientation}];
}]]);

XCUIDevice.sharedDevice.orientation = deviceOrientation;
[self.cameraPlugin
orientationChanged:[self createMockNotificationForOrientation:deviceOrientation]];
[self waitForExpectationsWithTimeout:30.0 handler:nil];
}

- (NSNotification *)createMockNotificationForOrientation:(UIDeviceOrientation)deviceOrientation {
UIDevice *mockDevice = OCMClassMock([UIDevice class]);
OCMStub([mockDevice orientation]).andReturn(deviceOrientation);

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

@jmagman jmagman Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mvanbeusekom mvanbeusekom Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

@end