Skip to content

Conversation

@charlotteliang
Copy link
Contributor

@charlotteliang charlotteliang commented Jan 12, 2021

Moving a few unit tests from InstanceID to Messaging after the refactor.

  • APNSTokenInfoTest
  • TokenManagerTest
  • AuthKeychainTest

Simplify some init methods as there's no need to inject some classes as they can be initialized inside.
Also since we updated clang-format so there are a few nonmessaging methods are formatted in the branch.

@google-cla google-cla bot added the cla: yes label Jan 12, 2021
@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@charlotteliang charlotteliang changed the title [draft] Move token unit tests from IID to Messaging [Remove IID] Move some token unit tests from IID to Messaging Feb 11, 2021
Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

LGTM

#import "FirebaseMessaging/Sources/FIRMessagingUtilities.h"
#import "FirebaseMessaging/Sources/NSError+FIRMessaging.h"
#import "FirebaseMessaging/Sources/Token/FIRMessagingAuthKeyChain.h"
#import "FirebaseMessaging/Sources/Token/FIRMessagingAuthKeychain.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if using GULKeychainUtils instead of FIRMessagingAuthKeychain may save some extra code and tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I will take a look at.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if you need any help with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah maybe we should work on separate PR. Feel free to checkin in chen/fm-master while I keep adding more unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure

@charlotteliang charlotteliang merged commit e1fbde9 into chen/fm-master Feb 17, 2021
@charlotteliang charlotteliang deleted the fm-refactor-unit-test branch February 17, 2021 03:20
@firebase firebase locked and limited conversation to collaborators Mar 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants