Skip to content

Move sliver_utils to test/widgets to fix rendering→widgets layering#185472

Merged
auto-submit[bot] merged 3 commits intoflutter:masterfrom
MohamedGawdat:remove-material-from-sliver-utils
Apr 29, 2026
Merged

Move sliver_utils to test/widgets to fix rendering→widgets layering#185472
auto-submit[bot] merged 3 commits intoflutter:masterfrom
MohamedGawdat:remove-material-from-sliver-utils

Conversation

@MohamedGawdat
Copy link
Copy Markdown
Contributor

@MohamedGawdat MohamedGawdat commented Apr 23, 2026

Part of #177412

Summary

The first commit on this branch swapped a legacy material.dart import in packages/flutter/test/rendering/sliver_utils.dart for widgets.dart. Reviewer feedback (gemini-code-assist) flagged a layering concern: a file under test/rendering/ should not depend on package:flutter/widgets.dart.

Since MockSliverToBoxAdapter extends SingleChildRenderObjectWidget, the widgets dependency is structural and cannot be removed. Both consumers of this utility (sliver_main_axis_group_test.dart, sliver_cross_axis_group_test.dart) already live in test/widgets/, so the second commit moves the file there and updates the import paths.

Changes:

  • Move packages/flutter/test/rendering/sliver_utils.dartpackages/flutter/test/widgets/sliver_utils.dart
  • Update import in packages/flutter/test/widgets/sliver_main_axis_group_test.dart
  • Update import in packages/flutter/test/widgets/sliver_cross_axis_group_test.dart

No behavior change.

Pre-launch Checklist

Tests

  • ./bin/dart analyze packages/flutter/test/widgets/sliver_utils.dart packages/flutter/test/widgets/sliver_main_axis_group_test.dart packages/flutter/test/widgets/sliver_cross_axis_group_test.dart
  • ./bin/flutter test packages/flutter/test/widgets/sliver_main_axis_group_test.dart packages/flutter/test/widgets/sliver_cross_axis_group_test.dart

@flutter-dashboard
Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Apr 23, 2026
@google-cla
Copy link
Copy Markdown

google-cla Bot commented Apr 23, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

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 sliver_utils.dart by replacing the material library import with the widgets library. A review comment identifies an architectural layering violation where the rendering layer now depends on the widgets layer and suggests moving the utility file to the widgets test directory to maintain proper repository boundaries.

Comment thread packages/flutter/test/widgets/sliver_utils.dart
@MohamedGawdat MohamedGawdat changed the title (Test cross-imports) Remove legacy Material import from sliver_utils Move sliver_utils to test/widgets to fix rendering→widgets layering Apr 27, 2026
@MohamedGawdat
Copy link
Copy Markdown
Contributor Author

Test exemption note: this is a no-semantic-change refactor — the file is moved between test directories and two import paths are updated, with no behavior change. The existing tests in sliver_main_axis_group_test.dart and sliver_cross_axis_group_test.dart continue to pass and exercise the same code paths. Per Tree Hygiene, refactors of this kind are not auto-exempt; happy to request an explicit test-exempt: <reason> from @test-exemption-reviewer on Discord if a reviewer prefers.

Copy link
Copy Markdown
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM!

@AbdeMohlbi AbdeMohlbi added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2026
@auto-submit
Copy link
Copy Markdown
Contributor

auto-submit Bot commented Apr 28, 2026

auto label is removed for flutter/flutter/185472, Failed to enqueue flutter/flutter/185472 with HTTP 400: Pull request Required status check "Merge Queue Guard" is expected..

@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2026
@AbdeMohlbi AbdeMohlbi added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App labels Apr 28, 2026
The previous commit replaced a legacy `material.dart` import in
`test/rendering/sliver_utils.dart` with `widgets.dart`. Reviewer feedback
flagged that a file under `test/rendering/` should not depend on the
widgets layer.

Since `MockSliverToBoxAdapter` extends `SingleChildRenderObjectWidget`
the widgets dependency is structural. Both consumers
(`sliver_main_axis_group_test.dart`, `sliver_cross_axis_group_test.dart`)
already live under `test/widgets/`, so this moves the utility to sit
next to them and updates the import paths.

No behavior change.
@Piinks Piinks force-pushed the remove-material-from-sliver-utils branch from bff8a78 to 22896c7 Compare April 29, 2026 01:00
@github-actions github-actions Bot removed the CICD Run CI/CD label Apr 29, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 29, 2026
@auto-submit
Copy link
Copy Markdown
Contributor

auto-submit Bot commented Apr 29, 2026

auto label is removed for flutter/flutter/185472, Failed to enqueue flutter/flutter/185472 with HTTP 400: Pull request Required status check "Merge Queue Guard" is expected..

@AbdeMohlbi AbdeMohlbi added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App labels Apr 29, 2026
@auto-submit auto-submit Bot added this pull request to the merge queue Apr 29, 2026
Merged via the queue into flutter:master with commit 1221b4b Apr 29, 2026
90 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants