Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prevent Messaging and IID singleton usage during tests. #2250

Merged
merged 5 commits into from
Jan 28, 2019
Merged

Prevent Messaging and IID singleton usage during tests. #2250

merged 5 commits into from
Jan 28, 2019

Conversation

ryanwilson
Copy link
Member

No description provided.

charlotteliang
charlotteliang approved these changes Jan 11, 2019
View reviewed changes
Copy link
Contributor

@charlotteliang charlotteliang left a comment

Choose a reason for hiding this comment

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

This means we can get rid of instanceIDForTest, right?

paulb777
paulb777 approved these changes Jan 11, 2019
View reviewed changes
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Not until making similar changes in RemoteConfig and Analytics

@paulb777
Copy link
Member

cc: @htcgh

charlotteliang
charlotteliang reviewed Jan 11, 2019
View reviewed changes
Example/Messaging/Tests/FIRMessagingTest.m Outdated

/// Starts fetching and configuration of InstanceID. This is necessary after the `initPrivately`
/// call.
- (void)start;
Copy link
Contributor

Choose a reason for hiding this comment

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

I will see if can put start inside initPrivately so most team can just call initPrivately only.

@charlotteliang
Copy link
Contributor

Yeah will do for both config and analytics

@ryanwilson
Copy link
Member Author

Update: I plan on getting back to this this week, removing the messagingForTests ended up being a larger change than expected.

@ryanwilson
Copy link
Member Author

Once Travis is green again this should be ready for re-review.

paulb777
paulb777 approved these changes Jan 23, 2019
View reviewed changes
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

LGTM

@ryanwilson
Copy link
Member Author

@chliangGoogle just wanted to confirm, are you good with these changes going in now?

@charlotteliang
Copy link
Contributor

Yes! LGTM

@ryanwilson ryanwilson merged commit 5982981 into master Jan 28, 2019
@ryanwilson ryanwilson deleted the rw-messaging-iid-tests branch January 28, 2019 23:05
@paulb777 paulb777 added this to the M43 milestone Feb 20, 2019
@xiao99xiao xiao99xiao mentioned this pull request Feb 27, 2019
Closed
@firebase firebase locked and limited conversation to collaborators Oct 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@charlotteliang charlotteliang charlotteliang approved these changes

@paulb777 paulb777 paulb777 approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
5.17.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ryanwilson @paulb777 @charlotteliang @googlebot