Skip to content

test: Refactor scrollbar tests to no longer depend on material - #185396

Merged
auto-submit[bot] merged 15 commits into
flutter:masterfrom
shivanshu877:fix/177412-legacy-imports
Sep 24, 2026
Merged

test: Refactor scrollbar tests to no longer depend on material#185396
auto-submit[bot] merged 15 commits into
flutter:masterfrom
shivanshu877:fix/177412-legacy-imports

Conversation

@shivanshu877

@shivanshu877 shivanshu877 commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Description

Moves two testWidgets that use MaterialApp, Scaffold, DatePickerDialog, and ElevatedButton out of test/widgets/scrollbar_test.dart into test/material/scrollbar_test.dart where they belong.

Also replaces the package:flutter/material.dart import in the widgets test file with package:flutter/widgets.dart, since those moved tests were the only Material usages.

Tests

All existing tests continue to pass:

  • packages/flutter/test/widgets/scrollbar_test.dart — 99 tests pass
  • packages/flutter/test/material/scrollbar_test.dart — 51 tests pass (includes the 2 moved tests)

Related Issues

Part of #177412

@shivanshu877
Move Material-dependent scrollbar tests to material/ directory
2e40442 
Move two testWidgets that use MaterialApp/Scaffold/DatePickerDialog/
ElevatedButton from test/widgets/scrollbar_test.dart into
test/material/scrollbar_test.dart where they belong.

Replace flutter/material.dart import with flutter/widgets.dart in the
widgets test file since the moved tests were the only Material usages.

Part of flutter#177412
@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages f: scrolling Viewports, list views, slivers, etc. labels Apr 22, 2026
@google-cla

google-cla Bot commented Apr 22, 2026

Copy link
Copy Markdown

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.

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Apr 22, 2026
View reviewed changes

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

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.

Code Review

This pull request moves two regression tests regarding scrollbar precision and dynamic controller assignment from the widgets test suite to the material test suite, while also updating an import in the widgets test file. A minor grammatical correction was suggested for one of the test descriptions to improve readability and clarity.

Comment thread packages/flutter/test/material/scrollbar_test.dart Outdated
@shivanshu877

shivanshu877 commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the review, @gemini-code-assist!

Grammar fix: Fixed — the test description now reads 'Scrollbar should not be shown due to precision error on desktop'.

@gemini-code-assist

gemini-code-assist Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Thanks for the update, @shivanshu877. The grammar fix looks good. I have no further comments on this pull request.

justinmc
justinmc reviewed Apr 29, 2026
View reviewed changes

@justinmc justinmc left a comment

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.

Thanks for the PR, left a comment below about one test that maybe should be refactored instead of moved.

Comment thread packages/flutter/test/material/scrollbar_test.dart Outdated
Comment thread packages/flutter/test/material/scrollbar_test.dart Outdated
@justinmc justinmc moved this from Todo to In Progress in Test cross-imports Review Queue Apr 29, 2026
shivanshu877 and others added 2 commits April 30, 2026 10:16
@shivanshu877
test: keep RawScrollbar regression test in widgets/, refactor off Mat…
bb0c656 
…erial

Per @justinmc's review on flutter#185396, the RawScrollbar regression test
exercises pure widget-level behavior (assigning a controller with
thumbVisibility true in the same frame) and only used Material because
the original repro used an ElevatedButton to trigger setState.

This commit:
* Moves the test back to packages/flutter/test/widgets/scrollbar_test.dart
* Replaces ElevatedButton with a GestureDetector wrapping a SizedBox so
  the test no longer needs Material to compile or run.

The DatePickerDialog precision-error test stays in test/material/ since
it genuinely exercises Material behavior.
@victorsanni
Merge branch 'master' into fix/177412-legacy-imports
4a39dcd
@victorsanni victorsanni added CICD Run CI/CD override code freeze Override an active code freeze. labels Apr 30, 2026
victorsanni
victorsanni previously approved these changes Apr 30, 2026
View reviewed changes
@victorsanni
victorsanni requested a review from justinmc April 30, 2026 23:41
justinmc
justinmc previously approved these changes May 4, 2026
View reviewed changes

@justinmc justinmc left a comment

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.

LGTM 👍. Thanks for helping out with this effort!

@justinmc

justinmc commented May 4, 2026

Copy link
Copy Markdown
Contributor

Heads up there is an analyzer failure.

@shivanshu877
fix: sort imports alphabetically in widgets/scrollbar_test.dart
3c4ed1d 
The analyzer flagged a directives_ordering lint on line 9 of
test/widgets/scrollbar_test.dart — the package:flutter imports were
not alphabetically sorted (foundation, widgets, src/physics/utils
instead of foundation, src/physics/utils, widgets).

Reordered the three imports so they're alphabetical.
@shivanshu877
Merge branch 'fix/177412-legacy-imports' of https://github.com/shivan…
396cbfb 
…shu877/flutter into fix/177412-legacy-imports
@shivanshu877
shivanshu877 dismissed stale reviews from justinmc and victorsanni via 396cbfb May 4, 2026 18:18
@shivanshu877

shivanshu877 commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the LGTM @justinmc! Fixed the analyzer failure in commit 3c4ed1d — the lint was a directives_ordering issue (the three package:flutter imports in widgets/scrollbar_test.dart weren't alphabetical). Reordered to foundation → src/physics/utils → widgets. flutter analyze is now clean and all 150 scrollbar tests pass.

@github-actions github-actions Bot removed the CICD Run CI/CD label May 4, 2026
@shivanshu877

shivanshu877 commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

@victorsanni @justinmc — your approvals were dismissed by the analyzer-fix push (commit 3c4ed1d). The only change since the previous approvals is the one-line directives_ordering reorder in widgets/scrollbar_test.dart (foundation → src/physics/utils → widgets). flutter analyze is now clean and all 150 tests still pass. Could you re-approve when you have a moment? Thanks!

@dkwingsmt
dkwingsmt requested review from justinmc and victorsanni May 6, 2026 18:23
@dkwingsmt
dkwingsmt self-requested a review August 12, 2026 18:17
@dkwingsmt

dkwingsmt commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Thank you for the explanation (and sorry for the late response)!

Correct me if I'm wrong because the diff of material/scrollbar_test.dart is bit hard to understand due to the reformatting. As far as I understand, this PR consists of the follows:

  1. Refactor a test 'RawScrollbar does not assert when dynamically assigning controller with thumbVisibility true' in widgets/scrollbar_test.dart so that it uses nothing from material.
  2. Move 'Scrollbar should not shown due to precision error on desktop' from widgets/scrollbar_test.dart to material/scrollbar_test.
  3. Some reformatting in material/scrollbar_test which I assume includes no semantic changes.

I don't see a total of 5 move tests here. Is it possible that the changes have been implemented by other PRs?

Anyway, the remaining changes here are still valuable, and I appreciate you for making them. The current idea is that, we'll no longer land changes to material, and instead such changes should be made to material_ui in flutter/packages. See #188444 .

My suggestion is as follows:

  • Reshape this PR so that it contains change 1 and the removal of the test in change 2.
  • Submit a new PR in flutter/packages that consists of the addition of the test in change 2.
  • Refrain from making format changes unless necessary.

If there are other cross-import issues or misplaced tests, you're welcome (and appreicated) to submit them together or in separate PRs.

Thank you again!

@shivanshu877
Merge remote-tracking branch 'upstream/master' into fix/177412-legacy…
786268f 
…-imports

# Conflicts:
#	dev/bots/check_tests_cross_imports.dart
@shivanshu877
test: limit this PR to the widgets-side scrollbar cleanup
188e5bd 
Reverts packages/flutter/test/material/scrollbar_test.dart, which drops
both the addition of the precision-error test and the reformatting churn
in that file. What remains is the refactor of the RawScrollbar dynamic
controller test off material, the removal of the precision-error test
from widgets/, and the matching knownWidgetsCrossImports entry.

The addition of the precision-error test goes to material_ui in
flutter/packages instead, per the material freeze in
flutter#188444.
@github-actions github-actions Bot removed the p: material_ui material_ui package in flutter/packages label Aug 31, 2026
@shivanshu877

shivanshu877 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

You're right — only one test actually moves. The other four (Hover animation is not triggered by tap gestures, Scrollbar passes controller to CupertinoScrollbar, Scrollbar doesn't show when scroll the inner scrollable widget, Scrollbar dragging can be disabled) were already in material/scrollbar_test.dart at the merge base — they landed via other PRs, and only show up in the diff because of the reformatting. My earlier count described the original April version of this PR.

Reshaped as you suggested: reverted material/scrollbar_test.dart, which drops both the addition and the format churn. What's left is the refactor of the RawScrollbar test off material, the removal of the precision-error test from widgets/, and its knownWidgetsCrossImports entry — 3 files down to 2, 7 insertions / 48 deletions. All 100 tests in widgets/scrollbar_test.dart pass.

I'll send the addition of the precision-error test to material_ui in flutter/packages as a separate PR.

@dkwingsmt dkwingsmt changed the title test: move Material-dependent scrollbar tests to material/ directory test: Refactor scrollbar tests to no longer depend on material Sep 9, 2026
@dkwingsmt dkwingsmt added the CICD Run CI/CD label Sep 9, 2026
dkwingsmt
dkwingsmt approved these changes Sep 24, 2026
View reviewed changes

@dkwingsmt dkwingsmt left a comment

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.

LGTM. Thank you.

@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Sep 24, 2026
@dkwingsmt dkwingsmt added the CICD Run CI/CD label Sep 24, 2026
@victorsanni victorsanni added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 24, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Sep 24, 2026
Merged via the queue into flutter:master with commit a266c59 Sep 24, 2026
40 of 41 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 24, 2026
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Test cross-imports Review Queue Sep 24, 2026
This was referenced Sep 25, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dkwingsmt dkwingsmt dkwingsmt approved these changes
@victorsanni victorsanni victorsanni approved these changes
@justinmc justinmc Awaiting requested review from justinmc
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CICD Run CI/CD Decoupling: Not ready to port yet Instructions will be provided when this is ready to move to flutter/packages. Decoupling: Split PR The PR will need to be split to separate Material & Cupertino changes f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

Test cross-imports Review Queue
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shivanshu877 @justinmc @Piinks @dkwingsmt @victorsanni