Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[battery] Support the v2 Android embedder #2152

Merged
amirh merged 11 commits into from
Oct 14, 2019
Merged

[battery] Support the v2 Android embedder #2152

amirh merged 11 commits into from
Oct 14, 2019

Conversation

@amirh
Copy link
Contributor

@amirh amirh commented Oct 4, 2019 •
edited
Loading

To run the e2e Android tests the same way they would be run on CI:

cd packages/battery/example/android
./gradlew connectedAndroidTest -Ptarget=`pwd`/../../test/battery_e2e.dart

Related Issues

flutter/flutter#41833

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.
  • [] My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

collinjackson
collinjackson reviewed Oct 7, 2019
View reviewed changes
packages/battery/android/src/main/java/io/flutter/plugins/battery/BatteryPlugin.java Outdated
this.registrar = registrar;
@Override
public void onAttachedToEngine(FlutterPluginBinding binding) {
applicationContext = binding.getApplicationContext();
Copy link
Contributor

@collinjackson collinjackson Oct 7, 2019

Choose a reason for hiding this comment

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

I think this is redundant with this.applicationContext = applicationContext; in the other onAttachedToEngine

@collinjackson collinjackson mentioned this pull request Oct 7, 2019
Merged
3 tasks
blasten
blasten reviewed Oct 9, 2019
View reviewed changes
@amirh amirh force-pushed the battery_embedder_v2 branch from 72475d8 to 541b33b Compare October 14, 2019 19:49
@amirh
Copy link
Contributor Author

amirh commented Oct 14, 2019

I believe this is now ready to land (including bumping the Flutter SDK constraint, adding CI-ready e2e tests, and making sure it builds on stable and master).

@collinjackson @mklim

collinjackson
collinjackson reviewed Oct 14, 2019
View reviewed changes
Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

a few minor comments, lgtm otherwise

collinjackson
collinjackson approved these changes Oct 14, 2019
View reviewed changes
packages/battery/test/battery_e2e.dart Outdated
void main() {
E2EWidgetsFlutterBinding.ensureInitialized();

testWidgets('Builds and runs', (WidgetTester tester) async {
Copy link
Contributor

@collinjackson collinjackson Oct 14, 2019

Choose a reason for hiding this comment

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

This is a nit and we're not doing it consistently in first-party unit tests but since this is supposed to be a canonical example of an integration tested plugin, I'm wondering if you think we should wrap the test in a group('$Battery', () { ... });

And the description of the test could be 'level' since that's the method you're invoking.

https://github.com/flutter/plugins/blob/master/packages/image_picker/test/image_picker_test.dart

Copy link
Contributor Author

@amirh amirh Oct 14, 2019

Choose a reason for hiding this comment

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

I don't think group('Battery') adds much as all tests in this file belong to that group...

Copy link
Contributor Author

@amirh amirh Oct 14, 2019

Choose a reason for hiding this comment

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

(renamed the test to 'Can get battery level')

@amirh amirh merged commit 326b21b into flutter:master Oct 14, 2019
@collinjackson
Copy link
Contributor

collinjackson commented Oct 15, 2019

If you encounter Default interface methods are only supported starting with Android N errors, change connectedAndroidTest to app:connectedAndroidTest

mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
@amirh @mormih
[battery] Support the v2 Android embedder (flutter#2152)
74111ca
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
@amirh
[battery] Support the v2 Android embedder (flutter#2152)
2ec5fe0
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
@collinjackson
[firebase_crashlytics] Remove WorkspaceSettings.xcsettings (flutter#2152
5508d7d 
)

* Remove WorkspaceSettings.xcsettings
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@blasten blasten blasten left review comments

@collinjackson collinjackson collinjackson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@amirh @collinjackson @blasten @googlebot