Skip to content

Fix plugin template app's tests#39080

Merged
collinjackson merged 13 commits intoflutter:masterfrom
collinjackson:ensureInitialized
Aug 26, 2019
Merged

Fix plugin template app's tests#39080
collinjackson merged 13 commits intoflutter:masterfrom
collinjackson:ensureInitialized

Conversation

@collinjackson
Copy link
Contributor

@collinjackson collinjackson commented Aug 22, 2019 •
edited
Loading

Description

Plugin template app needs to be updated with this breaking change:

https://groups.google.com/forum/#!msg/flutter-announce/sHAL2fBtJ1Y/mGjrKH3dEwAJ

#37489 proposed to move the default BinaryMessenger instance to ServicesBinding, and deprecate the former. This is going to be a breaking change, if users reference to the defaultBinaryMessenger directly. Instead, users will use ServicesBinding.instance.defaultBinaryMessenger to access the default BinaryMessenger that's used for sending platform messages.

Why this change?
By this change, we are capable of registering a different default BinaryMessenger under testing environment, by creating a ServicesBinding subclass for testing. With that, we can track # of pending platform messages for synchronization purposes. See more details in #37409.

Path to migrate
You'll use ServicesBinding.instance.defaultBinaryMessenger to access the default BinaryMessenger. In your tests, make sure the ServicesBinding is properly initialized before accessing any message channels.

If you have any concerns or otherwise feel that this change should not be made, please comment on the above PR or on the associated issue: #37409 -- or just reply to this email.

Related Issues

Fixes #39077

Tests

I included the following tests:

  • dev/devicelab/lib/tasks/plugin_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.

@dnfield dnfield added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Aug 22, 2019
@collinjackson collinjackson requested a review from adazh August 22, 2019 20:32
@collinjackson collinjackson mentioned this pull request Aug 22, 2019
Closed
@collinjackson collinjackson requested a review from tvolkert August 22, 2019 20:45
tvolkert
tvolkert approved these changes Aug 22, 2019
View reviewed changes
Copy link
Contributor

@tvolkert tvolkert left a comment

Choose a reason for hiding this comment

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

LGTM

adazh
adazh reviewed Aug 22, 2019
View reviewed changes
dev/devicelab/lib/tasks/plugin_tests.dart Outdated

Future<void> addPlugin(String plugin) async {
Future<void> addPlugin(String plugin, {String pluginPath}) async {
final File pubspec = File(path.join(rootPath, 'pubspec.yaml'));
Copy link
Contributor

@adazh adazh Aug 22, 2019

Choose a reason for hiding this comment

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

Does addPlugin have to be a method on the _FlutterPlugin class? It looks to me the two classes _FlutterProject and _FlutterPlugin are quite similar except the binary/project name? Wondering whether you'll have to define two classes.

Copy link
Contributor Author

@collinjackson collinjackson Aug 22, 2019

Choose a reason for hiding this comment

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

_FlutterPlugin extends _FlutterProject.

I've moved addPlugin from _FlutterApp into _FlutterProject and test from _FlutterPlugin into _FlutterProject since they make sense for both plugins and apps.

I haven't written any test that have plugins depending on plugins, but I added a line that ensures that the template app's unit tests pass.

adazh
adazh reviewed Aug 22, 2019
View reviewed changes
dev/devicelab/lib/tasks/plugin_tests.dart Outdated
}

class _FlutterPlugin extends _FlutterProject {
_FlutterPlugin(Directory parent, String name) : super(parent, name);
Copy link
Contributor

@adazh adazh Aug 22, 2019

Choose a reason for hiding this comment

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

Is it possible to have everything in the _FlutterProject class?
It appears you can define a create method with template, project_name/binary_name arguments, and simply create:

  • plugin by calling create('plugin', 'plugintest', ...);
  • app by calling create('app', 'plugintestapp').

Copy link
Contributor Author

@collinjackson collinjackson Aug 23, 2019

Choose a reason for hiding this comment

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

I could go either way on this. Since it's not a public API and is confined to this file it's easy to change later. I've consolidated everything into one class since you seem to prefer that.

adazh
adazh reviewed Aug 22, 2019
View reviewed changes
@collinjackson collinjackson merged commit a1c185f into flutter:master Aug 26, 2019
@collinjackson collinjackson deleted the ensureInitialized branch August 26, 2019 21:57
@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.

Reviewers

2 more reviewers

@tvolkert tvolkert tvolkert approved these changes

@adazh adazh adazh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

flutter create --template plugin has failing tests

5 participants

@collinjackson @tvolkert @adazh @googlebot @dnfield