[camera] Handle "create" method call in main thread on iOS #4007
Conversation
| // 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. | ||
|
|
There was a problem hiding this comment.
This file hasn't been added to the project, so isn't being run.
| self.mockRegistrar = OCMProtocolMock(@protocol(FlutterPluginRegistrar)); | ||
| self.mockMessenger = OCMProtocolMock(@protocol(FlutterBinaryMessenger)); | ||
| OCMStub([self.mockRegistrar messenger]).andReturn(self.mockMessenger); | ||
| self.plugin = [[CameraPlugin alloc] initWithRegistry:self.mockRegistrar |
There was a problem hiding this comment.
Please don't make the object under test stateful. Test fixtures should have as little state as possible so that it's obvious what's happening within each test, and that's especially true of seeing how the object under test is being set up.
|
|
||
| - (void)testHandleMethodCallSync_ShouldHandleSyncMethods { | ||
| id methodCallMock = OCMClassMock([FlutterMethodCall class]); | ||
| OCMStub([methodCallMock method]).andReturn(@"create"); |
There was a problem hiding this comment.
Why is this a mock? Mocking is a tool of last resort; FlutterMethodCall is trivial to instantiate.
| - (instancetype)initWithRegistry:(NSObject<FlutterTextureRegistry> *)registry | ||
| messenger:(NSObject<FlutterBinaryMessenger> *)messenger; | ||
| - (BOOL)handleMethodCallSync:(FlutterMethodCall *)call result:(FlutterResult)result; | ||
| - (BOOL)handleMethodCallAsync:(FlutterMethodCall *)call result:(FlutterResult)result; |
There was a problem hiding this comment.
Testing with private methods should only be done when a test can't be done with public methods. I don't see why accessing these should be necessary.
| [[self plugin] handleMethodCall:methodCallMock result:result]; | ||
|
|
||
| OCMVerify([mockedPlugin handleMethodCallSync:methodCallMock result:result]); | ||
| OCMVerify(never(), [mockedPlugin handleMethodCallAsync:methodCallMock result:result]); |
There was a problem hiding this comment.
Most of these tests are testing for an implementation detail rather than testing that the thing we want to be true—that interactions with the engine are on the main thread. We should test for what we want to assert, not for an implementation that we believe has the properties we want.
As one of many reasons this is true: would these tests fail if someone accidentally swapped which internal method call was done on a queue and which one wasn't?
See #3778 for an example where I fixed a similar issue in another plugin, and handled it with assertions in the code and tests designed to exercise the assertions without assuming things about the implementation details of the plugin itself.
| return; | ||
| } | ||
|
|
||
| // Otherwise invoke the plugin on another dispatch queue to avoid blocking the UI. | ||
| dispatch_async(_dispatchQueue, ^{ | ||
| [self handleMethodCallAsync:call result:result]; |
There was a problem hiding this comment.
I've commented on the issue to get clarity, but I believe all uses of result on the background queue are also incorrect, so this needs a larger restructure.
Delegating to a queue should very likely be limited to just expensive operations, rather than a default behavior.
This PR moves the handling of the "create" method call to the main thread on iOS, due to issues mentioned in flutter/flutter#52578.
Pre-launch Checklist
dart format. See plugin_tool format)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.