Remove unnecessary Material import from editable_text_scribe_test#185238
Closed
momshaddinury wants to merge 1 commit into
Closed
Remove unnecessary Material import from editable_text_scribe_test#185238momshaddinury wants to merge 1 commit into
momshaddinury wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the editable_text_scribe_test.dart file to remove its dependency on the Material library. The changes include replacing MaterialApp with TestWidgetsApp, substituting Colors.grey with a specific hex color value, and updating the selection controls to emptyTextSelectionControls. I have no feedback to provide.
navaronbracke
requested changes
Apr 20, 2026
This test is located in the Widgets layer and did not need to import package:flutter/material.dart. Replace the MaterialApp wrapper with TestWidgetsApp (from widgets_app_tester.dart), swap Colors.grey for its raw color value, and use emptyTextSelectionControls in place of materialTextSelectionControls. Part of flutter#177028
a81f7a4 to
2741a6c
Compare
10 tasks
navaronbracke
approved these changes
Apr 20, 2026
Contributor
|
@momshaddinury I wonder if you could add some more test-only cleanups like this one, in this pull request? We did have discussions around optimizing check runs for small pull requests in the past (to reduce load on CI). Your pull request is currently very tiny in terms of lines of code (~lines changed in total), but if I remember correctly the sweet spot is around ~300-500. Could you look into bundling some more fixes in this pull request? Feel free to request another review from myself using the Reviewers tab above, once you are ready to do so. |
Contributor
Author
|
@navaronbracke
If we implement these changes in this PR, the branch name may become misleading. Should I create a separate branch, or are you okay with keeping them in this branch? |
10 tasks
justinmc
approved these changes
Apr 29, 2026
Contributor
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍
Since these PRs are already created, let's just move forward with them as-is to avoid the async time lag in waiting for review again. I'll go ahead and review the other ones. In the future bundling a few together is probably more efficient but not a big deal.
Heads up there is a conflict that needs to be resolved here though.
Contributor
|
Closing as this was fixed in #184798. |
pull Bot
pushed a commit
to NOUIY/flutter
that referenced
this pull request
May 21, 2026
…r package (flutter#185271) Several tests under `packages/flutter/test/` were importing `package:flutter/material.dart` even though they only exercise APIs from `package:flutter/widgets.dart` (or lower layers like `painting`, `animation`, `gestures`). Pulling in Material unnecessarily increases cross-package coupling between these tests and the Material library and works against the cross-import hygiene enforced by `dev/bots/check_tests_cross_imports.dart`. This PR: - Switches the import from `package:flutter/material.dart` to `package:flutter/widgets.dart` in tests that do not reference any Material symbols. - Where tests referenced `Colors.*` only, replaces those with the equivalent raw `const Color(0x…)` values. - Where tests wrapped widgets in `MaterialApp` purely as a `Directionality`/`MediaQuery`/localization host, swaps `MaterialApp` for `TestWidgetsApp` from `widgets_app_tester.dart`, and replaces `materialTextSelectionControls` with `emptyTextSelectionControls`. - Removes the corresponding entry from the `check_tests_cross_imports.dart` allowlist where applicable. No production code is modified and no test semantics change — the tests exercise the same widget/painting/gesture/animation behavior, just without dragging in Material. ## Files changed Tests updated: - `packages/flutter/test/gestures/force_press_test.dart` - `packages/flutter/test/gestures/transformed_scale_test.dart` - `packages/flutter/test/widgets/default_text_editing_shortcuts_test.dart` - `packages/flutter/test/widgets/semantics_owner_test.dart` - `packages/flutter/test/animation/animation_sheet_test.dart` - `packages/flutter/test/painting/star_border_test.dart` - `packages/flutter/test/painting/paint_image_test.dart` Allowlist updated: - `dev/bots/check_tests_cross_imports.dart` ## Scope note — two files intentionally left out Two test files originally staged in this cleanup were dropped after local test runs showed they aren't a clean Material → widgets swap. Details and a request for guidance are in a separate PR comment below: - `packages/flutter/test/widgets/editable_text_scribe_test.dart` — the handle-tap tests rely on `CustomPaint` widgets painted by `materialTextSelectionControls`; `emptyTextSelectionControls` paints nothing, so the handle finders return 0 widgets. - `packages/flutter/test/widgets/editable_text_test.dart` (Live Text test only) — swapping `MaterialApp` → `TestWidgetsApp` triggers `A FocusManager was used after being disposed` in the shared `tearDown`, cascading into five downstream a11y/semantics tests. Both files remain on the `check_tests_cross_imports.dart` allowlist and can be picked up in follow-up PRs once we agree on the approach. ## Related issues - Part of umbrella issue flutter#177028 (Framework unit tests shouldn't cross-import libraries). - Also addresses flutter#177412 (No legacy Material imports in tests). ## Context — why one PR Per reviewer guidance from @navaronbracke on flutter#185238 ([comment](flutter#185238 (comment))), similar Material-import cleanups are being consolidated into a single PR that lands closer to the ~300–500-line "sweet spot" rather than as many tiny per-file PRs. ## Superseded PRs (please close once this lands) The following open PRs are subsumed by this one and can be closed if this PR is approved (see [this comment](flutter#185238 (comment))): - flutter#185236 — Remove unused material.dart import from force_press_test.dart - flutter#185241 — Remove unnecessary Material import from default_text_editing_shortcuts_test Not superseded (corresponding files were dropped from this PR — see the scope note above): - flutter#185238 — editable_text_scribe_test - flutter#185242 — editable_text_test Live Text ## 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]. <!-- test-only cleanup: no production code or test-behavior changes; existing coverage is preserved. --> - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- 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
matthewhendrix
pushed a commit
to matthewhendrix/flutter
that referenced
this pull request
May 23, 2026
…r package (flutter#185271) Several tests under `packages/flutter/test/` were importing `package:flutter/material.dart` even though they only exercise APIs from `package:flutter/widgets.dart` (or lower layers like `painting`, `animation`, `gestures`). Pulling in Material unnecessarily increases cross-package coupling between these tests and the Material library and works against the cross-import hygiene enforced by `dev/bots/check_tests_cross_imports.dart`. This PR: - Switches the import from `package:flutter/material.dart` to `package:flutter/widgets.dart` in tests that do not reference any Material symbols. - Where tests referenced `Colors.*` only, replaces those with the equivalent raw `const Color(0x…)` values. - Where tests wrapped widgets in `MaterialApp` purely as a `Directionality`/`MediaQuery`/localization host, swaps `MaterialApp` for `TestWidgetsApp` from `widgets_app_tester.dart`, and replaces `materialTextSelectionControls` with `emptyTextSelectionControls`. - Removes the corresponding entry from the `check_tests_cross_imports.dart` allowlist where applicable. No production code is modified and no test semantics change — the tests exercise the same widget/painting/gesture/animation behavior, just without dragging in Material. ## Files changed Tests updated: - `packages/flutter/test/gestures/force_press_test.dart` - `packages/flutter/test/gestures/transformed_scale_test.dart` - `packages/flutter/test/widgets/default_text_editing_shortcuts_test.dart` - `packages/flutter/test/widgets/semantics_owner_test.dart` - `packages/flutter/test/animation/animation_sheet_test.dart` - `packages/flutter/test/painting/star_border_test.dart` - `packages/flutter/test/painting/paint_image_test.dart` Allowlist updated: - `dev/bots/check_tests_cross_imports.dart` ## Scope note — two files intentionally left out Two test files originally staged in this cleanup were dropped after local test runs showed they aren't a clean Material → widgets swap. Details and a request for guidance are in a separate PR comment below: - `packages/flutter/test/widgets/editable_text_scribe_test.dart` — the handle-tap tests rely on `CustomPaint` widgets painted by `materialTextSelectionControls`; `emptyTextSelectionControls` paints nothing, so the handle finders return 0 widgets. - `packages/flutter/test/widgets/editable_text_test.dart` (Live Text test only) — swapping `MaterialApp` → `TestWidgetsApp` triggers `A FocusManager was used after being disposed` in the shared `tearDown`, cascading into five downstream a11y/semantics tests. Both files remain on the `check_tests_cross_imports.dart` allowlist and can be picked up in follow-up PRs once we agree on the approach. ## Related issues - Part of umbrella issue flutter#177028 (Framework unit tests shouldn't cross-import libraries). - Also addresses flutter#177412 (No legacy Material imports in tests). ## Context — why one PR Per reviewer guidance from @navaronbracke on flutter#185238 ([comment](flutter#185238 (comment))), similar Material-import cleanups are being consolidated into a single PR that lands closer to the ~300–500-line "sweet spot" rather than as many tiny per-file PRs. ## Superseded PRs (please close once this lands) The following open PRs are subsumed by this one and can be closed if this PR is approved (see [this comment](flutter#185238 (comment))): - flutter#185236 — Remove unused material.dart import from force_press_test.dart - flutter#185241 — Remove unnecessary Material import from default_text_editing_shortcuts_test Not superseded (corresponding files were dropped from this PR — see the scope note above): - flutter#185238 — editable_text_scribe_test - flutter#185242 — editable_text_test Live Text ## 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]. <!-- test-only cleanup: no production code or test-behavior changes; existing coverage is preserved. --> - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
packages/flutter/test/widgets/editable_text_scribe_test.dartlives in the Widgets layer of the test tree but was importingpackage:flutter/material.dart. The import was only needed for aMaterialAppwrapper,Colors.grey, andmaterialTextSelectionControls— none of which are required for what this test actually exercises (theScribesystem channel +EditableText).Changes:
MaterialAppwithTestWidgetsApp(already available next door inwidgets_app_tester.dart).Colors.greywithconst Color(0xFF9E9E9E)(same value).materialTextSelectionControlswithemptyTextSelectionControls.package:flutter/material.dartimport.Related Issues
Part of #177028
Related: #177412
Tests
same assertions. Verified locally with
flutter test packages/flutter/test/widgets/editable_text_scribe_test.dart.Pre-launch Checklist