refactor: remove material import froma sliver_main_axis_group_test, sliver_semantics_test, semantics_role_checks_test, sliver_fill_remaining_test, tap_region_test by rkishan516 · Pull Request #185567 · flutter/flutter

rkishan516 · 2026-04-25T02:58:39Z

This PR removes Material imports from

sliver_main_axis_group_test
sliver_semantics_test
semantics_role_checks_test
sliver_fill_remaining_test
tap_region_test

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

gemini-code-assist

Code Review

This pull request refactors several widget tests to eliminate dependencies on the Material library, replacing components like MaterialApp, Scaffold, and SliverAppBar with TestWidgetsApp and custom SliverPersistentHeader implementations. It also replaces Material-specific widgets such as ElevatedButton and Radio with basic widgets or test-specific wrappers like TestButton and RawRadio. Feedback identifies several opportunities to further optimize the code by using ColoredBox instead of Container when only a background color is required.

navaronbracke · 2026-04-25T10:28:38Z

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

-import 'package:flutter/material.dart';


@rkishan516 Nice to see that you're trying to tackle this test.

I do have some feedback on this specific one, though.

IIRC the last time I looked at it, there were some issues:

There is also sliversemantics_test.dart
I don't recall if that test had duplicated stuff between itself and sliver_semantics_test.dart

Both of these test files do get large (especially if you consider them both)
We should split these tests off into sliver_semantics_1_test.dart etc. Smaller test files are easier to post to the test runner, too

I think there was some usage of group(), together with a _testFoo() helper method, both seemed redundant (why not put the body of that method in void main() ?)

Hey, sure. I think sliversemantics_test.dart and sliver_semantics_test.dart have different scope. Will try to migrate that also.

I think there was some usage of group(), together with a _testFoo() helper method, both seemed redundant (why not put the body of that method in void main() ?)

Yes, this pattern still exist, we can drop group() + _tests() indirection.

nvm, that was already migrated.

I agree about the 3 points above. Possibly could be done in another PR though.

@Renzo-Olivares Do you know the history of sliver_semantics_test.dart and sliversemantics_test.dart? #167300

sliversemantics_test.dart tests the recent SliverSemantics widget which is a Sliver variant of the Semantics widget. It is essentially a duplicate of semantics_test.dart but for SliverSemantics.

Not as familiar with sliver_semantics_test.dart, but generally it just holds different integration tests for semantics and sliver components.

sliversemantics_test.dart and sliver_semantics_test.dart are definitely different in scope and should not be considered the same.

I'm not against splitting these files up, but it should be clear what the file is testing.

I can do a follow up MR with proper naming and split.

justinmc

LGTM 👍 , thanks!

justinmc · 2026-04-27T20:21:31Z

-    'SliverAppBar with floating: true, pinned: true, snap: true is painted within bounds of SliverMainAxisGroup',
+    'SliverPersistentHeader with floating: true, pinned: true, snap: true is painted within bounds of SliverMainAxisGroup',


It looks like there's no reason to duplicate these SliverAppBar tests in Material, right? I guess it wouldn't test anything that these are not already testing at the widgets level.

Yup, we don't need in Material.

justinmc · 2026-04-27T20:33:12Z

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

-import 'package:flutter/material.dart';


I agree about the 3 points above. Possibly could be done in another PR though.

@Renzo-Olivares Do you know the history of sliver_semantics_test.dart and sliversemantics_test.dart? #167300

victorsanni

LGTM with some nits.

victorsanni · 2026-04-28T01:03:22Z

+    // Push the second page so tapRegion2 is on top of tapRegion1.
+    Navigator.of(tester.element(find.byKey(tapRegion1Key))).pushNamed('/second');


Why do we need to do this?

We want second page to be on top when doing tap outside assertion.

SGTM. Previously this was done in onGenerateInitialRoutes.

victorsanni · 2026-04-28T01:15:02Z

-                },
-                child: Container(width: 250.0, height: 250.0, color: Colors.blue),
-              ),
+                  ).createRoute(context),


Why is this here?

Since we are using TestPage, to createPageRouteBuilder we are calling this.

…liver_semantics_test, semantics_role_checks_test, sliver_fill_remaining_test, tap_region_test

… based on feedback

justinmc

LGTM 👍

justinmc · 2026-05-01T17:17:02Z

+    // Push the second page so tapRegion2 is on top of tapRegion1.
+    Navigator.of(tester.element(find.byKey(tapRegion1Key))).pushNamed('/second');


SGTM. Previously this was done in onGenerateInitialRoutes.

@justinmc

Master (flutter#185567) already merged the move/refactor of TapRegion navigation tests off Material. This merge adopts that version of test/widgets/tap_region_test.dart and re-applies the small robustness improvement on top: * Replace the two duplicated `Future<void> tapOutside(...)` local closures with a single top-level `_tapOutside(tester, regionKey)` helper that locates the visible region's RenderBox dynamically (renderBox.localToGlobal(Offset.zero) + Offset(200, 200)) instead of using a hardcoded global coordinate. This addresses @justinmc's review feedback that the original material/-based tapOutside was more robust than the closure with hardcoded coords.

This PR is follow up to flutter#185567 (comment) conversation where we want to properly name and split test files. part of: flutter#177415 # Current Status - Renamed `sliversemantics_test.dart` to `sliver_semantics_widget_test.dart` ## 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]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: Renzo Olivares <rmolivares@renzo-olivares.dev>

github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) f: scrolling Viewports, list views, slivers, etc. labels Apr 25, 2026

rkishan516 requested review from justinmc and victorsanni April 25, 2026 02:59

rkishan516 added the CICD Run CI/CD label Apr 25, 2026

gemini-code-assist Bot reviewed Apr 25, 2026

View reviewed changes

Comment thread packages/flutter/test/widgets/sliver_fill_remaining_test.dart Outdated

Comment thread packages/flutter/test/widgets/sliver_fill_remaining_test.dart Outdated

Comment thread packages/flutter/test/widgets/sliver_fill_remaining_test.dart Outdated

navaronbracke reviewed Apr 25, 2026

View reviewed changes

rkishan516 force-pushed the simple-cross-imports-9 branch from a14852a to 743ebf2 Compare April 27, 2026 13:28

github-actions Bot removed the CICD Run CI/CD label Apr 27, 2026

rkishan516 added the CICD Run CI/CD label Apr 27, 2026

justinmc mentioned this pull request Apr 27, 2026

(Test cross-imports) Don't import Material in tests for convenience #177415

Open

justinmc previously approved these changes Apr 27, 2026

View reviewed changes

rkishan516 dismissed justinmc’s stale review via 9006266 April 28, 2026 01:02

github-actions Bot removed the CICD Run CI/CD label Apr 28, 2026

rkishan516 force-pushed the simple-cross-imports-9 branch from 9006266 to 89cc75a Compare April 28, 2026 01:05

rkishan516 added the CICD Run CI/CD label Apr 28, 2026

victorsanni previously approved these changes Apr 28, 2026

View reviewed changes

rkishan516 dismissed victorsanni’s stale review via 3a0b1e2 April 28, 2026 02:17

rkishan516 force-pushed the simple-cross-imports-9 branch from 89cc75a to 3a0b1e2 Compare April 28, 2026 02:17

github-actions Bot removed the CICD Run CI/CD label Apr 28, 2026

rkishan516 added the CICD Run CI/CD label Apr 28, 2026

rkishan516 force-pushed the simple-cross-imports-9 branch from 3a0b1e2 to 1238b57 Compare April 29, 2026 01:26

github-actions Bot removed the CICD Run CI/CD label Apr 29, 2026

rkishan516 added the CICD Run CI/CD label Apr 29, 2026

rkishan516 force-pushed the simple-cross-imports-9 branch from 1238b57 to 56ae0e5 Compare April 30, 2026 02:15

github-actions Bot removed the CICD Run CI/CD label Apr 30, 2026

rkishan516 added the CICD Run CI/CD label May 1, 2026

rkishan516 added 2 commits May 1, 2026 19:19

refactor: remove material import froma sliver_main_axis_group_test, s…

b1e1dd5

…liver_semantics_test, semantics_role_checks_test, sliver_fill_remaining_test, tap_region_test

refactor: remove unrequired group + tests from sliver semantics tests…

639a708

… based on feedback

github-actions Bot removed the CICD Run CI/CD label May 1, 2026

rkishan516 added the CICD Run CI/CD label May 1, 2026

justinmc added this to Test cross-imports Review Queue May 1, 2026

github-project-automation Bot moved this to Todo in Test cross-imports Review Queue May 1, 2026

justinmc approved these changes May 1, 2026

View reviewed changes

rkishan516 added the autosubmit Merge PR when tree becomes green via auto submit App label May 1, 2026

auto-submit Bot added this pull request to the merge queue May 1, 2026

Merged via the queue into flutter:master with commit c8cbdf7 May 1, 2026
170 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 1, 2026

github-project-automation Bot moved this from Todo to Done in Test cross-imports Review Queue May 1, 2026

rkishan516 mentioned this pull request May 2, 2026

refactor: update filename for sliver semantic widget #185917

Merged

10 tasks

This was referenced May 4, 2026

test: remove Material dependency from sliver_main_axis_group_test #185402

Closed

test: dynamic _tapOutside helper for TapRegion navigation tests #185397

Open

rkishan516 deleted the simple-cross-imports-9 branch May 16, 2026 06:15

Renzo-Olivares mentioned this pull request Jun 24, 2026

(Test cross-imports) Tests should go in the library that they're testing #177414

Open

57 tasks

		'SliverAppBar with floating: true, pinned: true, snap: true is painted within bounds of SliverMainAxisGroup',
		'SliverPersistentHeader with floating: true, pinned: true, snap: true is painted within bounds of SliverMainAxisGroup',

		// Push the second page so tapRegion2 is on top of tapRegion1.
		Navigator.of(tester.element(find.byKey(tapRegion1Key))).pushNamed('/second');

Uh oh!

Conversation

rkishan516 commented Apr 25, 2026

Pre-launch Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victorsanni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants