Added a Driver API that waits until the Flutter app is idle #36599

adazh · 2019-07-20T05:42:30Z

Description

This PR includes:

Added a pendingChannelInvokeMethodCount variable that tracks the # of pending [MethodChannel.invokeMethod] calls.
Added a Driver API that checks and wait until pendingChannelInvokeMethodCount == 0, and considers there's no going on async method channel calls.
Added mock message handlers for the "flutter/platform" channel's "SystemChrome.setApplicationSwitcherDescription" since it's called at start of test initialization, and we'd like to have a default response so that the pendingChannelInvokeMethodCount is 0 when tests are run.

Related Issues

In Espresso tests for the Flutter app, we noticed test flakiness when there are async method channel calls, especially for the ones that takes significant time to get the platform messages back. Thus, we propose to track the pending message channel calls and use it as a signal for the app to go idle.

There are certainly other types of async methods that could potentially lead to UI test flakiness, but we prefer to cover the common use cases that could lead to indeterministic UI state in Espresso tests (like message channels). For other async methods, if there are no common patterns, we can introduce similar IdlingResource to the Dart land.

Tests

I added the following tests:

flutter_driver/test/src/extension_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

adazh · 2019-07-20T05:51:37Z

cc @AlbertWang0116 @khandpur @Hixie

tvolkert

At the high level, my concern with this series of PRs is that we're adding APIs that all feel like they're getting at the same thing:

WaitUntilNoTransientCallbacks
WaitUntilNoPendingFrame
WaitUntilNoPendingChannelMessages

This is going to confuse callers - "Which one should I wait for and under which circumstances? Should I just wait for them all?" I think when faced with these questions, people are likely to just wait for them all. Do we want them to? Will doing so cause them problems in certain circumstances? If we want them to, we should just make this one API - WaitUntilIdle or something along those lines. Which in turn would argue for something similar to Espresso's IdlingResource.

tvolkert · 2019-07-20T17:03:53Z

packages/flutter_driver/test/src/extension_test.dart

+
+    test('returns immediately when there is no pending channel message',
+        () async {
+      FakeAsync().run((FakeAsync async) {


Calling a variable async is just asking for trouble - how about fakeAsync?

Renamed to fakeAsync.

tvolkert · 2019-07-20T17:18:30Z

packages/flutter/lib/src/services/platform_channel.dart

-    if (result == null) {
-      throw MissingPluginException('No implementation found for method $method on channel $name');
+    try {
+      _pendingChannelInvokeMethodCount += 1;


This should go immediately before the try

matthew-carroll · 2019-07-22T21:11:51Z

packages/flutter_driver/lib/src/common/find.dart

-  /// Creates a command that waits until there's no pending frame scheduled.
-  const WaitUntilNoPendingFrame({ Duration timeout }) : super(timeout: timeout);
+/// A Flutter Driver command that waits until the Flutter app is idle.
+class WaitUntilIdle extends Command {


@tvolkert will this need to be announced as a breaking change?

I added this API a couple of days ago (which shouldn't be used anywhere yet...), and kept the waitUntilNoTransientCallbacks API. LMK if this is a breaking change.

Since that API is so young, maybe revert that change so nobody starts depending on it until we have agreement what API we actually want to add?

packages/flutter_driver/lib/src/extension/extension.dart

packages/flutter/lib/src/services/platform_channel.dart

goderbauer · 2019-07-24T10:07:25Z

packages/flutter_driver/lib/src/extension/extension.dart

+  /// should instead disable the frame sync using the `set_frame_sync` method.
+  /// See [FlutterDriver.runUnsynchronized] for more details on how to do this.
+  /// Note, disabling frame sync will require the test author to use some other method to avoid flakiness.
+  Future<Result> _waitUntilIdle(Command command) async {


There should be a link to the other similar method waitUntilNoTransientCallbacks and an explanation when to use what.

Shall we just deprecate the waitUntilNoTransientCallbacks after we add this waitUntilIdle API? I don't think a test author/developer actually understand what "transient callbacks" means.

goderbauer · 2019-07-24T10:08:57Z

packages/flutter_driver/lib/src/extension/extension.dart

-          && !SchedulerBinding.instance.hasScheduledFrame;
+  /// should instead disable the frame sync using the `set_frame_sync` method.
+  /// See [FlutterDriver.runUnsynchronized] for more details on how to do this.
+  /// Note, disabling frame sync will require the test author to use some other method to avoid flakiness.


The doc comment will not appear in our API docs since the method is private. Maybe add the comment to the corresponding method in FlutterDriver?

Ya... if you think it makes sense to expose this API on Driver, I can add it in a follow-up PR.

Why not in this one?

My main goal for this PR is to make Espresso tests more reliable by calling this waitUntilIdle API.

As you noted in other comments, I think it makes sense to sort out the relationship between this one and the waitUntilNoTransientCallbacks before exposing it to Flutter Driver users... And I suspect it's going to take time and could be a breaking change. Also, considering I'm not familiar with Flutter's release process (e.g. how to handle breaking changes), I don't want to include all the changes in one PR. At the same time, it doesn't block Espresso.

Hope this works for you.

@tvolkert + Todd on this thread. Let me know what would be the best way to move forward.

packages/flutter_driver/lib/src/extension/extension.dart

goderbauer · 2019-07-24T10:12:38Z

packages/flutter_driver/lib/src/extension/extension.dart

-      return SchedulerBinding.instance.transientCallbackCount == 0
-          && !SchedulerBinding.instance.hasScheduledFrame;
+  /// should instead disable the frame sync using the `set_frame_sync` method.
+  /// See [FlutterDriver.runUnsynchronized] for more details on how to do this.


Should we document on runUnsynchronized that it changes the behavior of waitUntilIdle?

On second thought, isn't this interaction kinda wired? Why does runUnsynchronized change the definition of what "idle" means? With this implementation, waiting for idle while running unsynchronized will in fact not wait for the app to be idle...

Not sure if I understand your question here... I think runUnsyncrhonized will disable "frame sync"?

With the current implementation, if you execute a waitUntilIdle command within runUnsynchronized you are not waiting until the app is idle. That seems odd to me given the method name. So, I was wondering why you decided to make waitUntilIdle depend on set_frame_sync.

OK. Got your question.

IMHO, I think it should be fine to call this waitUntilIdle within runsynchronized and it waits until resources are idle except the frame sync. And if an app has animations running all the time, but developers want to test other parts of the app, they have the option to disable frame sync but call this waitUntilIdle to ensure everything else is idle.

On the other hand, since Driver/extension API has the notion of "frame sync" mechanism, I think it makes sense to respect that in a waitUntilIdle API. Otherwise, these APIs act inconsistently: _waitForElement API respects frame sync, but this one doesn't.

goderbauer · 2019-07-24T10:16:28Z

packages/flutter_driver/lib/src/common/find.dart

-  /// Creates a command that waits until there's no pending frame scheduled.
-  const WaitUntilNoPendingFrame({ Duration timeout }) : super(timeout: timeout);
+/// A Flutter Driver command that waits until the Flutter app is idle.
+class WaitUntilIdle extends Command {


Any reason we do not expose this command (like the other commands) in the FlutterDriver API? e.g.

flutter/packages/flutter_driver/lib/src/driver/driver.dart

Lines 475 to 477 in 5318670

Future<RenderTree> getRenderTree({ Duration timeout }) async {

return RenderTree.fromJson(await _sendCommand(GetRenderTree(timeout: timeout)));

}

Ya... Espresso can just call this extension API but if you think it makes sense to expose to the Driver API, happy to make that change in a follow-up PR. But we might want to sort out the relationship between this one and the waitUntilNoTransientCallbacks: shall we simply deprecate the waitUntilNoTransientCallbacks method? After we have this waitUntilIdle API, I don't see there are cases that users would want to use the waitUntilNoTransientCallbacks API.

We should probably sort out that relationship before we add more API.

codecov · 2019-07-25T08:04:08Z

Codecov Report

Merging #36599 into master will decrease coverage by 0.74%.
The diff coverage is 53.63%.

@@            Coverage Diff             @@
##           master   #36599      +/-   ##
==========================================
- Coverage   55.95%   55.21%   -0.75%     
==========================================
  Files         188      190       +2     
  Lines       17432    17603     +171     
==========================================
- Hits         9754     9719      -35     
- Misses       7678     7884     +206

Flag	Coverage Δ
#flutter_tool	`55.21% <53.63%> (-0.75%)`	⬇️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/device.dart	`65.06% <ø> (-2.56%)`	⬇️
...lutter_tools/lib/src/commands/update_packages.dart	`3.9% <ø> (ø)`	⬆️
...ackages/flutter_tools/lib/src/commands/unpack.dart	`6.93% <ø> (+0.06%)`	⬆️
.../flutter_tools/lib/src/commands/build_fuchsia.dart	`81.81% <ø> (ø)`	⬆️
packages/flutter_tools/lib/src/desktop.dart	`86.95% <ø> (+8.38%)`	⬆️
packages/flutter_tools/lib/src/base/io.dart	`64.86% <0%> (-5.73%)`	⬇️
...es/flutter_tools/lib/src/commands/build_macos.dart	`26.31% <0%> (-61.92%)`	⬇️
...ackages/flutter_tools/lib/src/commands/create.dart	`72.78% <0%> (ø)`	⬆️
packages/flutter_tools/lib/src/android/aar.dart	`0% <0%> (ø)`
packages/flutter_tools/lib/src/features.dart	`100% <100%> (ø)`	⬆️
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cdb4dd...fe7a519. Read the comment docs.

Hixie · 2019-07-25T22:46:43Z

This doesn't really wait for idleness (e.g. it doesn't wait for timers, network traffic, tasks that are started by a channel message that immediately replies and sends the result later, etc), so it might be better to name it something more descriptive of what it actually waits for.

adazh · 2019-07-26T01:21:53Z

Right, the current implementation only checks a couple of idleness signals, not all of them, but I'd like to evolve the implementation gradually.

Eventually I envision the Flutter synchronization API would be something close to:

Have a waitUntilIdle API that include common idleness checks that work for most of the cases; The testing framework or developers can call this method to check for idleness (without having to know all the details).
Provide specific waiting APIs or a customizable framework for users to register for their custom checks if they need to. Something like the IdlingResource API in Espresso.

I'm still experimenting with the synchronization mechanism & API (also, note, we probably don't want to ensure everything is idle but these that effect UI). And I think there are two ways to get to the end goal:

Define all the specific idle checks, and I actually started from there, but got three wait APIs quickly:

waitUntilNoTransientCallbacks;
waitUntilNoPendingFrames;
waitUntilNoPendingInvokeMethods;
This list will go on and on as we discover more to check.

Or start with the waitUntilIdle method, explain clearly what it checks in the documentation, and evolves this API as we understand more of the synchronization needs.

Comparing these two approaches, I decided to go with 2), as it actually aligns better:
Having a common waitUntilIdle method; but for approach 1), it's likely we'll have a long list of these APIs, and eventually throw them away if they turn out to be something that should be merged to the waitUntilIdle API.

I can add comments to explain that this waitUntilIdle is still experimental and hide it from the Driver API before it becomes more mature. WDYT?

Hixie · 2019-07-26T01:32:02Z

is there some way to solve this by composition? Having components for each mechanism, that you can combine?

adazh · 2019-07-26T01:45:51Z

I didn't see there's a good extensible way but you may have suggestions.

We probably shouldn't do this:
waitUntilIdle() { return Futures.wait( _waitUntilNoTransientCallbacks(), _waitUntilNoPendingFrames(), _waitUntilNoPendingInvokeMethods()) }
As we want all the conditions hold at the same time when the waitUntilIdle future completes.

Possible improvement by wrapping different conditions into separate methods. Something like:
bool hasNoTransientCallbacks() { return SchedulingBinding.transientCallbacks == 0; } waitUntilIdle() { return _waitUntilCondition( hasNoTransientCallbacks() && hasNoPendingFrames() && ...); }

packages/flutter_driver/lib/src/extension/extension.dart

tvolkert · 2019-07-27T00:45:23Z

packages/flutter/lib/src/services/platform_channel.dart

+  ///
+  /// This method is useful for a testing framework like Espresso to monitor
+  /// ongoing platform channel messages and decide whether the app is idle.
+  static int get pendingChannelInvokeMethodCount => _pendingChannelInvokeMethodCount;


I think if this were exposed through a listener registration similar to SchedulerBinding.instance.addPostFrameCallback(), it would clean up some smells elsewhere in this PR, such as the 10ms polling.

It'd probably amount to adding API to SchedulerBinding - something like addMethodChannelsIdleCallback() (and moving the state variable into the binding, which also means it no longer needs to be static, which is nice)

Then again, this work isn't being scheduled in the way that fits perfectly into the existing SchedulerBinding model. @Hixie @goderbauer wdyt?

tvolkert · 2019-07-27T00:46:43Z

packages/flutter_driver/lib/src/extension/extension.dart

  }
+
+  Future<void> _waitUntilCondition(bool condition(),
+      [Duration duration = const Duration(milliseconds: 10)]) {


Regardless of the duration, polling seems wrong here - see my comment above about making this callback based.

Hixie · 2019-07-30T01:46:09Z

We probably shouldn't do this:
waitUntilIdle() { return Futures.wait( _waitUntilNoTransientCallbacks(), _waitUntilNoPendingFrames(), _waitUntilNoPendingInvokeMethods()) }
As we want all the conditions hold at the same time when the waitUntilIdle future completes.

Sure, you'd want a solution a little more elaborate than this, where each waiter can say when it is met and when it is not, and you only go when all of them are met simultaneously.

goderbauer · 2019-08-07T21:44:22Z

@adazh Can we close this PR in favor of the other PRs?

adazh · 2019-08-07T21:49:55Z

Ya, I should.

Created Flutter synchronization support for Espresso/EarlGrey #37409 for tracking the Flutter synchronization support for Espresso/EarlGrey.
Two PRs under review: Moved the default BinaryMessenger instance to ServicesBinding #37489, Added a composable waitForCondition Driver/extension API #37736.

Added a Driver API that waits until there's no pending channel messages

7d0fb83

fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Jul 20, 2019

googlebot added the cla: yes label Jul 20, 2019

adazh requested review from collinjackson, goderbauer, matthew-carroll, mehmetf, tvolkert and xster July 20, 2019 05:48

Fixed the analyzer warnings

38a37e6

tvolkert suggested changes Jul 20, 2019

View reviewed changes

adazh added 2 commits July 21, 2019 17:13

Updated the wait API to waitUntilIdle

fb1ca6f

Fixed trailing whitespaces

3449b8c

adazh requested a review from tvolkert July 22, 2019 01:12

matthew-carroll approved these changes Jul 22, 2019

View reviewed changes

adazh mentioned this pull request Jul 23, 2019

Added a DartExecutor API for querying # of pending channel callbacks flutter/engine#10021

Merged

adazh changed the title ~~Added a Driver API that waits until there's no pending channel messages~~ Added a Driver API that waits until the Flutter app is idle Jul 23, 2019

goderbauer reviewed Jul 24, 2019

View reviewed changes

Addressed goderbauer@'s comments

6a856a4

adazh added 2 commits July 25, 2019 14:00

Addressed comments

41136bd

Fixed trailing whitespaces

fe7a519

tvolkert reviewed Jul 27, 2019

View reviewed changes

adazh mentioned this pull request Aug 1, 2019

Flutter synchronization support for Espresso/EarlGrey #37409

Closed

adazh closed this Aug 7, 2019

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021

	Future<RenderTree> getRenderTree({ Duration timeout }) async {
	return RenderTree.fromJson(await _sendCommand(GetRenderTree(timeout: timeout)));
	}

Added a Driver API that waits until the Flutter app is idle #36599

Added a Driver API that waits until the Flutter app is idle #36599

Uh oh!

Conversation

adazh commented Jul 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

adazh commented Jul 20, 2019

Uh oh!

tvolkert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adazh Jul 25, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Hixie commented Jul 25, 2019

Uh oh!

adazh commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hixie commented Jul 26, 2019

Uh oh!

adazh commented Jul 20, 2019 •

edited

Loading

adazh Jul 25, 2019 •

edited

Loading

codecov bot commented Jul 25, 2019 •

edited

Loading

adazh commented Jul 26, 2019 •

edited

Loading