Skip to content

[iOS] Fix flaky keyboard animation test#189353

Merged
cbracken merged 2 commits into
flutter:masterfrom
cbracken:fix-flaky-test
Jul 14, 2026
Merged

[iOS] Fix flaky keyboard animation test#189353
cbracken merged 2 commits into
flutter:masterfrom
cbracken:fix-flaky-test

Conversation

@cbracken

@cbracken cbracken commented Jul 13, 2026

Copy link
Copy Markdown
Member

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
#120555. As noted, that's already guaranteed by the run-loop and unrelated to our own logic.

Issue: #189352

Pre-launch Checklist

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

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, which causes this test to be
flaky/fragile.

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
flutter#120555. As noted, that's
already guaranteed by the run-loop and unrelated to our own logic.
@cbracken
cbracken requested a review from a team as a code owner July 13, 2026 06:08
@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label Jul 13, 2026
@github-actions github-actions Bot added platform-ios iOS applications specifically engine flutter/engine related. See also e: labels. team-ios Owned by iOS platform team labels Jul 13, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors a keyboard animation test in FlutterViewControllerTest.mm by renaming testKeyboardAnimationWillWaitUIThreadVsync to testKeyboardAnimationCallbackIsDeliveredAsynchronously. It replaces FlutterEngine with FlutterEnginePartialMock to run deterministically on the test message loop, removing the need to block the UI thread with a sleep task, and asserts that the keyboard animation callback is executed asynchronously rather than synchronously. There are no review comments, and I have no feedback to provide.

@cbracken
cbracken requested a review from hellohuanlin July 13, 2026 06:27
bundle:nil];
// Post a task to UI thread to block the thread.
const int delayTime = 1;
[[engine uiTaskRunner] postTask:^{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it a recent flakiness? Curious if it is coming from our merged thread change.

@cbracken cbracken Jul 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not super recent; but I've run into it a bunch while tweaking keyboard inset manager, maybe due to tiny changes in timing due to either swift memory handling being more agressive?

@cbracken
cbracken added this pull request to the merge queue Jul 13, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jul 14, 2026
@cbracken
cbracken enabled auto-merge July 14, 2026 01:57
@cbracken
cbracken added this pull request to the merge queue Jul 14, 2026
Merged via the queue into flutter:master with commit 7a7b1a5 Jul 14, 2026
197 checks passed
@cbracken
cbracken deleted the fix-flaky-test branch July 14, 2026 05:59
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Jul 14, 2026
flutter/flutter@cf9e8af...846664b

2026-07-14 [email protected] Roll Skia from dfcff99566c3 to 88954ef8f36d (1 revision) (flutter/flutter#189440)
2026-07-14 [email protected] refactor: remove material import from scrollable_semantics_test and selectable_region_context_menu_test (flutter/flutter#186611)
2026-07-14 [email protected] Roll Skia from 3d1fc554f1a2 to dfcff99566c3 (17 revisions) (flutter/flutter#189428)
2026-07-14 [email protected] Roll Dart SDK from 2c587df8f05a to 05bf153370c4 (5 revisions) (flutter/flutter#189426)
2026-07-14 [email protected] [flutter_tools] Remove web hot reload flag (flutter/flutter#185994)
2026-07-14 [email protected] [iOS] Fix flaky keyboard animation test (flutter/flutter#189353)
2026-07-14 [email protected] [Impeller] Playground expanded role (flutter/flutter#188889)
2026-07-14 [email protected] Roll pub packages (flutter/flutter#189409)
2026-07-13 [email protected] Update lock-threads dependency to 6.0.2 (flutter/flutter#189053)
2026-07-13 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 6.1.0 to 6.2.0 in the all-github-actions group (flutter/flutter#189396)
2026-07-13 [email protected] Remove  outdated todo about `analysis bug on Windows` and update condition to also perform `analysis on windows` (flutter/flutter#189283)
2026-07-13 [email protected] Add more 0x0 size tests part 4 (flutter/flutter#185187)
2026-07-13 [email protected] Roll Packages from 20928d5 to ad2eab1 (18 revisions) (flutter/flutter#189387)
2026-07-13 [email protected] [flutter_tools] Fix ADB device listing output parsing regression (flutter/flutter#189369)
2026-07-13 [email protected] Stop running most Mac x64 builders that have Mac ARM equivalents on master (flutter/flutter#189301)
2026-07-13 [email protected] Move a few benchmarks from x64 Intel Macs to ARM (flutter/flutter#189377)
2026-07-13 [email protected] Add note that `hcpp` needs impeller (flutter/flutter#189382)
2026-07-13 [email protected] Roll Fuchsia Linux SDK from vhIlDkWIy21IrlB9E... to oOETA0ISPouDt2xBo... (flutter/flutter#189349)
2026-07-13 [email protected] [flutter_tools] Respect mustMatchAppBuild on Windows native assets (flutter/flutter#186788)
2026-07-13 [email protected] Roll Skia from 8bf65996caba to 3d1fc554f1a2 (2 revisions) (flutter/flutter#189350)
2026-07-13 [email protected] Roll Dart SDK from 0fc1668c4af4 to 2c587df8f05a (9 revisions) (flutter/flutter#189351)
2026-07-13 [email protected] [web] Fall back to full CJK fonts for characters not covered by split slices (flutter/flutter#188890)
2026-07-13 [email protected] Take Mac tool_integration_tests_* out of bringup (flutter/flutter#189368)
2026-07-13 [email protected] [hooks] Roll record_use to 1.0 and unpin (flutter/flutter#189366)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD engine flutter/engine related. See also e: labels. platform-ios iOS applications specifically team-ios Owned by iOS platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants