test: Get route-transition duration robustly,instead of hard-coding

[MdSaifAliMolla]

Work towards #1668

[gnprice]

Thanks. Some of these changes look good, but there are other places where a lot more code is getting changed and it's not obvious what the reasons for those changes are. That makes it considerably more work for someone to review, because they have to read through all those changes and reverse-engineer what your intent is.

Please take a look at our guide to writing a clean commit history:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#write-a-clean-commit-history
and then try using those ideas to make this PR an efficient one for someone to review. So:

• avoid any changes that aren't relevant;
• and if there are places where you think a more complex change is needed, make that a separate commit with a commit message explaining why.

[MdSaifAliMolla]

@gnprice I guess it's fine now.

[gnprice]

This is improved, but there are still some places like this:

where a lot more code is getting changed and it's not obvious what the reasons for those changes are. That makes it considerably more work for someone to review, because they have to read through all those changes and reverse-engineer what your intent is.

For example:

-      testWidgets('can show snackbar on success', (tester) async {
-        // Regression test for: https://github.com/zulip/zulip-flutter/issues/732
-        testBinding.deviceInfoResult = const IosDeviceInfo(systemVersion: '16.0');
-
-        final message = eg.streamMessage();
-        await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(messa
ge));
+    testWidgets('can show snackbar on success', (WidgetTester tester) async {
+      // Regression test for: https://github.com/zulip/zulip-flutter/issues/732
+      final TransitionDurationObserver transitionDurationObserver = TransitionDurationObserver();
+      await tester.pumpWidget(
+        MaterialApp(
+          navigatorObservers: <NavigatorObserver>[transitionDurationObserver],
+          home: Scaffold(
+            body: Builder(
+              builder: (context) {
+                testBinding.deviceInfoResult = const IosDeviceInfo(systemVersion: '16.0');
+                final message = eg.streamMessage();
+                setupToMessageActionSheet(tester,message: message,narrow: TopicNarrow.ofMessage(messa
ge));
+                return const SizedBox.shrink();}
+            ))));
+      final message = eg.streamMessage();
+       // Make the request take a bit of time to complete
+      prepareRawContentResponseSuccess(message: message,rawContent: 'Hello world',delay: const Durati
on(milliseconds: 500),
+      );
+      await tapCopyMessageTextButton(tester);
+      // … and pump a frame to finish the NavigationState.pop animation…
+      await transitionDurationObserver.pumpPastTransition(tester);
+      // … before the request finishes.  This is the repro condition for #732.
+      await transitionDurationObserver.pumpPastTransition(tester);
 
-        // Make the request take a bit of time to complete…
-        prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world',
-          delay: const Duration(milliseconds: 500));
-        await tapCopyMessageTextButton(tester);
-        // … and pump a frame to finish the NavigationState.pop animation…
-        await tester.pump(const Duration(milliseconds: 250));
-        // … before the request finishes.  This is the repro condition for #732.
-        await tester.pump(const Duration(milliseconds: 250));
+      final snackbar = tester.widget<SnackBar>(find.byType(SnackBar));
+      check(snackbar.behavior).equals(SnackBarBehavior.floating);
 
-        final snackbar = tester.widget<SnackBar>(find.byType(SnackBar));
-        check(snackbar.behavior).equals(SnackBarBehavior.floating);
-        final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
-        tester.widget(find.descendant(matchRoot: true,
+      final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
+      tester.widget(find.descendant(matchRoot: true,
           of: find.byWidget(snackbar.content),
-          matching: find.text(zulipLocalizations.successMessageTextCopied)));
-      });
+          matching: find.text(zulipLocalizations.successMessageTextCopied),
+        ));
+    });
+

Take a look again at the changes in your PR. Use git log -p, or a graphical Git viewer, to see exactly what changes you have. Then please revise to accomplish this as far as you can:

[…] to make this PR an efficient one for someone to review. So:

• avoid any changes that aren't relevant;
• and if there are places where you think a more complex change is needed, make that a separate commit with a commit message explaining why.

[MdSaifAliMolla]

@gnprice done. Followed this flutter/flutter#171109 coding convention as suggested here #1668, otherwise the tests kept failing. fixed indentation mismatch related diffs(mostly).

[gnprice]

This revision is somewhat improved again, but there are still some irrelevant changes, and still some places where more code is getting changed and it's not clear exactly why.

If you need help in doing this:

Take a look again at the changes in your PR. Use git log -p, or a graphical Git viewer, to see exactly what changes you have. Then please revise to accomplish this as far as you can:

[…] to make this PR an efficient one for someone to review. So:

• avoid any changes that aren't relevant;
• and if there are places where you think a more complex change is needed, make that a separate commit with a commit message explaining why.

then the #git-help channel on chat.zulip.org is a good place to ask.

fixed indentation mismatch related diffs(mostly).

Why mostly and not entirely?

Followed this flutter/flutter#171109 coding convention as suggested here #1668, otherwise the tests kept failing.

I don't understand this comment. #1668 is the issue this PR is meant to fix, so in general these changes should be in the direction called for by that issue. If you're encountering test failures that you don't understand the cause of, then the #mobile-dev-help channel is the best venue to ask about them.

[MdSaifAliMolla]

@gnprice cleaned up the pr. added transitionDurationObserver inside prepareComposeBox setup (following setupToMessageActionSheet's code convention), every test that calls it automatically has an observer attached to the widget tree. And in this part,

        await transitionDurationObserver.pumpPastTransition(tester);
        // … before the request finishes.  This is the repro condition for #732.
        await transitionDurationObserver.pumpPastTransition(tester);
        // …pump for snackbar to show
        await tester.pumpAndSettle();

though pumpAndSettle is not preferred but I saw multiple uses ot it in the same file, so used it instead of hardcoding a time duration, or using await transitionDurationObserver.pumpPastTransition(tester) again (maybe works because pumpPastTransition() internally calls tester.pump() for a fixed duration and snackbar animation gets completed within that time).

Also, in showFromInbox, transitionDurationObserver was initialized like this

Future<void> showFromInbox(WidgetTester tester) async {
      transitionDurationObserver = TransitionDurationObserver();
      await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
        navigatorObservers: [transitionDurationObserver],
        child: const HomePage()));
      await tester.pump();
      check(find.byType(InboxPageBody)).findsOne();

      await tester.longPress(find.text(someChannel.name).hitTestable());
      await transitionDurationObserver.pumpPastTransition(tester);
    }

but in showFromTopicListAppBar, like this,

Future<void> showFromTopicListAppBar(WidgetTester tester, {int? streamId}) async {
    streamId ??= someChannel.streamId;
    final transitionDurationObserver = TransitionDurationObserver();

    connection.prepare(json: GetStreamTopicsResult(topics: []).toJson());
    await tester.pumpWidget(TestZulipApp(
      navigatorObservers: [transitionDurationObserver],
      accountId: eg.selfAccount.id,
      child: TopicListPage(streamId: streamId)));
    await tester.pump();
     .......

both works but I followed showFromInbox method.

[MdSaifAliMolla]

@gnprice have a look.

[gnprice]

Cool, this version seems clear enough for a review.

Please edit the commit message and PR description so it doesn't say it fixes #1668 yet, though. This covers some of the places that need to change but not all of them. (But let's not try to put more of them into this first PR — we can save them for a subsequent PR after you've gotten feedback on this one.)

[MdSaifAliMolla]

@gnprice done.

[chrisbobbe]

Thanks! Comments below.

Some of the commit-message bodies are a bit messy, as though you did some quick copy-and-paste without checking for space after punctuation, etc., and without wrapping to 68 columns. Please clean those up. 🙂

[chrisbobbe]

In the code comment, the part after the semicolon ";" doesn't refer to anything; remove it. (Here and in similar comments on code touched in this PR.)

[chrisbobbe]

How long is the snackbar entrance animation? Ideally we would pump with exactly that duration, rather than using pumpAndSettle.

[chrisbobbe]

Thanks! One comment below.

[chrisbobbe]

Ah: looking at the story of this test, I think the last two of these pumpPastTransition calls aren't actually desired.

The first 250ms pump was a route transition, as its comment says:

        // … and pump a frame to finish the NavigationState.pop animation…
        await tester.pump(const Duration(milliseconds: 250));

So we do want to convert that to use pumpPastTransition.

But the second 250ms pump is about pumping through the remainder of a 500ms API-request delay prepared above:

        // Make the request take a bit of time to complete…
        prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world',
          delay: const Duration(milliseconds: 500));

So that's not about a route transition, and we shouldn't use pumpPastTransition there.

And from a quick check of the snackbar code, its appearance isn't implemented as a route transition, so we don't want pumpPastTransition for that either. It looks like the snackbar animation duration is 250ms by default, but that value isn't part of the snackbar code's public API; it's in a private variable, along with the default display duration:

const Duration _snackBarTransitionDuration = Duration(milliseconds: 250);
const Duration _snackBarDisplayDuration = Duration(milliseconds: 4000);

The goal of the test is to check on the snackbar at some point while it's on the screen, right; how about the following:

        // Make the request take a bit of time to complete…
        prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world',
          delay: const Duration(milliseconds: 500));
        await tapCopyMessageTextButton(tester);
        // … and pump a frame to finish the NavigationState.pop animation…
        await transitionDurationObserver.pumpPastTransition(tester);
        // … before the request finishes and the snackbar shows.
        // This is the repro condition for #732.
        await tester.pump(Duration(milliseconds: 500));
        // (Default duration of the snackbar entrance animation)
        await tester.pump(Duration(milliseconds: 250));

[MdSaifAliMolla]

Done. Thanks for the explanation.

[chrisbobbe]

Thanks! LGTM, marking for Greg's review.

[gnprice]

Hmm — it seems awfully messy for there to be so many different functions that are setting this shared variable. When does it need to get set? How did you confirm for yourself that you've set it in all of the places it needs to get set in?

In short, this setup seems likely to cause bugs.

[gnprice]

In this instance, we're using transitionDurationObserver and it appears to work, even though a few lines above here there was a pumpWidget(TestZulipApp(… and it didn't pass navigatorObservers.

Does that mean we don't need to set navigatorObservers after all in the other "showFoo" functions where this PR adds it? Or, conversely, is it a bug that we're not setting it here?

[gnprice]

What's this change about? (And what does the comment mean? It doesn't seem to make any sense syntactically — this new comment line starts with a "…" as if it's continuing something, but the previous comment ends with a "." and the end of a sentence.)