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

Enable Android integration tests in remaining plugins #4514

Merged
merged 25 commits into from Nov 20, 2021

Conversation

blasten
Copy link

@blasten blasten commented Nov 15, 2021

@google-cla google-cla bot added the cla: yes label Nov 15, 2021
@blasten blasten changed the title Enable integration tests in remaining plugins Enable Android integration tests in remaining plugins Nov 15, 2021
@stuartmorgan
Copy link
Contributor

Looks like a number of these either aren't set up quite right or are failing tests; I think it would be easier to split them into one PR per plugin. (Looks like video_player worked, so that one we can land easily.)

@stuartmorgan
Copy link
Contributor

I've been trying to do shared_preferences #4505, so if I can get that working we won't need a separate PR for it. I've failed quite a few times though (it's not clear to me how to run things locally the way FTL runs them, so iteration is very slow).

@@ -5,4 +5,10 @@
android:theme="@android:style/Theme.NoTitleBar.Fullscreen"
android:exported="false"/>
</application>
<queries>
<intent>
<action android:name="android.intent.action.SENDTO" />
Copy link
Author

Choose a reason for hiding this comment

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

This was causing an issue in Android 11

Copy link
Contributor

Choose a reason for hiding this comment

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

For which scheme? It sounds like we may need to update the README.

Copy link
Author

Choose a reason for hiding this comment

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

Android manifests are aggregated. There's no need for manual instructions.

Copy link
Author

Choose a reason for hiding this comment

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

SENDTO works for SMS and emails

Copy link
Contributor

Choose a reason for hiding this comment

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

Android manifests are aggregated. There's no need for manual instructions.

Oh, I thought this was in the example app. I don't think we should add this here, because we don't know what schemes someone is going to use. We should instead document this query in the README for people who need it, as we are currently doing (but without this scheme). And then add this to the example instead, so that the test passes.

Copy link
Author

Choose a reason for hiding this comment

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

Done. I don't know why not adding them to the plugin AndroidManifest.xml automatically. I think the idea is to have 3p dependencies auto-configuring themselves when possible unless there's some major concern that prevents these from being the default.

I'm not sure if these concerns exist, or if we didn't know that the Android Gradle plugin merges manifests automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why not adding them to the plugin AndroidManifest.xml automatically.

It is entirely possible to use url_launcher without any permissions (just use launch without canLaunch), and almost nobody would use the entire set of possible queries that canLaunch could require. We shouldn't force people using a plugin to have their app require permissions they aren't using. Especially since the guidance from the stores is always to use the minimal permission set.

if we didn't know that the Android Gradle plugin merges manifests automatically

I didn't know that certainly, and it's good to know 🙂 . We should audit our READMEs for cases where the advice is "you must add X if you use this plugin" rather than "you might need X if you are using this plugin to do Y", and for the former, put it in the plugin manifest.

Copy link
Author

Choose a reason for hiding this comment

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

I haven’t used this plugin, but if canLaunch isn’t required, then the API seems very confusing. I assumed that canLaunch means that if it returns false, then never launch the intent, and handle it differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updating the docs is on my TODO list, because the example shown in the README (await canLaunch(_url) ? await launch(_url) : throw 'Could not launch $_url';) is pointless; it's just a slower and more cumbersome way of writing

if (!(await launch(_url))) throw 'Could not launch $_url';

and handle it differently

This is where canLaunch is actually useful, yes. But with https URLs, for instance, I would be willing to bet that nobody has fallback code in their application that handles the case where someone's device does not have a browser, so calling canLaunch in that cirumstance is pointless.

@blasten
Copy link
Author

blasten commented Nov 16, 2021

@stuartmorgan this is ready for another review

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Looks like camera is still broken. Per my earlier comment: is there a reason not to split that out so we can land the rest now while that's worked out?

@@ -5,4 +5,10 @@
android:theme="@android:style/Theme.NoTitleBar.Fullscreen"
android:exported="false"/>
</application>
<queries>
<intent>
<action android:name="android.intent.action.SENDTO" />
Copy link
Contributor

Choose a reason for hiding this comment

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

For which scheme? It sounds like we may need to update the README.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM with some nits. Thanks for getting all of these working!

packages/camera/camera/CHANGELOG.md Outdated Show resolved Hide resolved
dartMessenger.sendCameraClosingEvent();
super.onClosed(camera);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we need this?

Copy link
Author

Choose a reason for hiding this comment

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

The default implementation of onClosed does nothing. For callbacks, I'd be very surprised if there was an API that has a super implementation that does something. It'd be incorrect to call it a callback.
https://developer.android.com/reference/android/hardware/camera2/CameraDevice.StateCallback#onClosed(android.hardware.camera2.CameraDevice)

packages/url_launcher/url_launcher/README.md Show resolved Hide resolved
@stuartmorgan
Copy link
Contributor

stuartmorgan commented Nov 19, 2021

@blasten I wrote up https://github.com/flutter/flutter/wiki/Plugin-Tests#enabling-android-ui-tests based on everything I learned from my trial and error on shared_preferences and from your PR. Please edit it if you think anything is missing or misleading.

(My goal is to eventually make all of this part of the plugin template, but until then I want to make sure the process for all of our non-trivial test setup is clearly documented.)

@blasten
Copy link
Author

blasten commented Nov 19, 2021

@stuartmorgan yup. That's pretty much it

@blasten blasten added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Nov 19, 2021
@fluttergithubbot fluttergithubbot removed the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Nov 19, 2021
@flutter flutter deleted a comment from fluttergithubbot Nov 19, 2021
@blasten blasten added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Nov 20, 2021
@fluttergithubbot fluttergithubbot merged commit e5156e9 into flutter:master Nov 20, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 20, 2021
amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants