Skip to content

test: Make home-page tests robust to changes in route animation duration #1547

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

Merged
merged 8 commits into from
Jun 4, 2025

Conversation

chrisbobbe
Copy link
Collaborator

This would fix some tests to allow an upstream change to land.

Discussion; see Greg's comment: flutter/flutter#165832 (comment)

We definitely have some tests in Zulip that wait a fixed number of ms when what they really want is "until the page transition finishes", and the real point of the test doesn't care whether that takes 300ms or 800ms or some other value. (I'd expect Zulip to be representative of a lot of apps out there in having some tests of this kind.) So where that's the case, the test would be cleaner if there were some value it could refer to instead.

It might be tricky to specify the value as a constant, though — different transitions may take different durations, right? So maybe the API that's needed is some way to look up the duration (or the remaining duration?) of the page transition that's currently in progress, or perhaps to get a Future that will complete when the transition finishes.

There are more places we use hard-coded durations, but those don't seem to be affected—the four test failures are all in home_test.dart.

This will be useful for some other tests.
This test doesn't need to check the widget tree after waiting for
the route animation to complete. The navigator observer is alerted
when the navigation action is dispatched, not when its animation
completes, so it's fine to check pushedRoutes before waiting through
the animation.

(It still needs a Duration.zero wait so that a FakeApiConnection
timer isn't still pending at the end of the test.)
This duplicates the `find.descendant`s in `tester.tap` callsites,
but that's temporary; we'll deduplicate with a new helper function,
coming up.
This is robust to changes in the entrance-animation duration, and it
checks the state more thoroughly.
As in the previous commit, this removes some more hard-coding of
route-animation durations.
@chrisbobbe chrisbobbe requested a review from rajveermalviya June 4, 2025 02:27
@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Jun 4, 2025
Copy link
Member

@rajveermalviya rajveermalviya left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! Just one non-blocking comment.


final sheetPopDuration = (topRouteBeforePress as ModalBottomSheetRoute<void>)
.reverseTransitionDuration;
// TODO not sure why a 1ms fudge is needed; investigate.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the example provided upstream also has the same 1ms hack: flutter/flutter#165832 (comment).

Also the value of sheetPopDuration here turns out to be 200ms, different from the previous 250ms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder if we tried 200ms when writing the test, but it failed for the same (unknown) reason it fails now, and we added 50ms to hack around instead of just 1ms.

@rajveermalviya rajveermalviya removed the maintainer review PR ready for review by Zulip maintainers label Jun 4, 2025
@rajveermalviya rajveermalviya removed their assignment Jun 4, 2025
@rajveermalviya rajveermalviya added the integration review Added by maintainers when PR may be ready for integration label Jun 4, 2025
@rajveermalviya rajveermalviya requested a review from gnprice June 4, 2025 17:53
@chrisbobbe
Copy link
Collaborator Author

The CI failure is #1545; I have #1546 open for that. (That failure shouldn't affect the customer-tests run, because that's done with flutter analyze --no-fatal-infos, and the analyzer output in #1545 is all info-level.)

I'll go ahead and merge this and send a flutter/tests PR bumping the Zulip commit ID. Thanks for the review! Normally I wouldn't merge without Greg's review, but the changes are only in tests, Greg is OOO through Monday, and I'd like to unblock the upstream work.

@chrisbobbe chrisbobbe merged commit 834834b into zulip:main Jun 4, 2025
1 check failed
@chrisbobbe chrisbobbe deleted the pr-home-tests-route-animations branch June 4, 2025 20:18
chrisbobbe added a commit to chrisbobbe/tests that referenced this pull request Jun 4, 2025
Piinks pushed a commit to chrisbobbe/tests that referenced this pull request Jun 11, 2025
auto-submit bot pushed a commit to flutter/tests that referenced this pull request Jun 11, 2025
…ration (#460)

This updates to the following change in Zulip:
  zulip/zulip-flutter@834834b
  zulip/zulip-flutter#1547

in order to unblock this PR:
  flutter/flutter#165832

> [!IMPORTANT]
> This repository is a testing suite that contains references to tests (in the `registry` directory) that are run with every commit to Flutter
> to verify that no breaking changes have been introduced (in the "customer_testing" shards). Your merged PR is _not_ automatically run in the
> Flutter CI tree.
>
> After merging this PR, you must send another PR to flutter/flutter updating `dev/customer_testing/tests.version` in that repo with the latest git commit SHA of the flutter/test repo.
>
> See <flutter/flutter#162048> for details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants