Skip to content

Conversation

@miechoo
Copy link
Contributor

@miechoo miechoo commented Sep 14, 2024

Part of #130459

This pull request adds tests for the example files shown in the GlowingOverscrollIndicator Flutter API reference documentation.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Sep 14, 2024
@miechoo miechoo changed the title 130459 overscroll indicator Missing tests of GlowingOverscrollIndicator Sep 14, 2024
nate-thegrate

This comment was marked as outdated.

@nate-thegrate nate-thegrate marked this pull request as draft September 14, 2024 20:10
@nate-thegrate nate-thegrate self-requested a review September 17, 2024 16:29
@nate-thegrate nate-thegrate marked this pull request as ready for review September 17, 2024 16:32
Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

I went ahead and pushed the example updates to this branch.

We should be good to add tests that look for the GlowingOverscrollIndicator now!

@nate-thegrate nate-thegrate changed the title Missing tests of GlowingOverscrollIndicator Fix GlowingOverscrollIndicator examples Sep 17, 2024
@miechoo
Copy link
Contributor Author

miechoo commented Sep 20, 2024

Please let me know if these tests are sufficient. I encountered some unexpected behaviors during the test, so I decided not to test behaviors for now.

Different occurrences

I tried:

testWidgets('raw occurrences', (WidgetTester tester) async {
    await tester.pumpWidget(const example.GlowingOverscrollIndicatorExampleApp());
    final Finder goiFinder = find.byType(GlowingOverscrollIndicator);
    print(goiFinder);
});

and when testing glowing_overscroll_indicator.0.dart the output returns one widget which is expected, but when testing glowing_overscroll_indicator.1.dart the output returns two widgets which is a bit confusing to me. In the second case, the widget tree structure is:

- GlowingOverscrollIdicatorExampleApp
  - MaterialApp
    - Scaffold
      - GlowingOverscrollIdicatorExample
        - NestedScrollView
          - GlowingOverscrollIdicator
            - SliverAppBar
              - Text
            - CustomScrollView
              - GlowingOverscrollIdicator
                - ...
                  - ...

First I thought it was a result of using NestedScrollView, but I guess it's not.

Dragging and calculating the position

I tried to test both examples using await tester.drag() and tester.getBottomLeft(goiFinder).dy to check behaviors and calculate positions, but in both cases (glowing_overscroll_indicator.0.dart and glowing_overscroll_indicator.1.dart) I got inconsistent and incomprehensible results.

So, I hope that the visibility tests will be sufficient.

@nate-thegrate
Copy link
Contributor

in both cases (glowing_overscroll_indicator.0.dart and glowing_overscroll_indicator.1.dart) I got inconsistent and incomprehensible results.

After looking it over again, I think that was my fault 😅
I copy-pasted the glowing_overscroll_indicator.0.dart changes into glowing_overscroll_indicator.1.dart, not realizing that it was adding an extra AppBar that shouldn't have been there.

I'll go ahead and push the fix (and restore the tests you added previously); if everything passes then I think we'll be good to merge!

Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

@nate-thegrate nate-thegrate removed the request for review from bleroux September 23, 2024 15:12
Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

@TahaTesser thanks very much for the review! Since I contributed the example updates, I'll respond to those; I'll leave test updates to @miechoo :)

@miechoo miechoo requested a review from TahaTesser September 25, 2024 18:09
@miechoo
Copy link
Contributor Author

miechoo commented Sep 29, 2024

Hi guys. Should we do something more with this PR?

@nate-thegrate
Copy link
Contributor

Generally we give code reviewers 2 weeks before pinging them, since they often have busy lives with many responsibilities. 🙂

(more info here)

@Piinks Piinks force-pushed the 130459_overscroll_indicator branch from e97d01d to 460bcf5 Compare October 22, 2024 22:08
@Piinks Piinks requested a review from bleroux October 22, 2024 22:08
Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

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

LGMT with nit

@nate-thegrate nate-thegrate force-pushed the 130459_overscroll_indicator branch from 9d01435 to 5a5e70b Compare October 30, 2024 19:23
@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Oct 30, 2024

auto label is removed for flutter/flutter/155203, due to - The status or check suite Windows tool_integration_tests_2_9 has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2024
@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2024
@auto-submit auto-submit bot merged commit 2d2ebbe into flutter:master Oct 30, 2024
149 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 31, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 31, 2024
Manual roll requested by [email protected]

flutter/flutter@fe71cad...0fe6153

2024-10-31 [email protected] Roll Flutter Engine from c40b0b602822 to f2154ef3e31c (8 revisions) (flutter/flutter#157926)
2024-10-31 [email protected] Hides added routes before top-most route finishes pushing (flutter/flutter#156104)
2024-10-31 [email protected] Fix cursor on hover expand/collapse icon (#155207) (flutter/flutter#155209)
2024-10-31 [email protected] Add test for `media_query_data.system_gesture_insets.0.dart` (flutter/flutter#157854)
2024-10-31 [email protected] Add and plumb `useImplicitPubspecResolution` across `flutter_tools`. (flutter/flutter#157879)
2024-10-31 [email protected] Roll Flutter Engine from 090c33aeae83 to c40b0b602822 (1 revision) (flutter/flutter#157896)
2024-10-31 [email protected] Roll Flutter Engine from 9295eeb6d3ce to 090c33aeae83 (4 revisions) (flutter/flutter#157893)
2024-10-30 [email protected] Roll Flutter Engine from 2bd854e23b61 to 9295eeb6d3ce (5 revisions) (flutter/flutter#157882)
2024-10-30 [email protected] Adds a new helpful tool exit message for SocketExceptions thrown during mdns discovery (flutter/flutter#157638)
2024-10-30 [email protected] Fix `GlowingOverscrollIndicator` examples (flutter/flutter#155203)
2024-10-30 [email protected] Roll Flutter Engine from 906a7ad88052 to 2bd854e23b61 (1 revision) (flutter/flutter#157878)
2024-10-30 [email protected] Upgrade templates to AGP 8.7/Gradle 8.10.2 (flutter/flutter#157872)
2024-10-30 [email protected] Roll Flutter Engine from 57ed5d338e7e to 906a7ad88052 (13 revisions) (flutter/flutter#157875)
2024-10-30 [email protected] Update Material 3  `LinearProgressIndicator` for new visual style (flutter/flutter#154817)
2024-10-30 [email protected] Roll Flutter Engine from 999797a2f690 to 57ed5d338e7e (5 revisions) (flutter/flutter#157833)
2024-10-30 [email protected] Add hidden `--no-implicit-pubspec-resolution` flag for one stable release. (flutter/flutter#157635)
2024-10-30 [email protected] Roll Packages from 028027e to 7cc1caa (5 revisions) (flutter/flutter#157864)
2024-10-30 [email protected] Mention partial PRs in the contributing docs (flutter/flutter#157863)

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 [email protected],[email protected] 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
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 d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos 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.

4 participants