-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[url_launcher] fix: url_launcher - updated _launchUniversalLinkIos and Added Tests #2552
Conversation
updated _launchUniversalLinkIos to launch the url passed. It could also be done that we remove the url argument passed and open the hardcoded url. Either of the case needs to be done.
|
LGTM but not sure if there might be an initial reason for having that hardcoded link. @cyanglaz added that snippet so might be the best person to approve. If approved, please also update https://github.com/flutter/plugins/blob/master/packages/url_launcher/url_launcher_macos/example/lib/main.dart, which is a copy of that example. |
mklim
left a comment
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.
Thank you for the contribution! The bugfix itself LG.
This PR also needs tests before it can be merged, see Tree Hygiene #5. Can we possibly add some kind of driver test that checks that the link launching works? If not a unit test would also be OK.
It should also update the pubspec.yaml and CHANGELOG.md version so that we can publish the fixed version.
|
@mklim There is already a test written for universal links here - https://github.com/flutter/plugins/blob/master/packages/url_launcher/url_launcher/test/url_launcher_test.dart#L105 and it passes. I don't think we need to change this test anything drastically. Should I add anything more?:thought_balloon: |
|
@jddeep please change the macOS example app as well. I know it's another package, and a duplicated example is not optimal, but until we have a better solution we should try to keep them as equal as possible. |
|
@franciscojma86 Done. Thanks. |
mklim
left a comment
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.
nit: The url_launcher_macos package also needs its pubspec.yaml and CHANGELOG updated too now.
For testing: that unit test only covers the plugin API itself, and not the behavior of the example app. If it had been covering this behavior it would have been broken before this PR fixed it. What we'd want for this patch is a little different, ideally we'd want a test that checks that the example app itself is launching the URLs correctly. Maybe something with a mock platform channel instance like that unit test you linked, but a widget test?
Thanks again for contributing.
|
@mklim So here is a widget test I was writing, but there were some doubts. How would we test whether the url is launchable or not once the button is tapped and is directed to a browser of OS? Also but the line |
|
@jddeep that's pretty close. So the missing piece there is that when you click on the button, it's still trying to actually call through the plugin to the platform code to launch the link. That error message you're seeing is the Flutter app expecting the plugin to return a bool and then panicking when it doesn't. For our purposes here a widget test is basically the same as a unit test. There is no underlying platform, so the plugin can't "really" work and really launch any links. So the thing to do here is to test that the button works is to mock out the Dart part of the plugin that would normally send the calls down to Java/ObjC to actually launch the link, and then verify that the mock Dart plugin interface received a call from the example app asking it to launch the link. I uploaded a working test doing this to mklim/plugins@6a9d4387cb200943043a67e7fce46d862e364650, feel free to patch it into your PR. |
|
@mklim Thanks for the help. I see now. I have updated my test as well. Do we want tests for each button here or testing for one button to check if the url is launchable is cool? Do let me know or we can merge this up. |
mklim
left a comment
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.
LGTM! Thanks again for contributing and working on this. Just the test for the change in this PR is enough here.
…d Added Tests (flutter#2552) updated _launchUniversalLinkIos to launch the url passed. It could also be done that we remove the url argument passed and open the hardcoded url. Either of the case needs to be done.
…d Added Tests (flutter#2552) updated _launchUniversalLinkIos to launch the url passed. It could also be done that we remove the url argument passed and open the hardcoded url. Either of the case needs to be done.
Description
updated _launchUniversalLinkIos to launch the url passed. It could also be done that we remove the url argument passed and open the hardcoded url. Either of the case needs to be done.
Related Issues
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.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?