Fix scrollUntilVisible in WidgetTester #159582

hgraceb · 2024-11-28T08:53:38Z

The single moveBy event may be consumed when onTap is present.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2024-11-28T11:43:01Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

victorsanni · 2024-12-05T00:38:29Z

packages/flutter_test/lib/src/controller.dart

+    assert(moveStep != Offset.zero);
+    assert(maxIteration > 0);


(from offline review with @Piinks) these asserts (plus the delta assert) look valid. Are they related to this change specifically or just good to have generally? And can we add tests for them in either case?

I submitted the corresponding unit tests.

The assets are unrelated to this change. I just thought of an optimization that could be added when I saw the code, as I noticed that the Flutter documentation encourages us to use assertions.

flutter/packages/flutter_test/lib/src/controller.dart

Line 2182 in ab5b20c

assert(maxScrolls > 0);

Use asserts liberally to detect contract violations and verify invariants

assert() allows us to be diligent about correctness without paying a performance penalty in release mode, because Dart only evaluates asserts in debug mode.

However, I didn't realize that adding asserts also requires corresponding unit tests. Maybe in the future, I should try to submit code that is only related to the issue at hand.

The assertions seem valid to add, but maybe in a subsequent PR so it's easier to track and review.

Unit tests for them shouldn't be too difficult to add as well. Consider this PR that only added assertions for some test inspiration.

I have reverted the asserts and the corresponding tests, and I may submit a new PR later.

victorsanni

LGTM! Thanks for the PR.

Piinks

LGTM thank you!

victorsanni · 2024-12-06T21:35:36Z

packages/flutter_test/lib/src/controller.dart

      while (maxIteration > 0 && finder.evaluate().isEmpty) {
-        await drag(view, moveStep);
+        gesture ??= await startGesture(getCenter(view, warnIfMissed: true));
+        await gesture.moveBy(moveStep);


There are quite a lot of google testing failures. I wonder if there are things in the drag method that this refactor doesn't replace?

I'm guessing the tests this affects in Google were written to expect the behavior written in the bug. This might be difficult to change since it breaks existing test logic.

goderbauer · 2024-12-17T23:18:23Z

Maybe we need to introduce this as a new method or guard it behind a parameter to not break all the existing code?

goderbauer · 2025-01-14T23:11:30Z

(triage): @hgraceb Are you interested in updating this to make it opt-in (e.g. as a new method or via a parameter).

hgraceb · 2025-01-15T14:22:24Z

@goderbauer Sorry, I haven't had time to follow up on this issue recently because I've been job hunting. I might try to make some updates this week or next week.

hgraceb · 2025-01-18T12:49:09Z

@victorsanni @goderbauer Hi, I tried to keep all the original code logic and only reuse the gesture, but the Google tests are still failing. I might need some more specific error examples to make further changes. Can someone help provide a portion of the failing test code?

victorsanni · 2025-01-21T20:09:09Z

Hi, I tried to keep all the original code logic and only reuse the gesture, but the Google tests are still failing. I might need some more specific error examples to make further changes. Can someone help provide a portion of the failing test code?

Most of the test failures are image diffs reflecting the previous behavior, as @Piinks mentioned. Can we make the behavior opt in as @goderbauer described?

victorsanni · 2025-01-27T19:04:22Z

packages/flutter_test/test/controller_test.dart

+    await tester.pumpWidget(buildFrame(true));
+    await tester.pumpAndSettle();
+    expect(target, findsNothing);
+    await tester.scrollUntilVisible(target, 20, scrollable: scrollable, continuous: true);


Should this test also check for the correct behavior when continuous = false?

@victorsanni I believe that in this case, there is no such thing as the correct behavior when continuous = false. The fact that all tests that did not pass continuous are currently passing is the correct behavior.

Roll Flutter from c1ffaa9 to b007899 (43 revisions) flutter/flutter@c1ffaa9...b007899 2025-01-29 [email protected] Fix `Tab` linear and elastic animation blink (flutter/flutter#162315) 2025-01-29 [email protected] Pass-through `textInputAction` in `DropdownMenu` (flutter/flutter#162309) 2025-01-29 [email protected] Fix scrollUntilVisible in WidgetTester (flutter/flutter#159582) 2025-01-29 [email protected] Pass-through `maxLines` in `DropdownMenu` (flutter/flutter#161903) 2025-01-29 [email protected] fix: appbar leading width is not square for custom toolbar height (flutter/flutter#161880) 2025-01-29 [email protected] [DisplayList] Don't call Skia Ganesh methods when its not available. (flutter/flutter#162345) 2025-01-29 [email protected] Update README.md to include googler post verification steps (flutter/flutter#162272) 2025-01-29 [email protected] [engine, web] return switch expressions in many places (flutter/flutter#162336) 2025-01-29 [email protected] Update README.md to not have engine link for android (flutter/flutter#162330) 2025-01-29 [email protected] Reland "[ Widget Previews ] Add support for detecting previews and generating code (#161911)"" (flutter/flutter#162337) 2025-01-29 [email protected] Add instructions to download the Gradle wrapper to FGP readme, and add to gitignore (flutter/flutter#162332) 2025-01-29 [email protected] Fix tests to prepare for `--explicit-package-dependencies` and a bug. (flutter/flutter#162289) 2025-01-29 [email protected] Add a currently unused `runs_in_merge_queue` property to `Linux analyze`. (flutter/flutter#162335) 2025-01-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[ Widget Previews ] Add support for detecting previews and generating code (#161911)" (flutter/flutter#162327) 2025-01-28 [email protected] Support hot restart for DDC library bundle format (flutter/flutter#162123) 2025-01-28 [email protected] Started adjusting uvs to match pixel snapping. (flutter/flutter#162049) 2025-01-28 [email protected] Refactor code inside flutter.groovy (flutter/flutter#160250) 2025-01-28 [email protected] Table implements redepth (flutter/flutter#162282) 2025-01-28 [email protected] [ Widget Previews ] Add support for detecting previews and generating code (flutter/flutter#161911) 2025-01-28 [email protected] remove dependency on `Usage` from `Pub` class (flutter/flutter#162279) 2025-01-28 [email protected] Roll Packages from 258f6dc to 02c6fef (6 revisions) (flutter/flutter#162313) 2025-01-28 [email protected] Remove `scenario_app/android` and rename to `ios_scenario_app`. (flutter/flutter#160992) 2025-01-28 [email protected] Apparently it is illegal to use `stderr` in this project. (flutter/flutter#162294) 2025-01-28 [email protected] Fix update_engine_version_test in presence of FLUTTER_PREBUILT_ENGINE_VERSION env vars. (flutter/flutter#162270) 2025-01-28 [email protected] Add missing `properties: ...` and move to presubmit. (flutter/flutter#162170) 2025-01-27 [email protected] [Impeller] make swapchain related external fence/semaphore extensions optional. (flutter/flutter#162205) 2025-01-27 49699333+dependabot[bot]@users.noreply.github.com Bump the all-github-actions group with 2 updates (flutter/flutter#162277) 2025-01-27 [email protected] Don't depend on Dart from FML. (flutter/flutter#162271) 2025-01-27 [email protected] [DisplayList] Move nested canvas enums into their own TU. (flutter/flutter#162037) 2025-01-27 [email protected] Avoid iOS text selection crash by returning nil range (flutter/flutter#161996) 2025-01-27 [email protected] fix `felt` link to point to flutter repo instead of the engine repo (flutter/flutter#161423) 2025-01-27 [email protected] Enable the Android Engine OpenGLES/Vulkan suites. (flutter/flutter#162258) 2025-01-27 [email protected] [canvaskit] Fix debug build for CanvasKit (flutter/flutter#162198) 2025-01-27 [email protected] Roll Packages from 3d3ab7b to 258f6dc (19 revisions) (flutter/flutter#162254) 2025-01-25 [email protected] Pin `customer_testing` to the SHA specified in `tests.version` (flutter/flutter#162048) 2025-01-25 [email protected] Formalize `update_engine_version.{sh|ps1}`. (flutter/flutter#162118) 2025-01-25 [email protected] Rename 'SelectionChangedCause.scribble' to 'SelectionChangedCause.stylusHandwriting' (flutter/flutter#161518) 2025-01-25 [email protected] Don't install xcode when doing `local_engine` web builds on mac. (flutter/flutter#162164) 2025-01-25 [email protected] Force Impeller backend for `android_engine_test`, and test both OpenGLES and Vulkan (flutter/flutter#162089) 2025-01-24 [email protected] [Impeller] when a command pool has many unused buffers, reset with release resources flag. (flutter/flutter#162171) 2025-01-24 [email protected] [web] Remove HTML renderer from framework tests (flutter/flutter#162038) 2025-01-24 [email protected] [Impeller] Skip clip entity replay that cannot impact current clip. (flutter/flutter#162113) 2025-01-24 [email protected] Update Android integration test package for newer AGP (flutter/flutter#161856) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: ...

github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Nov 28, 2024

hgraceb marked this pull request as draft November 28, 2024 11:42

hgraceb force-pushed the tester branch from 472b298 to 0e886ec Compare November 28, 2024 11:49

hgraceb marked this pull request as ready for review November 28, 2024 12:13

goderbauer requested review from Piinks and victorsanni December 3, 2024 23:13

hgraceb force-pushed the tester branch from 0e886ec to f90b181 Compare December 4, 2024 06:52

victorsanni reviewed Dec 5, 2024

View reviewed changes

victorsanni approved these changes Dec 5, 2024

View reviewed changes

hgraceb force-pushed the tester branch from 520c8e0 to a626c7e Compare December 6, 2024 03:15

Piinks approved these changes Dec 6, 2024

View reviewed changes

victorsanni reviewed Dec 6, 2024

View reviewed changes

hgraceb force-pushed the tester branch from a626c7e to c11821a Compare January 18, 2025 09:19

hgraceb force-pushed the tester branch 3 times, most recently from 077be8c to 571f26d Compare January 27, 2025 15:58

victorsanni approved these changes Jan 27, 2025

View reviewed changes

hgraceb force-pushed the tester branch from cd32ede to a1c136c Compare January 28, 2025 06:20

hgraceb added 5 commits January 28, 2025 14:25

Fix scrollUntilVisible in WidgetTester

e189e2c

Fix scrollUntilVisible in WidgetTester

0a6e936

Improve documentation comment

93b6057

Format code

4c96b5c

Format code

3f5eefa

hgraceb force-pushed the tester branch from a1c136c to 3f5eefa Compare January 28, 2025 06:25

hgraceb added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 29, 2025

auto-submit bot added this pull request to the merge queue Jan 29, 2025

Merged via the queue into flutter:master with commit b8de503 Jan 29, 2025
177 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 29, 2025

hgraceb mentioned this pull request Jan 29, 2025

[flutter_test]scrollUntilVisible has different results if ListTile's onTap is null or not #143921

Closed

engine-flutter-autoroll mentioned this pull request Jan 29, 2025

Roll Flutter from c1ffaa9d9deb to b007899d3a7a (43 revisions) flutter/packages#8527

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 29, 2025

b8de503 Fix scrollUntilVisible in WidgetTester (flutter/flutter#159582)

20f6050

Piinks mentioned this pull request Jan 31, 2025

Flutter Notable Commits - 2025 #161460

Open

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

b8de503 Fix scrollUntilVisible in WidgetTester (flutter/flutter#159582)

3b84ca6

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

b8de503 Fix scrollUntilVisible in WidgetTester (flutter/flutter#159582)

b67a486

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025

b8de503 Fix scrollUntilVisible in WidgetTester (flutter/flutter#159582)

3f9a679

Fix scrollUntilVisible in WidgetTester #159582

Fix scrollUntilVisible in WidgetTester #159582

Uh oh!

Conversation

hgraceb commented Nov 28, 2024

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Nov 28, 2024

Uh oh!

victorsanni Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

hgraceb Dec 5, 2024

Choose a reason for hiding this comment

Use asserts liberally to detect contract violations and verify invariants

Uh oh!

victorsanni Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

hgraceb Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victorsanni left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

victorsanni Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

Piinks Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Dec 17, 2024

Uh oh!

goderbauer commented Jan 14, 2025

Uh oh!

hgraceb commented Jan 15, 2025

Uh oh!

hgraceb commented Jan 18, 2025

Uh oh!

victorsanni commented Jan 21, 2025

Uh oh!

victorsanni Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

hgraceb Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hgraceb Dec 5, 2024 •

edited

Loading

hgraceb Jan 28, 2025 •

edited

Loading