-
Notifications
You must be signed in to change notification settings - Fork 260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix android open link verify #1999
Fix android open link verify #1999
Conversation
Whoah, this is awesome. Thanks!
Could you share this code? We could add this to our e2e testing suite:
Alternatively, the relevant intent filter stuff could be added to our demo_app. Wdyt?
This is a action item for us to run on all supported API levels in CI. Currently we run only on API 28. |
val apkPath = dadb.shell("pm list packages -f --user 0 | grep $appId | head -1") | ||
.output.substringAfterLast("package:").substringBefore("=$appId") | ||
apkPath.substringBefore("=$appId") | ||
val apkPath = dadb.shell("pm path $appId").output.removePrefix("package:").trim() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for making it shorter.
try { | ||
dadb.pull(dst, apkPath) | ||
} catch (e: IOException) { | ||
val newApkPath = "/sdcard/$appId.apk" | ||
dadb.shell("cp $apkPath $newApkPath") | ||
dadb.pull(dst, newApkPath) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? I'm curious to learn why /sdcard
is used as fallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I encountered a
permission denied
when pulling the APK on some of the API levels - This is because
/data
is not accessible to pull from without the device being rooted in those cases - However running
shell cp
and copying the files over is possible - And
/sdcard
is accessible to pull from
See this SO answer for example
If we add multi-API-level testing on the CI, we should add simple tests for AndroidAppFiles
as this is the kind of things that breaks with API changes.
This topic is much more complex that I though. Thank you very much for sharing your experience and contributing. If you think that the documentation for |
Proposed changes
TL;DR
I fixed a whole lot of stuff that was wrong with the
autoVerify
. It should now work as expected.Note
I cherry picked the commit from #1998 locally as I seemed to stumble on that same issue in other places as I tested
Long version
apkMeta.name
<application android:label="name">
in the Manifest. Which obviously is not mandatory, not guaranteed to be there, and even better, not what the chooser shows as valueString!
from Java-world)autoVerify
and I opened another can of worms...autoVerify
is supposed to do 2 things. Select the right app from the Chooser and validate some of the onboarding screens from Chrome. Both had issues in the way they were implementedintent.resolveActivity(packageManager)
in order to get the labels associated with the right ActivitiesautoVerify
is selectedTesting
This is interesting to test. I'm not sure how we can automate that (or if we should). It would make a lot of integration tests. How were the various drivers tested?
What I did
dev.mobile.maestro.app
that I used to test the null nameChrome
Chooser
Open by default
Issues fixed
Fixes #1956 (Definitely fixed)
Fixes #1397 (Fixed in spirit but not in letter. I think it's just better to use
autoVerify
for that purpose and not add another option)