refactor: remove material in reorderable_list_test, scroll_notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test#182698
Conversation
There was a problem hiding this comment.
Code Review
The pull request successfully removes Material library dependencies from several widget tests by replacing MaterialApp with TestWidgetsApp, Scrollbar with RawScrollbar, and Material-specific widgets like Scaffold or Checkbox with widgets-layer equivalents. The refactoring of the Drawer regression test in reorderable_list_test.dart and the Checkbox logic in shortcuts_test.dart are particularly well-handled, maintaining the test's intent while decoupling from Material. One minor improvement is suggested to use a constant for the magic hex code used for the drag handle icon to improve readability and consistency.
| const _kRedColor = Color(0xFFFF0000); | ||
| const _kGreenColor = Color(0xFF00FF00); |
There was a problem hiding this comment.
Since you are defining constants for colors used in the tests, consider also defining a constant for the IconData used for the drag handle (hex 0xe25d). This would improve readability and avoid repeating the magic hex value and font family string in multiple places (lines 1780 and 2177).
| const _kRedColor = Color(0xFFFF0000); | |
| const _kGreenColor = Color(0xFF00FF00); | |
| const _kRedColor = Color(0xFFFF0000); | |
| const _kGreenColor = Color(0xFF00FF00); | |
| const _kDragHandleIconData = IconData(0xe25d, fontFamily: 'MaterialIcons'); |
References
- Optimize for readability: Code is read more often than it is written. (link)
| children: <Widget>[ | ||
| Text('item ${items[index]}'), | ||
| const Icon(Icons.drag_handle), | ||
| const Icon(IconData(0xe25d, fontFamily: 'MaterialIcons')), |
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: <Widget>[ | ||
| Text('item ${items[index]}'), | ||
| const Icon(IconData(0xe25d, fontFamily: 'MaterialIcons')), |
049e78c to
7bac882
Compare
| children: <Widget>[ | ||
| Text('item ${items[index]}'), | ||
| const Icon(Icons.drag_handle), | ||
| const Icon(IconData(0xe25d, fontFamily: 'MaterialIcons')), |
| onTap: () => setState(() { | ||
| showList = false; | ||
| }), | ||
| child: const Text('Close drawer'), |
There was a problem hiding this comment.
Are we still working with drawers?
There was a problem hiding this comment.
No, we are not, I will replace with Close list or hide list.
| expect(notification, isNull); | ||
|
|
||
| final TestGesture dragScrollbarGesture = await tester.startGesture(const Offset(790, 45)); | ||
| final TestGesture dragScrollbarGesture = await tester.startGesture(const Offset(797, 45)); |
There was a problem hiding this comment.
Can you explain the numbers change?
There was a problem hiding this comment.
If i am not wrong then Material Scrollbar positioned the thumb center around x=790 (800 - some internal padding) but RawScrollbar positions the thumb center closer to x=797 (800 - half the default thumb width, approximately)
| expect(notification, isA<ScrollStartNotification>()); | ||
|
|
||
| await dragScrollbarGesture.moveBy(const Offset(0, -20)); | ||
| await dragScrollbarGesture.moveBy(const Offset(0, 10)); |
There was a problem hiding this comment.
Why are we now moving by positive dy instead of negative as in existing code?
There was a problem hiding this comment.
The drag direction changed from - to + because we replaced Scaffold + Scrollbar with a plain RawScrollbar. The Scaffold added extra layout layers that shifted the scrollbar track's coordinate space, causing a negative drag delta to map to a positive scroll offset. Without Scaffold, the coordinate mapping is straightforward — dragging down (+dy) scrolls down i.e. increases offset, which is the natural scroll. The magnitude also changed to half because removing Scaffold altered the viewport dimensions and thus the drag-to-scroll ratio.
| bool? value = true; | ||
| Widget buildApp() { | ||
| return MaterialApp( | ||
| return WidgetsApp( |
There was a problem hiding this comment.
Why can't we use TestWidgetsApp here?
There was a problem hiding this comment.
Yup, we can. Replacing.
| // A focusable widget that handles ActivateIntent, | ||
| // replacing Checkbox for this widgets-layer test. |
There was a problem hiding this comment.
| // A focusable widget that handles ActivateIntent, | |
| // replacing Checkbox for this widgets-layer test. | |
| // A focusable widget that handles ActivateIntent. |
We should not specify what it is replacing, future code readers will not see a Checkbox here and will get confused. If more code changes happen here, it could be hard to find the PR that added the Checkbox in the first place. So it is just breadcrumbing to an irrelevant implementation detail, wasting developer time.
Instead we want to guarantee that the behavior reinforced by the test is preserved. As long as that is guaranteed implementation details are free to change.
c227bbc to
062c9a9
Compare
062c9a9 to
65d120f
Compare
…on_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test
65d120f to
1045667
Compare
victorsanni
left a comment
There was a problem hiding this comment.
Thanks for the work here!
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
…notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698)
Roll Flutter from 46fb7210422d to d3dd7744e81f (33 revisions) flutter/flutter@46fb721...d3dd774 2026-03-04 15619084+vashworth@users.noreply.github.com Show warning when plugins do not support SwiftPM (flutter/flutter#182506) 2026-03-04 15619084+vashworth@users.noreply.github.com Give guided message when project is not compatible with SwiftPM (flutter/flutter#182394) 2026-03-04 danny@tuppeny.com Pass --web-define through to web runner when using --machine mode (flutter/flutter#183228) 2026-03-04 104147021+MohammedTarigg@users.noreply.github.com Improve SwiftPM minimum platform mismatch diagnostics (flutter/flutter#182375) 2026-03-04 vegorov@google.com Use dart::bin::SetupDartIo to setup dart:io (flutter/flutter#176714) 2026-03-04 engine-flutter-autoroll@skia.org Roll Skia from 3197848b14ad to ada0b7628c79 (5 revisions) (flutter/flutter#183221) 2026-03-04 engine-flutter-autoroll@skia.org Roll Skia from fe9e9f22c531 to 3197848b14ad (15 revisions) (flutter/flutter#183198) 2026-03-04 34465683+rkishan516@users.noreply.github.com refactor: remove material in reorderable_list_test, scroll_notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test (flutter/flutter#182698) 2026-03-04 34465683+rkishan516@users.noreply.github.com refactor: remove material in pop_scope_test, route_notification_message_test, two_dimensional_utils, two_dimensional_viewport_test (flutter/flutter#182699) 2026-03-04 737941+loic-sharma@users.noreply.github.com Add dev/benchmarks/README.md (flutter/flutter#182976) 2026-03-03 jason-simmons@users.noreply.github.com Roll RapidJSON to a branch based on the current upstream head (flutter/flutter#183048) 2026-03-03 flar@google.com [Impeller] Update comments to reflect new info about 2-pass rendering (flutter/flutter#183050) 2026-03-03 47866232+chunhtai@users.noreply.github.com Add vmservices for accessibilityEvaluation (flutter/flutter#182791) 2026-03-03 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 0dCDM2oORHwDf_pyb... to JJw5EJ87vLGqFVl4h... (flutter/flutter#183177) 2026-03-03 lukas.klingsbo@gmail.com Support mixed color spaces in `Color.lerp` (flutter/flutter#182934) 2026-03-03 36861262+QuncCccccc@users.noreply.github.com Add warning when there is a widget with color between `Material` and `ListTile` (flutter/flutter#181402) 2026-03-03 41930132+hellohuanlin@users.noreply.github.com [ios]uitest for admob banner in scrollable list gesture issue (flutter/flutter#183128) 2026-03-03 engine-flutter-autoroll@skia.org Roll Packages from faa4e22 to 9083bc9 (4 revisions) (flutter/flutter#183164) 2026-03-03 15619084+vashworth@users.noreply.github.com Build App and native asset frameworks for Add to App FlutterPluginRegistrant (flutter/flutter#183136) 2026-03-03 engine-flutter-autoroll@skia.org Roll Skia from f886711f180d to fe9e9f22c531 (4 revisions) (flutter/flutter#183155) 2026-03-03 engine-flutter-autoroll@skia.org Roll Dart SDK from e86dbe9aa742 to c597ef90d2dc (2 revisions) (flutter/flutter#183147) 2026-03-03 codefu@google.com fix: bump matcher (flutter/flutter#183167) 2026-03-02 58529443+srujzs@users.noreply.github.com Use isA to test for exceptions (flutter/flutter#183129) 2026-03-02 dpxhfxywang@163.com [two_dimensional_scrollables] Fix tableview janks when first row/column pinned (flutter/flutter#180563) 2026-03-02 victorsanniay@gmail.com Add await to callsites of BasicMessageChannel.send (flutter/flutter#182868) 2026-03-02 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#183133) 2026-03-02 robert.ancell@canonical.com Improve FFI code for windowing (flutter/flutter#183098) 2026-03-02 kevinjchisholm@google.com [workflow] Update the changelog merge action to fetch the stable branch (flutter/flutter#183132) 2026-03-02 engine-flutter-autoroll@skia.org Roll Skia from e180358b7a7a to f886711f180d (2 revisions) (flutter/flutter#183130) 2026-03-02 jacksongardner@google.com Merge changelog from 3.41.3. (flutter/flutter#183131) 2026-03-02 50985133+SpiralMomentum@users.noreply.github.com Make TextDecoration final and unify maskValue across platforms (flutter/flutter#183070) 2026-03-02 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#182640) 2026-03-02 15619084+vashworth@users.noreply.github.com Enable SwiftPM by default on master and beta (flutter/flutter#182923) 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 bmparr@google.com,stuartmorgan@google.com 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: ...
This PR removes Material imports from reorderable_list_test, scroll_notification_test, scroll_physics_test, shortcuts_test, sliver_floating_header_test, snapshot_widget_test.
part of: #177415
depends on: #182805
Pre-launch Checklist
///).