-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[quick_actions]make QuickActions testable #1067
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
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.
Thanks for your contribution!
Please update your pull request following the contribution checklist: https://github.com/flutter/plugins/blob/master/.github/PULL_REQUEST_TEMPLATE.md
In particular, it would be helpful if you could contribute a simple unit test for this plugin and update the pubspec/CHANGELOG, and make sure the bots are happy.
Thanks for your answer, since it's my first pr I ask myself what should the unit test consist of ? |
I made the requested changes! |
Hey, can anybody please look into this pr? Thanks! |
@The-Redhat Can you merge master into this and fix the conflicts? Then we can try to get a code review for this and get things moving. |
merged master |
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 contributing. Did a quick pass. It looks good overall, left some comments.
I made the requested changes |
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! cc @collinjackson Since you initially looked at this, I'd like to also get a 'go' from you before merging this in.
We discussed as a team and we'd like unit tests to be written using Here's an example unit test: Could you please update your unit test to use this style instead? |
@collinjackson I updated the tests |
@collinjackson Can I do something to get this pr merged ? |
Description
I used the same approach like in FirebaseMessaging and added a
QuickActions.private
constructor used for testing with a mock MethodChannel.Related Issues
Fixes #25887
Checklist
///
).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?