FIS Public API #3135

maksymmalyhin · 2019-06-04T16:28:28Z

public API
FIS instance config
basic tests

…into mm/fis-keychain

FirebaseInstallations/Source/Library/FIRInstallations.m

paulb777 · 2019-06-04T16:57:23Z

FirebaseInstallations/Source/Tests/FIRInstallationsTests.m

+
+- (FIRApp *)createAndConfigureAppWithName:(NSString *)name {
+  FIROptions *options = [[FIROptions alloc] initInternalWithOptionsDictionary:@{
+    @"GOOGLE_APP_ID" : @"1:1085102361755:ios:f790a919483d5bdf",


Is this real?

Good question. It might be because I took it from another Firebase test. I'll better replace it with an obviously fake value

paulb777 · 2019-06-04T16:57:48Z

FirebaseInstallations/Source/Library/FIRInstallations.m

+}
+
+- (void)installationIDWithCompletion:(FIRInstallationsIDHandler)completion {
+  completion(@"123", nil);


Just a placeholder so far. Real implementation will be added in next PRs

Adding a TODO would be good in that case to keep track and make it more visible for reviewers :)

FirebaseInstallations/Source/Library/FIRInstallations.m

FirebaseInstallations/Source/Library/Public/FIRAuthTokenResult.h

FirebaseInstallations/Source/Library/FIRInstallations.m

ryanwilson · 2019-06-04T16:47:52Z

FirebaseInstallations/Source/Library/FIRInstallations.m

+  FIRComponentCreationBlock creationBlock =
+      ^id _Nullable(FIRComponentContainer *container, BOOL *isCacheable) {
+    *isCacheable = YES;
+    FIRInstallations *installations = [[FIRInstallations alloc] initWithApp:container.app];


See the other comment I left below - we could pull out the information needed here to inject into the FIRInstallations initializer.

@ryanwilson I would rather keep both initWithGoogleAppID:appName: and initWithApp: initializers, so the 2nd one uses the 1st. WDYT?

Is there a particular reason you want to keep the initWithApp: initializer? I'm not sure what benefit there is outside of not having to change the initializer here in the component creation if it ever changes.

I just would prefer to keep the logic (in this case extracting info from FIRApp) outside of the block used by a very specific method. If we can initialize an instance with a FIRApp, then having a convenience initializer sounds logical to me.
Not too strong opinion here, I am fine with initWithGoogleAppID:appName: only if there are downsides of having both.

@ryanwilson I pushed the version with 2 initializers just because I had it before your response. Let me know if you have a strong opinion to remove initWithApp:

FirebaseInstallations/Source/Library/FIRInstallations.m

FirebaseInstallations/Source/Library/Public/FIRInstallations.h

ryanwilson · 2019-06-04T17:08:22Z

FirebaseInstallations/Source/Tests/FIRInstallationsTests.m

+  FIROptions *options = [[FIROptions alloc] initInternalWithOptionsDictionary:@{
+    @"GOOGLE_APP_ID" : @"1:1085102361755:ios:f790a919483d5bdf",
+  }];
+  [FIRApp configureWithName:name options:options];


configureWithName:options: has some internal state associated with it that may affect tests running concurrently that can lead to flakes. Using the [FIRInstallations installations] singleton can do the same. I mentioned a way to get around this in the other file about the initializer, then no FIRApp is necessary for most

Good point. I have couple questions here:

Will I need to configure a FIRApp to test singleton?

Do we run tests concurrently?

Yes, that is required. There are ways to do it without explicitly calling configure but if it's a single test case that's probably okay and reduces the surface for flakes due to global state. I believe we have some other code to have the container properly initialized without global state but I'll have to find it if needed.

Yes. Tests in other files run concurrently, but test cases in the same class run synchronously.

I kept the logic for the singleton tests but other tests use initWithGoogleAppID:appName:

charlotteliang

LGTM, we should have a test app calling your API for end to end testing, which could potentially be used for integration testing as well.
I have something similar setup inside google3.

maksymmalyhin · 2019-06-04T20:13:29Z

@chliangGoogle Good point! I am going to add it later on.

FirebaseInstallations/Source/Library/FIRInstallations.m

FirebaseInstallations/Source/Library/Public/FIRInstallations.h

ryanwilson · 2019-06-05T16:27:50Z

FirebaseInstallations/Source/Library/FIRInstallations.m

+}
+
+- (void)installationIDWithCompletion:(FIRInstallationsIDHandler)completion {
+  completion(@"123", nil);


Adding a TODO would be good in that case to keep track and make it more visible for reviewers :)

ryanwilson · 2019-06-05T16:30:44Z

FirebaseInstallations/Source/Library/Public/FIRInstallations.h

+    NS_SWIFT_NAME(InstallationsTokenHandler);
+
+NS_SWIFT_NAME(Installations)
+@interface FIRInstallations : NSObject


We do also need a + (FIRInstallations *)installations NS_SWIFT_NAME(installations); for the default app instance.

@ryanwilson I'll add it in following PRs if you don't mind.

FirebaseInstallations/Source/Library/Public/FIRInstallationsAuthTokenResult.h

FirebaseInstallations/Source/Tests/FIRInstallationsTests.m

ryanwilson · 2019-06-05T16:39:52Z

FirebaseInstallations/Source/Tests/FIRInstallationsTests.m

+  FIRApp *app = [self createAndConfigureAppWithName:appName];
+  FIRInstallations *installations = [FIRInstallations installationsWithApp:app];
+
+  XCTAssertEqualObjects(installations.appID, app.options.googleAppID);


I believe this should fail right now based on the information in the setUp method and the helper FIRApp method below. Perhaps having a FIROptions creation helper would help.

@ryanwilson It's not failing now, though, I think it is worth to understand why :) Why do you think it should fail?

Whoops nevermind, you're right. I was thinking this was comparing against self.installations

maksymmalyhin added 30 commits May 31, 2019 15:41

FirebaseInstallations initial podspec

1bfc0de

Run FIS test on fis-master

8008f95

Run FIS test on mm/fis-master

2f34414

Run checks for mm/fis-master

f4ae296

Remove extra flags

fdf0ffb

Fix - run checks and test-fis for mm/fis-master

70eb50f

Trailing spaces

58de380

Copyright added to the example files to pass checks

fbf9e61

Promises dependency added

8403b5f

Merge branch 'mm/fis-master' of github.com:firebase/firebase-ios-sdk …

bd4b295

…into mm/fis-keychain

FIRSecureStorage introduced

8615968

FIRSecureStorage - object read-write

29297ac

Merge branch 'mm/fis-master'

3b0f829

FIRInstallations files added

5a80459

Formatting

f6a1efd

FIRInstallations.h - API definition

a9a412d

Run ./scripts/style.sh

0ca9a22

pod lib lint fixes.

4c0ee6e

Typo

7de45ba

Move source to subdirectory

f05ad80

pod lib lint - fix ARC for dependencies

098f904

Remove requires_arc = true - rely on the default value

74cdf97

Travis - temporary disable --use-libraries

2b9a623

Use __has_include(<FBLPromises/FBLPromises.h>)

378d1e2

Merge branch 'mm/fis-keychain' into mm/fis-api

9c825ff

Move files

8d54bef

Fixes

efbc6e2

FirebaseInstallationsVersion files

ee18638

Temporary disable testing for macOS

293bf10

FIRInstallationsVersion

bb96e08

maksymmalyhin commented Jun 4, 2019

View reviewed changes

FirebaseInstallations/Source/Library/FIRInstallations.m Show resolved Hide resolved

maksymmalyhin requested review from ryanwilson and paulb777 June 4, 2019 16:31

paulb777 approved these changes Jun 4, 2019

View reviewed changes

ryanwilson requested changes Jun 4, 2019

View reviewed changes

maksymmalyhin added 8 commits June 4, 2019 13:11

Use fake value of GOOGLE_APP_ID in the tests

87cc51d

Travis - rename project

2259ba0

Renaming FIRAuthTokenResult -> FIRInstallationsAuthTokenResult

86874bf

Use FIRInstantiationTimingAlwaysEager

cbf59b0

- [FIRInstallations initWithGoogleAppID:appName]

fb38729

Remove FIRInstallationsDeleteHandler

4a0d115

./scripts/style.sh

c6d5d3d

Fix copy-paste typo

8d5e361

maksymmalyhin requested review from ryanwilson and charlotteliang June 4, 2019 18:57

charlotteliang approved these changes Jun 4, 2019

View reviewed changes

charlotteliang requested changes Jun 4, 2019

View reviewed changes

FirebaseInstallations/Source/Library/FIRInstallations.m Show resolved Hide resolved

FirebaseInstallations/Source/Library/Public/FIRInstallations.h Show resolved Hide resolved

Merge branch 'mm/fis-master' into mm/fis-api

13a78f9

ryanwilson reviewed Jun 5, 2019

View reviewed changes

maksymmalyhin added 3 commits June 5, 2019 12:51

FIRInstallationsAuthTokenResult internal init

e60cc96

TODO

d2a5772

expirationTime -> expirationDate

377427a

ryanwilson approved these changes Jun 5, 2019

View reviewed changes

./scripts/style.sh

d81f51a

maksymmalyhin requested a review from charlotteliang June 5, 2019 17:42

charlotteliang approved these changes Jun 5, 2019

View reviewed changes

maksymmalyhin merged commit 422f5c4 into mm/fis-master Jun 5, 2019

maksymmalyhin deleted the mm/fis-api branch June 5, 2019 18:02

firebase locked and limited conversation to collaborators Oct 14, 2019

FIS Public API #3135

FIS Public API #3135

Uh oh!

Conversation

maksymmalyhin commented Jun 4, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maksymmalyhin Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charlotteliang left a comment

Choose a reason for hiding this comment

Uh oh!

maksymmalyhin commented Jun 4, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maksymmalyhin Jun 4, 2019 •

edited

Loading