-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[firebase_messaging] add integration tests #1816
[firebase_messaging] add integration tests #1816
Conversation
cyanglaz
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.
LTGM, with some nits.
| @@ -1,3 +1,7 @@ | |||
| ## 4.0.0+4 | |||
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.
Probably should be 4.0.1 since post 1.0.0.
| ## 4.0.0+4 | |
| ## 4.0.1 |
| author: Flutter Team <[email protected]> | ||
| homepage: https://github.com/flutter/plugins/tree/master/packages/firebase_messaging | ||
| version: 4.0.0+3 | ||
| version: 4.0.0+4 |
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.
nits:
| version: 4.0.0+4 | |
| version: 4.0.1 |
| }); | ||
|
|
||
| test('subscribeToTopic', () async { | ||
| firebaseMessaging.subscribeToTopic('foo'); |
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.
Some idea, doesn't need to be in this PR. Maybe we can make subscribeToTopic and unsubscribeFromTopic to return a Future to indicate the invocation to the platform is happened correctly. That'd make the integration test easier.
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 agree, and will update in a subsequent PR
* Initial firebase_messaging integration tests
* Initial firebase_messaging integration tests
Includes a test for #1756 (crash on Android calling deleteInstanceId)