Skip to content

Conversation

@adazh
Copy link
Contributor

@adazh adazh commented Aug 6, 2019

Description

This PR added a waitForCondition API that can be used to compose conditions that the client would like to wait for. Currently supported conditions are:

  • NoTransientCallbacksCondition;
  • NoPendingFrameCondition;
  • Or a combined condition of the above two.

In favor of this change, we deprecate the _waitUntilNoTransientCallbacks, _waitUntilNoPendingFrame, _waitUntilFirstFrameRasterized extension APIs.

Related Issues

#37409

Tests

I added the following tests:

  • flutter_driver_test.dart

  • extension_test.dart

  • wait_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?

It deprecates a few Flutter Driver extension wait APIs in favor of the new waitForCondition API:

  • _waitUntilNoTransientCallbacks & WaitUntilNoTransientCallbacks command;

  • _waitUntilNoPendingFrame & WaitUntilNoPendingFrame command;

  • _waitUntilFirstFrameRasterized & WaitUntilFirstFrameRasterized command.

Note, the user facing Driver API waitUntilNoTransientCallbacks, waitUntilFirstFrameRasterized are NOT deprecated, but under the hood, it uses waitForCondition to talk to the Dart service extension API. Thus, we expect minimum user impact unless they use the extension APIs directly (e.g. a testing framework like Espresso/EarlGrey).

@fluttergithubbot fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Aug 6, 2019
@adazh adazh requested review from Hixie, goderbauer and tvolkert August 7, 2019 17:01
@adazh adazh requested a review from goderbauer August 7, 2019 23:00
final Map<String, String> jsonMap = <String, String>{
'conditionName': 'CombinedCondition'
};
final List<Map<String, String>> jsonConditions = <Map<String, String>>[];
Copy link
Member

Choose a reason for hiding this comment

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

Could the following few lines be simplified by using List.map? e.g. conditions.map((WaitCondition c) => ...);

Copy link
Member

Choose a reason for hiding this comment

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

The deserialization logic may also simplify with something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified serialize(). Kept the deserialize() the same, as I didn't find a good solution to handle the type casting from the json.decode() resultdynamic => Map<String, dynamic>...

@adazh adazh added the c: API break Backwards-incompatible API changes label Aug 10, 2019
@adazh adazh requested a review from goderbauer August 10, 2019 00:43
bool get condition => SchedulerBinding.instance.transientCallbackCount == 0;

@override
Future<void> wait() async {
Copy link
Member

Choose a reason for hiding this comment

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

I'd still argue that the default is to wait until the next frame if the condition is currently unmet. FirstFrameRasterizedCondition can still override that to provide a custom impl

@Piinks Piinks added customer: espresso t: flutter driver "flutter driver", flutter_drive, or a driver test labels Aug 12, 2019
@adazh adazh requested a review from goderbauer August 12, 2019 20:56
@adazh
Copy link
Contributor Author

adazh commented Aug 14, 2019

Ping for comments?

@dnfield
Copy link
Contributor

dnfield commented Aug 14, 2019

Is there a link to the breaking change announcment for this?

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Sorry for the late review - most of these should be small.

@adazh adazh requested a review from dnfield August 15, 2019 01:10
@adazh
Copy link
Contributor Author

adazh commented Aug 15, 2019

Is there a link to the breaking change announcment for this?

Thanks for checking! Will work on the announcement.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM, but we should wait until some time after an announcement has been posted to make sure this doesn't break anyone too badly.

@adazh
Copy link
Contributor Author

adazh commented Aug 15, 2019

For sure. I'm going to send an announcement tomorrow, but this change probably won't break any Flutter users, except Espressso :), as Espresso calls the extension APIs directly.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

Please link to the breaking change announcement.

@adazh adazh merged commit 8edc3f7 into flutter:master Aug 19, 2019
@tvolkert
Copy link
Contributor

This broke a bunch of tests on Flutter's devicelab. Please revert.

@adazh
Copy link
Contributor Author

adazh commented Aug 19, 2019

Oh, sorry for the breakage! Working on a revert.

adazh added a commit to adazh/flutter that referenced this pull request Aug 19, 2019
adazh added a commit that referenced this pull request Aug 19, 2019
adazh added a commit to adazh/flutter that referenced this pull request Aug 19, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: tests "flutter test", flutter_test, or one of our tests c: API break Backwards-incompatible API changes customer: espresso framework flutter/packages/flutter repository. See also f: labels. t: flutter driver "flutter driver", flutter_drive, or a driver test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants