Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed -> DropdownMenu throws exception when it is in any scrollable l… #140566

Merged
merged 9 commits into from Mar 7, 2024

Conversation

faisalansari0367
Copy link
Contributor

Fixed -> DropdownMenu throws exception when it is in any scrollable list view and scrolls quickly #139871

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Dec 23, 2023
@faisalansari0367
Copy link
Contributor Author

I'd appreciate some assistance in writing the necessary tests to cover the changes introduced in this PR.

Could someone with expertise in testing or familiarity with this part of the codebase kindly review and contribute by creating the required tests? Your input will significantly enhance the reliability and robustness of the code.

Thank you in advance for your collaboration!

@navaronbracke
Copy link
Contributor

navaronbracke commented Dec 23, 2023

You could write a widget test that does the following:

  • pump a ListView.builder with the WidgetTester; this widget contains a lot of Containers and a dropdown somewhere in the middle
  • start a fling gesture with a high enough velocity that scrolls the list fast enough to both create the Dropdown widget state and to make the widget unmount before the post frame callback ran
  • dispose of the gesture that you created, at the end of the test
  • add an expect(tester.takeException(), isNull); to verify that no exception happened

@faisalansari0367
Copy link
Contributor Author

You could write a widget test that does the following:

  • pump a ListView.builder with the WidgetTester; this widget contains a lot of Containers and a dropdown somewhere in the middle
  • start a fling gesture with a high enough velocity that scrolls the list fast enough to both create the Dropdown widget state and to make the widget unmount before the post frame callback ran
  • dispose of the gesture that you created, at the end of the test
  • add an expect(tester.takeException(), isNull); to verify that no exception happened

Thanks @navaronbracke

I have added the tests for the same.

Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution! This looks good:) Could you help rebase master to fix the conflicts?

@faisalansari0367
Copy link
Contributor Author

@QuncCccccc I have merged it with master, Please approve this PR.
Thanks

Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

LGTM:) Thanks for the contribution!

Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for putting this together.

@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 7, 2024
@auto-submit auto-submit bot merged commit 874804e into flutter:master Mar 7, 2024
67 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 8, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Mar 8, 2024
flutter/flutter@471a828...7c89ec8

2024-03-08 kustermann@google.com Remove `toString()` overrides in `dart:ui`/`package:flutter` in profile/release mode on wasm/vm targets (flutter/flutter#144763)
2024-03-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 80cd7981043f to bbb1ed00af49 (3 revisions) (flutter/flutter#144813)
2024-03-08 65758246+Nenuphar12@users.noreply.github.com Update documentation of `AlertDialog`'s default `TextStyle` for Material 3 (flutter/flutter#144697)
2024-03-08 engine-flutter-autoroll@skia.org Roll Flutter Engine from 175069397a40 to 80cd7981043f (5 revisions) (flutter/flutter#144804)
2024-03-07 andrewrkolos@gmail.com Enable asset transformation for `flutter run -d <browser>` and `flutter test` (flutter/flutter#144734)
2024-03-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from ec4f9af3d53b to 175069397a40 (2 revisions) (flutter/flutter#144799)
2024-03-07 git@reb0.org refactor: Remove `throwOnPluginPubspecError` flag for plugin validation (flutter/flutter#144214)
2024-03-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 305333c50191 to ec4f9af3d53b (1 revision) (flutter/flutter#144797)
2024-03-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from c2863530ce39 to 305333c50191 (1 revision) (flutter/flutter#144793)
2024-03-07 faisal.hiyaz@gmail.com Fixed -> DropdownMenu throws exception when it is in any scrollable l� (flutter/flutter#140566)
2024-03-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9e8ccaa3842e to c2863530ce39 (2 revisions) (flutter/flutter#144792)
2024-03-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 12828950890a to 9e8ccaa3842e (1 revision) (flutter/flutter#144784)
2024-03-07 fluttergithubbot@gmail.com Marks Mac_x64 module_test_ios to be flaky (flutter/flutter#144681)
2024-03-07 christopherfujino@gmail.com [flutter_tools] add custom tool analysis to analyze.dart, lint Future.catchError (flutter/flutter#140122)
2024-03-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from f8c3b2db8cd1 to 12828950890a (1 revision) (flutter/flutter#144775)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants