[iOS] Fix flaky keyboard animation test (#189353)
`testKeyboardAnimationWillWaitUIThreadVsync` posted a `sleep(1)` task to
the UI task runner and asserted that the keyboard animation callback did
not fire until at least that delay had elapsed. Whether or not the
callback actually observed the delay depended on thread scheduling,
which made the test flaky.
The property the old test was attempting to verify, that the callback
does not run until in-flight UI-thread work completes, is not
implemented by this code, so there is nothing here for a test to
protect. This is guaranteed by the run-loop itself. The vsync client
registers its `CADisplayLink` on the UI task runner's run loop (see
`setUpKeyboardAnimationVsyncClient` in `FlutterKeyboardInsetManager` and
`VSyncClient.init`), and a run loop services one source at a time, so
the display-link callback can't possibly run while the UI thread is busy
processing a begin-frame event. Asserting that ordering means asserting
`CFRunLoop` and `CADisplayLink` scheduling semantics, which is not only
outside our control but inherently racy/fragile, which causes flakes.
What this code *does* enforce is that delivery is asynchronous. The
callback is wrapped in `dispatch_async(dispatch_get_main_queue())`
rather than invoked inline from `-onDisplayLink:`. This updates the test
to trigger the display link directly, asserts the callback has not run
synchronously, then waits on an `XCTestExpectation` and asserts that
it's eventually delivered on the main queue.
The view controller is now built with `FlutterEnginePartialMock`, whose
`uiTaskRunner` runs on the test's own message loop, so the vsync
client's display-link registration and invalidation execute
deterministically on the current thread. I've also renamed the test to
`testKeyboardAnimationCallbackIsDeliveredAsynchronously` to match what
it actually verifies.
As mentioned above, this intentionally drops coverage of the
wait-for-UI-thread behavior originally added for
https://github.com/flutter/flutter/issues/120555. As noted, that's
already guaranteed by the run-loop and unrelated to our own logic.
Issue: https://github.com/flutter/flutter/issues/189352
## Pre-launch Checklist
- [X] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [X] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [X] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [X] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [X] I signed the [CLA].
- [X] I listed at least one issue that this PR fixes in the description
above.
- [X] I updated/added relevant documentation (doc comments with `///`).
- [X] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [X] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [X] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel
on [Discord].
If this change needs to override an active code freeze, provide a
comment explaining why. The code freeze workflow can be overridden by
code reviewers. See pinned issues for any active code freezes with
guidance.
**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
index 471cfb6..d26e130 100644
--- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
+++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
@@ -678,23 +678,14 @@
XCTAssertLessThan(capturedInset, 300.0);
}
-- (void)testKeyboardAnimationWillWaitUIThreadVsync {
- // We need to make sure the new viewport metrics get sent after the
- // begin frame event has processed. And this test is to expect that the callback
- // will sync with UI thread. So just simulate a lot of works on UI thread and
- // test the keyboard animation callback will not execute until UI task completed.
- // Related issue: https://github.com/flutter/flutter/issues/120555.
-
- FlutterEngine* engine = [[FlutterEngine alloc] init];
+- (void)testKeyboardAnimationCallbackIsDeliveredAsynchronously {
+ // FlutterEnginePartialMock.uiTaskRunner runs on the current (test) message loop, so the vsync
+ // client's display-link registration and invalidation happen deterministically on this thread.
+ FlutterEnginePartialMock* engine = [[FlutterEnginePartialMock alloc] init];
[engine runWithEntrypoint:nil];
FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engine
nibName:nil
bundle:nil];
- // Post a task to UI thread to block the thread.
- const int delayTime = 1;
- [[engine uiTaskRunner] postTask:^{
- sleep(delayTime);
- }];
id mockCADisplayLink = OCMClassMock([CADisplayLink class]);
OCMStub(
@@ -702,21 +693,23 @@
selector:sel_registerName("onDisplayLink:")]));
XCTestExpectation* expectation = [self expectationWithDescription:@"keyboard animation callback"];
- __block CFTimeInterval fulfillTime = 0;
- CFTimeInterval startTime = CACurrentMediaTime();
+ __block BOOL callbackExecuted = NO;
[viewController.keyboardInsetManager
setUpKeyboardAnimationVsyncClient:^(NSTimeInterval targetTime) {
- fulfillTime = CACurrentMediaTime();
+ callbackExecuted = YES;
[expectation fulfill];
}];
FlutterVSyncClient* client = viewController.keyboardInsetManager.keyboardAnimationVSyncClient;
[client onDisplayLink:client.displayLink];
+ // The callback is dispatched to the main queue, so it must not have run synchronously within
+ // -onDisplayLink:.
+ XCTAssertFalse(callbackExecuted);
+
+ // Spinning the run loop drains the main queue, at which point the callback runs.
[self waitForExpectationsWithTimeout:5.0 handler:nil];
- NSTimeInterval epsilon = 0.005;
- XCTAssertGreaterThanOrEqual(fulfillTime - startTime, delayTime - epsilon);
- fml::MessageLoop::GetCurrent().RunExpiredTasksNow();
+ XCTAssertTrue(callbackExecuted);
[mockCADisplayLink stopMocking];
}
