refactor: remove material from widget_inspector_test, sliver_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test#182702
Conversation
There was a problem hiding this comment.
Code Review
The pull request successfully refactors several widget tests to remove dependencies on the material library, which is a great step towards better library decoupling. The use of TestWidgetsApp and the replacement of Material-specific widgets with generic ones like GestureDetector and Directionality is consistent and correct. I have a few suggestions regarding test isolation and redundancy in scrollable_fling_test.dart to ensure global state is properly reset even on test failure.
| @@ -860,7 +863,7 @@ void main() { | |||
| ); | |||
|
|
|||
| testWidgets( | |||
There was a problem hiding this comment.
Can we add the original test(s) in SliverAppBar tests over in Material?
There was a problem hiding this comment.
I was thinking to move SliverAppBar to sliver.dart, because thats the only sliver stuck inside material.
There was a problem hiding this comment.
Ah that could be a good idea, it looks like SliverAppBar does not depend on AppBar or anything else Material? I wonder what @Piinks thinks.
For another PR though.
There was a problem hiding this comment.
Yup, i will create another PR.
There was a problem hiding this comment.
Have created #182925, SliverAppBar actually depends quite on material, So made new version as RawSliverAppBar.
| return Stack( | ||
| children: <Widget>[ | ||
| const Icon(Icons.watch), | ||
| const Icon(IconData(0xe6ce, fontFamily: 'MaterialIcons')), |
There was a problem hiding this comment.
Is there a constant from the widget inspector that we can leverage, instead of the magic number?
There was a problem hiding this comment.
I don't think there is. But will check.
8f02d93 to
de8ad46
Compare
justinmc
left a comment
There was a problem hiding this comment.
Thanks @rkishan516! Heads up that there are test failures.
|
|
||
| void main() { | ||
| testWidgets('Flings on different platforms', (WidgetTester tester) async { | ||
| addTearDown(() => debugDefaultTargetPlatformOverride = null); |
There was a problem hiding this comment.
I would put this teardown inside of pumpTest to avoid some future test calling pumpTest and forgetting to include the teardown.
There was a problem hiding this comment.
Sure, will move.
| @@ -860,7 +863,7 @@ void main() { | |||
| ); | |||
|
|
|||
| testWidgets( | |||
There was a problem hiding this comment.
Ah that could be a good idea, it looks like SliverAppBar does not depend on AppBar or anything else Material? I wonder what @Piinks thinks.
For another PR though.
| _CreationLocation location = knownLocations[id]!; | ||
| expect(location.file, equals(file)); | ||
| // ClockText widget. | ||
| expect(location.line, equals(57)); |
There was a problem hiding this comment.
I'm not sure exactly why that would be off the top of my head.
de8ad46 to
0d0de1d
Compare
0d0de1d to
8cde0f9
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| expect(linuxResult, equals(androidResult)); | ||
| expect(linuxResult, equals(androidResult)); | ||
|
|
||
| debugDefaultTargetPlatformOverride = null; |
There was a problem hiding this comment.
This can be removed? We have a teardown now
There was a problem hiding this comment.
I think this is required because between variants teardown doesn't run.
| final String details = service.getDetailsSubtree(objectId, 'foo2'); | ||
| // ignore: avoid_dynamic_calls | ||
| final detailedChildren = json.decode(details)['children'] as List<Object?>; | ||
| final detailedChildren = |
| final scaffold = childrenOfMaterialApp.first! as Map<String, Object?>; | ||
| expect(scaffold['description'], 'Scaffold'); | ||
| final objectId = scaffold['valueId']! as String; | ||
| final childrenOfRoot = |
There was a problem hiding this comment.
This can probably be cleaned up using a switch expression? The double forced cast makes it look a little worse now
There was a problem hiding this comment.
I feel this pattern is being used at multiple places and converting to switch expression adds more verbosity but not clarity. WDYT?
There was a problem hiding this comment.
Either way is good for me.
| }); | ||
| } | ||
|
|
||
| class _TabScaffoldWidgetInspectorService extends TestWidgetInspectorService { |
There was a problem hiding this comment.
This is really odd for several reasons?
- TestWidgetInspectorService is public, you don't need to subclass it to access
resetAllState()? That method is visibleForTests, so I think you can dofinal service = TestWidgetInspectorService();? - Can you put the set up / tear down directly inside the test
ext.flutter.inspector.getLayoutExplorerNode does not throw StackOverflowError service.disposeAllGroups();can be put in the teardown? (or if resetAllState() call it, then you can omit it?)
There was a problem hiding this comment.
If i am not wrong
While resetAllState() is indeed @VisibleForTesting and doesn't require a subclass, the subclass is needed for other reasons: WidgetInspectorService.instance (setter), setSelection, and toId are all @Protected, so they can only be accessed from within a subclass of WidgetInspectorService.
For the same very reason, WidgetInspectorService.instance = service must be called from within the subclass scope (the setter is @Protected), so the runTests() static method pattern is necessary.
You're right on this one. resetAllState() already calls disposeAllGroups() as its first line, so the explicit call at the end of the test was redundant. Removing it.
8cde0f9 to
cea365b
Compare
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
cea365b to
bd35a9c
Compare
| final service = _TabScaffoldWidgetInspectorService(); | ||
| WidgetInspectorService.instance = service; | ||
|
|
||
| tearDown(() { | ||
| service.resetAllState(); | ||
| }); |
There was a problem hiding this comment.
Should you also reset WidgetInspectorService.instance to whatever it was before?
There was a problem hiding this comment.
Updated by saving previous instance.
| debugDefaultTargetPlatformOverride = platform; | ||
| addTearDown(() => debugDefaultTargetPlatformOverride = null); |
There was a problem hiding this comment.
I was going to suggest just using a variant instead of debugDefaultTargetPlatformOverride, but since I see the test compares the results between different platforms, I think keeping it as-is is the best bet that I can think of.
| final scaffold = childrenOfMaterialApp.first! as Map<String, Object?>; | ||
| expect(scaffold['description'], 'Scaffold'); | ||
| final objectId = scaffold['valueId']! as String; | ||
| final childrenOfRoot = |
There was a problem hiding this comment.
Either way is good for me.
| } | ||
|
|
||
| class _TabScaffoldWidgetInspectorService extends TestWidgetInspectorService { | ||
| static void runTests() { |
There was a problem hiding this comment.
Can you include a comment similar to the one in the original widget_inspector_test.dart version? "These tests need access to protected members of WidgetInspectorService."
529a998 to
6cf3360
Compare
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
16185d6 to
0bbdaba
Compare
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
justinmc
left a comment
There was a problem hiding this comment.
There was 1 failing golden and I've approved it. It was just a very slight color change.
…r_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter/flutter#182702)
…r_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter/flutter#182702)
…r_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter/flutter#182702)
…r_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter/flutter#182702)
…r_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter/flutter#182702)
…r_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter/flutter#182702)
flutter/flutter@d182143...2ec61af 2026-03-08 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from P9D6h4D2ks99Ct7TO... to giLoee6arX5CRHuRh... (flutter/flutter#183366) 2026-03-07 engine-flutter-autoroll@skia.org Roll Skia from 6643c1bd93bb to af994ae4d990 (1 revision) (flutter/flutter#183359) 2026-03-07 34465683+rkishan516@users.noreply.github.com refactor: remove material import from animated_cross_fade, physical_model_test, pinned_header_sliver_test, spell_check_test (flutter/flutter#183234) 2026-03-07 engine-flutter-autoroll@skia.org Roll Skia from a69ef43650ee to 6643c1bd93bb (13 revisions) (flutter/flutter#183346) 2026-03-07 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#183344) 2026-03-07 engine-flutter-autoroll@skia.org Roll Dart SDK from 0c24edc41e09 to 1604910613c7 (2 revisions) (flutter/flutter#183342) 2026-03-07 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from nR2ESa1Gd8yPcWo06... to R2EllDf4DgBXVNuiN... (flutter/flutter#183341) 2026-03-07 97480502+b-luk@users.noreply.github.com Support BGRA textures in BlitCopyTextureToBufferCommandGLES::Encode and fix improper mapping of BGRA to RGBA in blit_command_gles and texture_gles (flutter/flutter#182397) 2026-03-06 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 8ay15_eQOEgPHCypm... to P9D6h4D2ks99Ct7TO... (flutter/flutter#183329) 2026-03-06 41930132+hellohuanlin@users.noreply.github.com [doc]add discord channel to ios triage meeting (flutter/flutter#183285) 2026-03-06 engine-flutter-autoroll@skia.org Roll Dart SDK from 7c7c1e3d024d to 0c24edc41e09 (2 revisions) (flutter/flutter#183328) 2026-03-06 43054281+camsim99@users.noreply.github.com Revert "Make HCPP upgrading work for vd/tlhc (#181024)" (flutter/flutter#183310) 2026-03-06 stuartmorgan@google.com Add AI contribution guidelines (flutter/flutter#183326) 2026-03-06 jason-simmons@users.noreply.github.com [Impeller] Do not wait for a frame's acquire fence if the frame was never presented (flutter/flutter#183288) 2026-03-06 jacksongardner@google.com Add back in accidentally removed line from `create_updated_flutter_deps.py` (flutter/flutter#183314) 2026-03-06 matt.boetger@gmail.com Fix typo in README (flutter/flutter#183245) 2026-03-06 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#183319) 2026-03-06 sokolovskyi.konstantin@gmail.com Add displayCornerRadii support to predictive back transitions. (flutter/flutter#181326) 2026-03-06 34465683+rkishan516@users.noreply.github.com refactor: remove material from widget_inspector_test, sliver_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter/flutter#182702) 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: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…is_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter#182702) This PR removes Material imports from widget_inspector_test, sliver_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test. part of: flutter#177415 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [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.
…r#11202) flutter/flutter@d182143...2ec61af 2026-03-08 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from P9D6h4D2ks99Ct7TO... to giLoee6arX5CRHuRh... (flutter/flutter#183366) 2026-03-07 engine-flutter-autoroll@skia.org Roll Skia from 6643c1bd93bb to af994ae4d990 (1 revision) (flutter/flutter#183359) 2026-03-07 34465683+rkishan516@users.noreply.github.com refactor: remove material import from animated_cross_fade, physical_model_test, pinned_header_sliver_test, spell_check_test (flutter/flutter#183234) 2026-03-07 engine-flutter-autoroll@skia.org Roll Skia from a69ef43650ee to 6643c1bd93bb (13 revisions) (flutter/flutter#183346) 2026-03-07 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#183344) 2026-03-07 engine-flutter-autoroll@skia.org Roll Dart SDK from 0c24edc41e09 to 1604910613c7 (2 revisions) (flutter/flutter#183342) 2026-03-07 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from nR2ESa1Gd8yPcWo06... to R2EllDf4DgBXVNuiN... (flutter/flutter#183341) 2026-03-07 97480502+b-luk@users.noreply.github.com Support BGRA textures in BlitCopyTextureToBufferCommandGLES::Encode and fix improper mapping of BGRA to RGBA in blit_command_gles and texture_gles (flutter/flutter#182397) 2026-03-06 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 8ay15_eQOEgPHCypm... to P9D6h4D2ks99Ct7TO... (flutter/flutter#183329) 2026-03-06 41930132+hellohuanlin@users.noreply.github.com [doc]add discord channel to ios triage meeting (flutter/flutter#183285) 2026-03-06 engine-flutter-autoroll@skia.org Roll Dart SDK from 7c7c1e3d024d to 0c24edc41e09 (2 revisions) (flutter/flutter#183328) 2026-03-06 43054281+camsim99@users.noreply.github.com Revert "Make HCPP upgrading work for vd/tlhc (#181024)" (flutter/flutter#183310) 2026-03-06 stuartmorgan@google.com Add AI contribution guidelines (flutter/flutter#183326) 2026-03-06 jason-simmons@users.noreply.github.com [Impeller] Do not wait for a frame's acquire fence if the frame was never presented (flutter/flutter#183288) 2026-03-06 jacksongardner@google.com Add back in accidentally removed line from `create_updated_flutter_deps.py` (flutter/flutter#183314) 2026-03-06 matt.boetger@gmail.com Fix typo in README (flutter/flutter#183245) 2026-03-06 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#183319) 2026-03-06 sokolovskyi.konstantin@gmail.com Add displayCornerRadii support to predictive back transitions. (flutter/flutter#181326) 2026-03-06 34465683+rkishan516@users.noreply.github.com refactor: remove material from widget_inspector_test, sliver_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter/flutter#182702) 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: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…is_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter#182702) This PR removes Material imports from widget_inspector_test, sliver_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test. part of: flutter#177415 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [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.
…is_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter#182702) This PR removes Material imports from widget_inspector_test, sliver_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test. part of: flutter#177415 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [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.
…r#11202) flutter/flutter@d182143...2ec61af 2026-03-08 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from P9D6h4D2ks99Ct7TO... to giLoee6arX5CRHuRh... (flutter/flutter#183366) 2026-03-07 engine-flutter-autoroll@skia.org Roll Skia from 6643c1bd93bb to af994ae4d990 (1 revision) (flutter/flutter#183359) 2026-03-07 34465683+rkishan516@users.noreply.github.com refactor: remove material import from animated_cross_fade, physical_model_test, pinned_header_sliver_test, spell_check_test (flutter/flutter#183234) 2026-03-07 engine-flutter-autoroll@skia.org Roll Skia from a69ef43650ee to 6643c1bd93bb (13 revisions) (flutter/flutter#183346) 2026-03-07 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#183344) 2026-03-07 engine-flutter-autoroll@skia.org Roll Dart SDK from 0c24edc41e09 to 1604910613c7 (2 revisions) (flutter/flutter#183342) 2026-03-07 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from nR2ESa1Gd8yPcWo06... to R2EllDf4DgBXVNuiN... (flutter/flutter#183341) 2026-03-07 97480502+b-luk@users.noreply.github.com Support BGRA textures in BlitCopyTextureToBufferCommandGLES::Encode and fix improper mapping of BGRA to RGBA in blit_command_gles and texture_gles (flutter/flutter#182397) 2026-03-06 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 8ay15_eQOEgPHCypm... to P9D6h4D2ks99Ct7TO... (flutter/flutter#183329) 2026-03-06 41930132+hellohuanlin@users.noreply.github.com [doc]add discord channel to ios triage meeting (flutter/flutter#183285) 2026-03-06 engine-flutter-autoroll@skia.org Roll Dart SDK from 7c7c1e3d024d to 0c24edc41e09 (2 revisions) (flutter/flutter#183328) 2026-03-06 43054281+camsim99@users.noreply.github.com Revert "Make HCPP upgrading work for vd/tlhc (#181024)" (flutter/flutter#183310) 2026-03-06 stuartmorgan@google.com Add AI contribution guidelines (flutter/flutter#183326) 2026-03-06 jason-simmons@users.noreply.github.com [Impeller] Do not wait for a frame's acquire fence if the frame was never presented (flutter/flutter#183288) 2026-03-06 jacksongardner@google.com Add back in accidentally removed line from `create_updated_flutter_deps.py` (flutter/flutter#183314) 2026-03-06 matt.boetger@gmail.com Fix typo in README (flutter/flutter#183245) 2026-03-06 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#183319) 2026-03-06 sokolovskyi.konstantin@gmail.com Add displayCornerRadii support to predictive back transitions. (flutter/flutter#181326) 2026-03-06 34465683+rkishan516@users.noreply.github.com refactor: remove material from widget_inspector_test, sliver_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test (flutter/flutter#182702) 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: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This PR removes Material imports from widget_inspector_test, sliver_cross_axis_group_test, editable_text_show_on_screen_test, scrollable_fling_test, selection_container_test.
part of: #177415
Pre-launch Checklist
///).