Reland: Started waiting for the notifications locally before asserting side-effects #25257
Conversation
were called for plugin lifecycle tests.
| /// Sometimes we have to use a custom mock to avoid retain cycles in ocmock. | ||
| @interface FlutterEnginePartialMock : FlutterEngine | ||
| @property(nonatomic, strong) FlutterBasicMessageChannel* lifecycleChannel; | ||
| @property(nonatomic, weak) FlutterViewController* viewController; | ||
| @property(nonatomic, assign) BOOL didCallNotifyLowMemory; | ||
| @end | ||
|
|
||
| @implementation FlutterEnginePartialMock | ||
| @synthesize viewController; | ||
| @synthesize lifecycleChannel; | ||
|
|
||
| - (void)notifyLowMemory { | ||
| _didCallNotifyLowMemory = YES; | ||
| } | ||
| @end |
There was a problem hiding this comment.
@jmagman This is what I had to do in order to get rid of the cycles. I would hope that upgrading ocmock would also fix this, but it was hard to upgrade ocmock because it changed its dependencies, it's complicated to get GN working with it.
| @@ -970,5 +970,6 @@ - (void)testAccessibilityMessageAfterDeletion { | |||
| }); | |||
| latch.Wait(); | |||
| OCMVerify([messenger cleanupConnection:connection]); | |||
| [engine stopMocking]; | |||
There was a problem hiding this comment.
This might be superfluous now. I don't think it hurts anything. Once I got everything working I didn't try to cull any of the previous work I did.
| [viewController beginAppearanceTransition:NO animated:NO]; | ||
| [viewController endAppearanceTransition]; | ||
| OCMVerify([self.mockEngine notifyLowMemory]); | ||
| XCTAssertTrue(mockEngine.didCallNotifyLowMemory); |
There was a problem hiding this comment.
If you can actually allocate a FlutterEngine subclass without mocking, is the subclass necessary? Does it work if you want for an XCTKVOExpectation?
[self keyValueObservingExpectation:mockEngine keyPath:@"notifyLowMemory" handler:nil]There was a problem hiding this comment.
You mean could we use a real FlutterEngine? There shouldn't be a keyPath notifyLowMemory on a standard FlutterEngine, that would only get created if there was a property called notifyLowMemory or if we manually registered one.
There was a problem hiding this comment.
(the subclass is also necessary for setting the lifecycle channel, not just needed for the notifyLowMemory stuff)
gaaclarke
added
the
waiting for tree to go green
gaaclarke
removed
the
waiting for tree to go green
…asserting side-effects (flutter/engine#25257)
Relanding #25226
It had to be reverted because other tests were leaking FlutterViewControllers because of OCMock stubbing. I couldn't get ocmock to release the objects so I had to make my own mock engine.
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.