Repository navigation
feat(flutter_test): add startHover and startHoverAt helper to WidgetTester - #190630
Conversation
github-actions
Bot
added
a: tests
b9fb4d0 to
26c31b7
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces startHover and startHoverAt methods to WidgetController to support dispatching hover sequences during widget testing, and updates TestPointer to respect active buttons during hover events. Feedback suggests expanding the newly added test to fully cover both new APIs and to verify non-default pointer kinds like PointerDeviceKind.stylus as indicated by the test's name.
26c31b7 to
7d09f09
Compare
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
Hi @Mathvdias, I was trying to re-review but I saw no latest commits. Maybe still local and you forgot to push? Please re-request a review once you have updated the PR with the latest changes.
7d09f09 to
fd95b72
Compare
|
Hi @victorsanni, apologies for that! The changes were indeed sitting in my local working tree. I have now pushed the updated commit (and rebased on latest master). Please take another look when you have a moment. |
|
Hi @Mathvdias, please remember to re-request reviews when you have pushed a commit. Else it doesn't appear on my review queue and so I never see your PR to review. |
|
Hi @victorsanni, thank you so much for the detailed feedback and for your patience! I apologize for the delay and for forgetting to explicitly re-request the review earlier. I've just pushed a new commit addressing all your points:
I've re-requested a review. Please take a look when you have some time and let me know if everything looks good to land! |
| /// | ||
| /// * [press], which dispatches a pointer down sequence. | ||
| /// * [startHoverAt], which starts a hover gesture at a specific location. | ||
| /// {@macro flutter.flutter_test.WidgetController.tap.warnIfMissed} |
There was a problem hiding this comment.
See also is conventionally the last section in API docs, can you apply the macro where it would be interpolating from the other API docs in this file?
There was a problem hiding this comment.
Done! Moved the macro right after the first paragraph and kept See also at the end.
| kind: kind, | ||
| pointer: pointer ?? _getNextPointer(), | ||
| pointer: p, | ||
| device: kind == PointerDeviceKind.mouse ? p : null, |
There was a problem hiding this comment.
this will affect every mouse gesture and may break tests. I think it is fine if multi-hover is not enabled for now as long as it is documented (and asserted).
There was a problem hiding this comment.
Reverted _createGesture and added assert(!binding.mouseTracker.mouseIsConnected, ...) along with docs and a test.
| PointerDeviceKind kind = PointerDeviceKind.mouse, | ||
| FlutterView? view, | ||
| }) { | ||
| assert(kind != PointerDeviceKind.trackpad); |
There was a problem hiding this comment.
should also not be .touch right? can't hover with a finger.
Also this should have an accompanying message.
There was a problem hiding this comment.
Done! Updated the assert to disallow both .touch and .trackpad with a message.
| expect(logs, equals(<String>['down $b'])); | ||
| }); | ||
|
|
||
| testWidgets('WidgetTester.startHover and startHoverAt must respect kinds and allow click sequences', ( |
There was a problem hiding this comment.
is there a multi-view test? and a test for pointer?
There was a problem hiding this comment.
Added tests for both pointer and multi-view.
| ); | ||
|
|
||
| final TestGesture gesture = await tester.startHover(find.text('test')); | ||
| addTearDown(() => gesture.removePointer()); |
There was a problem hiding this comment.
Why not just addTearDown(gesture.removePointer) directly? And everywhere else
There was a problem hiding this comment.
Updated to use addTearDown(gesture.removePointer) everywhere.
| ); | ||
| assert( | ||
| !binding.mouseTracker.mouseIsConnected, | ||
| 'A hover pointer is already connected. Call removePointer() on the ' |
There was a problem hiding this comment.
mouseIsConnected becomes true after any mouse/stylus event, not just hovers. The real precondition is no mouse/stylus device already known to MouseTracker, e.g. from tap/press/drag with kind: mouse.
There was a problem hiding this comment.
also a test for this path
There was a problem hiding this comment.
Done — updated the assertion message to clarify that no mouse or stylus device may already be connected to MouseTracker (including from prior tap, press, or drag calls), and added a test for calling startHover after tap(..., kind: PointerDeviceKind.mouse).
| isA<AssertionError>().having( | ||
| (AssertionError e) => e.message, | ||
| 'message', | ||
| 'Touch and trackpad pointers cannot generate hover events.', |
There was a problem hiding this comment.
maybe we can use contains to make this assertion less brittle?
There was a problem hiding this comment.
Done — switched to contains.
| isA<AssertionError>().having( | ||
| (AssertionError e) => e.message, | ||
| 'message', | ||
| 'A hover pointer is already connected. Call removePointer() on the ' |
There was a problem hiding this comment.
Done — switched to contains here too.
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
LGTM with a final comment.
Thanks for the new APIs, should we open a tracking issue to migrate framework tests to use it? We have had problems (especially recently) with pointers not being disposed correctly leading to state persistence across tests and test failures (with arbitrary test orderings), this might help there.
|
Thanks for the review @victorsanni! Addressed the final comment and opened #194600 to track migrating framework tests to |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM with a small docs nit. Thanks for making this kind of thing easier!
| /// No mouse or stylus device may already be connected to [MouseTracker] when | ||
| /// this method is called (for example, from an active hover gesture or a prior |
There was a problem hiding this comment.
What happens if a mouse/stylus is connected to MouseTracker?
There was a problem hiding this comment.
My question was answered by the tests: it triggers an assertion. I guess the docs here are good as-is.
| /// Dispatch a hover sequence at the center of the given widget, assuming it is | ||
| /// exposed. |
There was a problem hiding this comment.
Nit: By exposed, do you mean visible on screen and able to receive pointer events? What happens if it is not exposed? Maybe explain below somewhere.
There was a problem hiding this comment.
Good question! This phrasing ("assuming it is exposed" followed by {@macro flutter.flutter_test.WidgetController.tap.warnIfMissed}) mirrors tap, press, and longPress right above in WidgetController.
In the generated Dartdoc, that {@macro flutter.flutter_test.WidgetController.tap.warnIfMissed} expands right below the first sentence to explain both what "exposed" means and what happens when it is not:
"The
warnIfMissedargument, if true (the default), causes a warning to be displayed on the console if the specified [Finder] indicates a widget and location that, were a pointer event to be sent to that location, would not actually send any events to the widget (e.g. because the widget is obscured, or the location is off-screen, or the widget is transparent to pointer events)."
Happy to add extra wording if you'd still prefer, otherwise keeping it as-is avoids duplicating that paragraph in the generated docs and preserves the current green CI run!
| Future<void> addPointer({ | ||
| Duration timeStamp = Duration.zero, | ||
| Offset? location, | ||
| FlutterView? view, |
There was a problem hiding this comment.
I agree we should be piping the FlutterView through, good call.
victorsanni
added
waiting for response
|
Thanks for the review @justinmc, and thanks for merging Replied to the inline docs nit ( |
github-actions
Bot
removed
the
waiting for response
victorsanni
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
flutter/flutter@9347202...3623d2d 2026-11-04 [email protected] Roll Skia from 49ec2655bf51 to aaa39bfcd393 (5 revisions) (flutter/flutter#195956) 2026-11-04 [email protected] Roll pub packages (flutter/flutter#195950) 2026-11-04 [email protected] Roll pub packages (flutter/flutter#195948) 2026-11-04 [email protected] local_engine_builds_common out of bringup (flutter/flutter#195862) 2026-11-04 [email protected] [web][a11y] Initialize slider semantics values when enabling (flutter/flutter#195564) 2026-11-04 [email protected] Drop Android SDK, JDK, and Gradle dependencies from framework_tests_misc (flutter/flutter#195869) 2026-11-04 [email protected] Roll Fuchsia Test Scripts from B6zarykZp6PKWRueg... to Jo-ZgQSNDuPsjlZ4s... (flutter/flutter#195937) 2026-11-04 [email protected] Balance tool_tests_commands and web_tool_tests subshards by duration (flutter/flutter#195863) 2026-11-04 [email protected] Roll Fuchsia Linux SDK from T_-6-6X1lDMghhAvX... to p0HDXLISPIElJfSz7... (flutter/flutter#195939) 2026-11-03 [email protected] Roll Skia from 3eeafac94f07 to 49ec2655bf51 (8 revisions) (flutter/flutter#195933) 2026-11-03 [email protected] Roll pub packages (flutter/flutter#195932) 2026-11-03 [email protected] [flutter_tools] Lazily initialize AppleContext dependencies via builder closures (flutter/flutter#191992) 2026-11-03 [email protected] [Android] Upgrade linux_android_emu_vulkan_stable avd_cipd_version to 36.1.9 (build_id:8670032186093418337) (flutter/flutter#195856) 2026-11-03 [email protected] [vulkan]: adds "color attachment read" dst dependency to subpass (flutter/flutter#195714) 2026-11-03 [email protected] [flutter_tools] Stream web dev server proxy responses instead of buffering them (flutter/flutter#194934) 2026-11-03 [email protected] Roll Skia from d80db8f8f856 to 3eeafac94f07 (6 revisions) (flutter/flutter#195926) 2026-11-03 [email protected] [flutter_tools] Add flutter assemble support for tool extension assemble targets (flutter/flutter#194563) 2026-11-03 [email protected] Manual roll Dart SDK from fe609902984b to e362a23e9738 (2 revisions) (flutter/flutter#195914) 2026-11-03 [email protected] feat(flutter_test): add startHover and startHoverAt helper to WidgetTester (flutter/flutter#190630) 2026-11-03 [email protected] [flutter_tools] Remove trailing dots from bug report URLs (flutter/flutter#194132) 2026-11-03 [email protected] Update skills_lint to 0.5.3 (flutter/flutter#195796) 2026-11-03 [email protected] Roll Skia from 58295d932350 to d80db8f8f856 (5 revisions) (flutter/flutter#195900) 2026-11-03 [email protected] [flutter_tools] Make DeviceLogReader.dispose asynchronous (flutter/flutter#195389) 2026-11-03 [email protected] Roll Packages from 3f34a6f to 1df8447 (10 revisions) (flutter/flutter#195895) 2026-11-03 [email protected] Roll Skia from c733ad3e37cb to 58295d932350 (1 revision) (flutter/flutter#195894) If this roll has caused a breakage, revert this CL and set the roller to dry run mode 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
Addresses #102754 by adding startHover and startHoverAt helpers to WidgetTester to simplify hover gesture simulation in tests. Also fixes TestPointer.hover to pass down _buttons.