Skip to content

Remove unnecessary Material import from restoration_scopes_moving_test#184591

Merged
auto-submit[bot] merged 5 commits into
flutter:masterfrom
xfce0:fix-remove-material-from-scribe-test
Apr 28, 2026
Merged

Remove unnecessary Material import from restoration_scopes_moving_test#184591
auto-submit[bot] merged 5 commits into
flutter:masterfrom
xfce0:fix-remove-material-from-scribe-test

Conversation

@xfce0
Copy link
Copy Markdown
Contributor

@xfce0 xfce0 commented Apr 3, 2026

Replace OutlinedButton with GestureDetector to remove the package:flutter/material.dart import from restoration_scopes_moving_test.dart. The test only needs a tappable widget with text, so GestureDetector is sufficient.

Part of #177415.

Pre-launch Checklist

@github-actions github-actions Bot added the framework flutter/packages/flutter repository. See also f: labels. label Apr 3, 2026
Copy link
Copy Markdown
Contributor

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

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 updates the import statement in the test file and replaces an OutlinedButton with a GestureDetector in the CounterState widget. The review suggests adding HitTestBehavior.opaque to the GestureDetector to ensure reliable hit testing during tap interactions in tests.

Comment on lines +210 to +211
return GestureDetector(
onTap: () {
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.

medium

When replacing a button with a GestureDetector in tests, it is recommended to set behavior: HitTestBehavior.opaque. This ensures that the entire area of the widget is hit-testable, even if the child (like a Text widget) has transparent areas between its glyphs, which helps prevent flaky test failures during tap interactions.

    return GestureDetector(
      behavior: HitTestBehavior.opaque,
      onTap: () {

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.

If the tests pass I'm not concerned with this.

Copy link
Copy Markdown
Contributor

@victorsanni victorsanni left a comment

Choose a reason for hiding this comment

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

Should this be removed from dev/bots/check_tests_cross_imports.dart?

@xfce0
Copy link
Copy Markdown
Contributor Author

xfce0 commented Apr 3, 2026

Good catch, thanks! Removed it from knownWidgetsCrossImports in check_tests_cross_imports.dart since the Material import is no longer there.

victorsanni
victorsanni previously approved these changes Apr 4, 2026
@victorsanni victorsanni requested a review from justinmc April 4, 2026 00:51
@victorsanni victorsanni added the CICD Run CI/CD label Apr 4, 2026
rkishan516
rkishan516 previously approved these changes Apr 8, 2026
Copy link
Copy Markdown
Contributor

@rkishan516 rkishan516 left a comment

Choose a reason for hiding this comment

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

LGTM

Replace OutlinedButton with GestureDetector to remove the Material
dependency. The test only needs a tappable widget with text.

Part of flutter#177415.
@xfce0 xfce0 dismissed stale reviews from rkishan516 and victorsanni via c6a9de2 April 8, 2026 15:22
@xfce0 xfce0 force-pushed the fix-remove-material-from-scribe-test branch from 06c121e to c6a9de2 Compare April 8, 2026 15:22
@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 8, 2026
@xfce0
Copy link
Copy Markdown
Contributor Author

xfce0 commented Apr 8, 2026

Resolved the merge conflict in check_tests_cross_imports.dart and rebased onto the latest master.

@victorsanni victorsanni added the CICD Run CI/CD label Apr 8, 2026
victorsanni
victorsanni previously approved these changes Apr 8, 2026
Comment thread dev/bots/check_tests_cross_imports.dart Outdated
Comment on lines +113 to +114
'packages/flutter/test/widgets/list_view_viewporting_test.dart',
'packages/flutter/test/widgets/semantics_clipping_test.dart',
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.

Why add these two?

The Material import was removed, so the file no longer needs to be
in the knownWidgetsCrossImports allowlist.
@xfce0 xfce0 force-pushed the fix-remove-material-from-scribe-test branch from c6a9de2 to a7c9e69 Compare April 8, 2026 20:04
@xfce0
Copy link
Copy Markdown
Contributor Author

xfce0 commented Apr 8, 2026

Sorry about that — list_view_viewporting_test.dart and semantics_clipping_test.dart were accidentally included when resolving a rebase conflict (they appeared on our side of the conflict at the time but don't actually have cross-imports). Removed them in the latest push — the diff now only removes restoration_scopes_moving_test.dart.

@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 8, 2026
@rkishan516 rkishan516 added the CICD Run CI/CD label Apr 8, 2026
rkishan516
rkishan516 previously approved these changes Apr 8, 2026
justinmc
justinmc previously approved these changes Apr 14, 2026
Copy link
Copy Markdown
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines +210 to +211
return GestureDetector(
onTap: () {
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.

If the tests pass I'm not concerned with this.

@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 21, 2026
@rkishan516 rkishan516 dismissed stale reviews from justinmc and themself via 9fd8487 April 27, 2026 13:31
Copy link
Copy Markdown
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

Re LGTM 👍

@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 27, 2026
@rkishan516 rkishan516 added the CICD Run CI/CD label Apr 28, 2026
@justinmc justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2026
@auto-submit auto-submit Bot added this pull request to the merge queue Apr 28, 2026
Merged via the queue into flutter:master with commit 3687ca6 Apr 28, 2026
167 of 168 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2026
@xfce0 xfce0 deleted the fix-remove-material-from-scribe-test branch April 29, 2026 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants