-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[camera] Run iOS methods on UI thread by default #4140
Changes from 36 commits
dea50fc
da83b51
277d923
9bd75bd
a079af9
bd7d8eb
f0a2bb0
4125244
71e321e
bdfde1a
e6b848c
d59a80d
0f35095
0b63dea
8bc557f
b53ab3e
b6acf82
9f31156
a4cf79e
2b84db2
92c2d00
2722cf4
4a1155a
04566ae
57c63ae
5c69728
e2ba654
17d1f5d
a0be148
ee453bc
cd48ddd
98fe08a
a4e5262
39d1438
3a12cbf
8e8bdfc
3e86910
9093887
1d51203
f0b9f53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| @import camera; | ||
| @import camera.Test; | ||
| @import XCTest; | ||
| @import AVFoundation; | ||
| #import <OCMock/OCMock.h> | ||
| #import "MockFLTThreadSafeFlutterResult.h" | ||
|
|
||
| @interface CameraMethodChannelTests : XCTestCase | ||
| @property(readonly, nonatomic) CameraPlugin *camera; | ||
| @property(readonly, nonatomic) MockFLTThreadSafeFlutterResult *resultObject; | ||
| @end | ||
|
|
||
| @implementation CameraMethodChannelTests | ||
|
|
||
| - (void)setUp { | ||
| _camera = [[CameraPlugin alloc] initWithRegistry:nil messenger:nil]; | ||
|
|
||
| XCTestExpectation *expectation = | ||
| [[XCTestExpectation alloc] initWithDescription:@"Result finished"]; | ||
|
|
||
| // Set up mocks for initWithCameraName method | ||
|
stuartmorgan-g marked this conversation as resolved.
|
||
| id avCaptureDeviceInputMock = OCMClassMock([AVCaptureDeviceInput class]); | ||
| OCMStub([avCaptureDeviceInputMock deviceInputWithDevice:[OCMArg any] error:[OCMArg anyObjectRef]]) | ||
| .andReturn([AVCaptureInput alloc]); | ||
|
|
||
| id avCaptureSessionMock = OCMClassMock([AVCaptureSession class]); | ||
| OCMStub([avCaptureSessionMock alloc]).andReturn(avCaptureSessionMock); | ||
| OCMStub([avCaptureSessionMock canSetSessionPreset:[OCMArg any]]).andReturn(YES); | ||
|
|
||
| _resultObject = [[MockFLTThreadSafeFlutterResult alloc] initWithExpectation:expectation]; | ||
| } | ||
|
renefloor marked this conversation as resolved.
Outdated
|
||
|
|
||
| - (void)testCreate_ShouldCallResultOnMainThread { | ||
| // Set up method call | ||
| FlutterMethodCall *call = [FlutterMethodCall | ||
| methodCallWithMethodName:@"create" | ||
| arguments:@{@"resolutionPreset" : @"medium", @"enableAudio" : @(1)}]; | ||
|
|
||
| [self->_camera handleMethodCallAsync:call result:self->_resultObject]; | ||
|
|
||
| // Verify the result | ||
| NSDictionary *dictionaryResult = (NSDictionary *)_resultObject.receivedResult; | ||
| XCTAssertNotNil(dictionaryResult); | ||
| XCTAssert([[dictionaryResult allKeys] containsObject:@"cameraId"]); | ||
| } | ||
|
|
||
| @end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,11 @@ - (void)setUp { | |
| self.cameraPlugin = [[CameraPlugin alloc] initWithRegistry:nil messenger:self.mockMessenger]; | ||
| } | ||
|
|
||
| - (void)tearDown { | ||
| self.mockMessenger = nil; | ||
| self.cameraPlugin = nil; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would actually much rather the ivars, the setup, and the teardown all be removed, and those two lines move into the test. Per the article I linked in a recent PR comment, stateless fixtures >> stateful fixtures, and in this case we're only saving two lines of boilerplate (assuming we eventually have more tests in this file; right now this is actually more code in order to theoretically reduce future duplication.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| } | ||
|
|
||
| - (void)testOrientationNotifications { | ||
| id mockMessenger = self.mockMessenger; | ||
| [mockMessenger setExpectationOrderMatters:YES]; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,44 +6,36 @@ | |
| @import XCTest; | ||
| @import AVFoundation; | ||
| #import <OCMock/OCMock.h> | ||
| #import "MockFLTThreadSafeFlutterResult.h" | ||
|
|
||
| @interface FLTCam : NSObject <FlutterTexture, | ||
| AVCaptureVideoDataOutputSampleBufferDelegate, | ||
| AVCaptureAudioDataOutputSampleBufferDelegate> | ||
| @property(assign, nonatomic) BOOL isPreviewPaused; | ||
| - (void)pausePreviewWithResult:(FlutterResult)result; | ||
| - (void)resumePreviewWithResult:(FlutterResult)result; | ||
| - (void)pausePreviewWithResult:(FLTThreadSafeFlutterResult *)result; | ||
| - (void)resumePreviewWithResult:(FLTThreadSafeFlutterResult *)result; | ||
| @end | ||
|
|
||
| @interface CameraPreviewPauseTests : XCTestCase | ||
| @property(readonly, nonatomic) FLTCam* camera; | ||
| @property(readonly, nonatomic) FLTCam *camera; | ||
| @property(readonly, nonatomic) MockFLTThreadSafeFlutterResult *resultObject; | ||
| @end | ||
|
|
||
| @implementation CameraPreviewPauseTests | ||
|
|
||
| - (void)setUp { | ||
| _camera = [[FLTCam alloc] init]; | ||
|
|
||
| _resultObject = [[MockFLTThreadSafeFlutterResult alloc] init]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be local (and while you're here, you could move
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| } | ||
|
|
||
| - (void)testPausePreviewWithResult_shouldPausePreview { | ||
| XCTestExpectation* resultExpectation = | ||
| [self expectationWithDescription:@"Succeeding result with nil value"]; | ||
| [_camera pausePreviewWithResult:^void(id _Nullable result) { | ||
| XCTAssertNil(result); | ||
| [resultExpectation fulfill]; | ||
| }]; | ||
| [self waitForExpectationsWithTimeout:2.0 handler:nil]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you still need this (and an expectation passed to the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so, the Also the result object that is passed in is a simple mock implementation of the So as far as I understand everything will run synchronously after each other. |
||
| [_camera pausePreviewWithResult:_resultObject]; | ||
| XCTAssertTrue(_camera.isPreviewPaused); | ||
| } | ||
|
|
||
| - (void)testResumePreviewWithResult_shouldResumePreview { | ||
| XCTestExpectation* resultExpectation = | ||
| [self expectationWithDescription:@"Succeeding result with nil value"]; | ||
| [_camera resumePreviewWithResult:^void(id _Nullable result) { | ||
| XCTAssertNil(result); | ||
| [resultExpectation fulfill]; | ||
| }]; | ||
| [self waitForExpectationsWithTimeout:2.0 handler:nil]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See explanation above. |
||
| [_camera resumePreviewWithResult:_resultObject]; | ||
| XCTAssertFalse(_camera.isPreviewPaused); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| #ifndef MockFLTThreadSafeFlutterResult_h | ||
| #define MockFLTThreadSafeFlutterResult_h | ||
|
|
||
| /** | ||
| * Extends FLTThreadSafeFlutterResult to give tests the ability to wait on the result and | ||
| * read the received result. | ||
| */ | ||
| @interface MockFLTThreadSafeFlutterResult : FLTThreadSafeFlutterResult | ||
| @property(readonly, nonatomic) XCTestExpectation *_Nonnull expectation; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this use the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this is possible, I used the Xcode "Fix" feature when the analyser warned me to specify a |
||
| @property(nonatomic, nullable) id receivedResult; | ||
|
|
||
| - (instancetype _Nonnull)initWithExpectation:(XCTestExpectation *_Nonnull)expectation; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These should be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| @end | ||
|
|
||
| #endif /* MockFLTThreadSafeFlutterResult_h */ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // 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 "MockFLTThreadSafeFlutterResult.h" | ||
|
|
||
| @implementation MockFLTThreadSafeFlutterResult | ||
| /** | ||
| * Initializes the MockFLTThreadSafeFlutterResult. | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overrides of superclass methods don't need (and shouldn't have, to avoid duplication that can drift over time) declaration comments, as the interface there should have the declaration comment.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
| - (instancetype)init { | ||
| self = [super init]; | ||
| return self; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But also this method shouldn't exist, because it's not doing anything.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
|
|
||
| /** | ||
| * Initializes the MockFLTThreadSafeFlutterResult with an expectation. | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a declaration comment, so should be in the header. It should also describe what the expectation is (i.e., that it's fulfilled when a result is called), so that the comment is actually providing useful information.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved into the header file and added additional description. |
||
| - (instancetype)initWithExpectation:(XCTestExpectation *)expectation { | ||
| self = [super init]; | ||
| _expectation = expectation; | ||
| return self; | ||
| } | ||
|
|
||
| /** | ||
| * Called when result is successful. | ||
| * | ||
| * Stores the data in the `receivedResult` property and fulfills the expectation. | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove per above.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
| - (void)sendSuccessWithData:(id)data { | ||
| _receivedResult = data; | ||
| [self->_expectation fulfill]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Property access should not use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
| } | ||
|
|
||
| /** | ||
| * Called when result is successful. | ||
| * | ||
| * Fulfills the expectation. | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
| - (void)sendSuccess { | ||
| _receivedResult = nil; | ||
| [self->_expectation fulfill]; | ||
| } | ||
| @end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| @import camera; | ||
| @import XCTest; | ||
|
|
||
| @interface ThreadSafeFlutterResultTests : XCTestCase | ||
| @end | ||
|
|
||
| @implementation ThreadSafeFlutterResultTests | ||
| - (void)testAsyncSendSuccess_ShouldCallResultOnMainThread { | ||
| XCTestExpectation* expectation = | ||
| [[XCTestExpectation alloc] initWithDescription:@"Result finished"]; | ||
|
|
||
| FLTThreadSafeFlutterResult* threadSafeFlutterResult = | ||
| [[FLTThreadSafeFlutterResult alloc] initWithResult:^(id _Nullable result) { | ||
| XCTAssert(NSThread.isMainThread); | ||
| [expectation fulfill]; | ||
| }]; | ||
| dispatch_queue_t dispatchQueue = dispatch_queue_create("test dispatchqueue", NULL); | ||
| dispatch_async(dispatchQueue, ^{ | ||
| [threadSafeFlutterResult sendSuccess]; | ||
| }); | ||
|
|
||
| [self waitForExpectations:[NSArray arrayWithObject:expectation] timeout:1]; | ||
| } | ||
|
|
||
| - (void)testSyncSendSuccess_ShouldCallResultOnMainThread { | ||
| XCTestExpectation* expectation = | ||
| [[XCTestExpectation alloc] initWithDescription:@"Result finished"]; | ||
|
|
||
| FLTThreadSafeFlutterResult* threadSafeFlutterResult = | ||
| [[FLTThreadSafeFlutterResult alloc] initWithResult:^(id _Nullable result) { | ||
| XCTAssert(NSThread.isMainThread); | ||
| [expectation fulfill]; | ||
| }]; | ||
| [threadSafeFlutterResult sendSuccess]; | ||
| [self waitForExpectations:[NSArray arrayWithObject:expectation] timeout:1]; | ||
| } | ||
|
|
||
| - (void)testSendNotImplemented_ShouldSendNotImplementedToFlutterResult { | ||
| XCTestExpectation* expectation = | ||
| [[XCTestExpectation alloc] initWithDescription:@"Result finished"]; | ||
|
|
||
| FLTThreadSafeFlutterResult* threadSafeFlutterResult = | ||
| [[FLTThreadSafeFlutterResult alloc] initWithResult:^(id _Nullable result) { | ||
| XCTAssert([result isKindOfClass:FlutterMethodNotImplemented.class]); | ||
| [expectation fulfill]; | ||
| }]; | ||
| dispatch_queue_t dispatchQueue = dispatch_queue_create("test dispatchqueue", NULL); | ||
| dispatch_async(dispatchQueue, ^{ | ||
| [threadSafeFlutterResult sendNotImplemented]; | ||
| }); | ||
|
|
||
| [self waitForExpectations:[NSArray arrayWithObject:expectation] timeout:1]; | ||
| } | ||
|
|
||
| - (void)testSendErrorDetails_ShouldSendErrorToFlutterResult { | ||
| NSString* errorCode = @"errorCode"; | ||
| NSString* errorMessage = @"message"; | ||
| NSString* errorDetails = @"error details"; | ||
| XCTestExpectation* expectation = | ||
| [[XCTestExpectation alloc] initWithDescription:@"Result finished"]; | ||
|
|
||
| FLTThreadSafeFlutterResult* threadSafeFlutterResult = | ||
| [[FLTThreadSafeFlutterResult alloc] initWithResult:^(id _Nullable result) { | ||
| XCTAssert([result isKindOfClass:FlutterError.class]); | ||
| FlutterError* error = (FlutterError*)result; | ||
| XCTAssertEqualObjects(error.code, errorCode); | ||
| XCTAssertEqualObjects(error.message, errorMessage); | ||
| XCTAssertEqualObjects(error.details, errorDetails); | ||
| [expectation fulfill]; | ||
| }]; | ||
| dispatch_queue_t dispatchQueue = dispatch_queue_create("test dispatchqueue", NULL); | ||
| dispatch_async(dispatchQueue, ^{ | ||
| [threadSafeFlutterResult sendErrorWithCode:errorCode message:errorMessage details:errorDetails]; | ||
| }); | ||
|
|
||
| [self waitForExpectations:[NSArray arrayWithObject:expectation] timeout:1]; | ||
| } | ||
|
|
||
| - (void)testSendNSError_ShouldSendErrorToFlutterResult { | ||
| NSError* originalError = [[NSError alloc] initWithDomain:NSURLErrorDomain code:404 userInfo:nil]; | ||
| XCTestExpectation* expectation = | ||
| [[XCTestExpectation alloc] initWithDescription:@"Result finished"]; | ||
|
|
||
| FLTThreadSafeFlutterResult* threadSafeFlutterResult = | ||
| [[FLTThreadSafeFlutterResult alloc] initWithResult:^(id _Nullable result) { | ||
| XCTAssert([result isKindOfClass:FlutterError.class]); | ||
| FlutterError* error = (FlutterError*)result; | ||
| NSString* constructedErrorCode = | ||
| [NSString stringWithFormat:@"Error %d", (int)originalError.code]; | ||
| XCTAssertEqualObjects(error.code, constructedErrorCode); | ||
| [expectation fulfill]; | ||
| }]; | ||
| dispatch_queue_t dispatchQueue = dispatch_queue_create("test dispatchqueue", NULL); | ||
| dispatch_async(dispatchQueue, ^{ | ||
| [threadSafeFlutterResult sendError:originalError]; | ||
| }); | ||
|
|
||
| [self waitForExpectations:[NSArray arrayWithObject:expectation] timeout:1]; | ||
| } | ||
|
|
||
| - (void)testSendResult_ShouldSendResultToFlutterResult { | ||
| NSString* resultData = @"resultData"; | ||
| XCTestExpectation* expectation = | ||
| [[XCTestExpectation alloc] initWithDescription:@"Result finished"]; | ||
|
|
||
| FLTThreadSafeFlutterResult* threadSafeFlutterResult = | ||
| [[FLTThreadSafeFlutterResult alloc] initWithResult:^(id _Nullable result) { | ||
| XCTAssertEqualObjects(result, resultData); | ||
| [expectation fulfill]; | ||
| }]; | ||
| dispatch_queue_t dispatchQueue = dispatch_queue_create("test dispatchqueue", NULL); | ||
| dispatch_async(dispatchQueue, ^{ | ||
| [threadSafeFlutterResult sendSuccessWithData:resultData]; | ||
| }); | ||
|
|
||
| [self waitForExpectations:[NSArray arrayWithObject:expectation] timeout:1]; | ||
| } | ||
| @end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the comment in the other file: please remove these and make them local to the test. (Having the object under test be part of the fixture state is always a major red flag to me unless there's a very compelling reason.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.