getId() implementation with instrumented tests. #703

ankitaj224 · 2019-08-13T18:16:43Z

No description provided.

andirayo

Looks good! Major comments are:
a) Please avoid using "Fiid", the identifier is called "Fid".
b) Most methods look good, but there is one method that is too big and hard too read. My general suggestion is that (besides input-parameter checking) every method should try to be limited to 1 if-clause or 1 try-catch-block or 1 loop.
c)

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

andirayo · 2019-08-13T23:10:20Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

    this.firebaseApp = firebaseApp;
+    this.serviceClient = serviceClient;
+    this.executor = Executors.newFixedThreadPool(6);


Why is this not a constant?
Why 6?

Changed it. Its a random number, based on 3 APIs I chose double the number of available threads.

andirayo · 2019-08-13T23:14:11Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+   *
+   * <pre>
+   *     The workflow is:
+   *         check if cache empty or cache status is REGISTER_ERROR


This description is somewhat confusing to me: What happens if the status is REGISTER_ERROR or not?

Also what happens if status is UNREGISTERED?

Updated the description. Let me know, if you want me to change further.

andirayo · 2019-08-13T23:30:18Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+        long creationTime = Utils.getCurrentTimeInSeconds();
+
+        InstallationResponse installationResponse =
+            serviceClient.createFirebaseInstallation(


I think, I mentioned this before (I know it's pretty nitty):
Can we change the order of the parameters maybe?
like first FID, then API-Key, then all the metadata?
Or first the API-Key, then FID, then all the metadata?

Changed it in the parent PR. 34848c6

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

- Detailed JavaDocs - Renaming FiidCache as PersistedFid

… into create_fis

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

andirayo · 2019-08-14T21:52:32Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

   */
-  private void registerAndSaveFID(String fid) throws FirebaseInstallationsException {
+  private void registerAndSaveFid(String fid) throws FirebaseInstallationsException {


This method has to be asynchronous to not block callers to #getId.
What I'm saying is that if someone calls #getId, the method should immediately return an FID independent of if that FID has to be registered with FIS or not.

Done. Please take another look.

andirayo · 2019-08-14T21:53:09Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+      return createFidAndPersistFidEntry();
+    }
+
+    if (persistedFidExistsAndRegistered(persistedFidEntryValue)) {


Can we please move this case to the top, because this is the standard case.

Also, something we added to the Web SDK:
We should check here if the current FIS Auth Token is still valid.
If not, this request should trigger an asynchronous request to FIS to update the local FIS Auth Token.

As discussed offline, will add this when auth token method is implemented.

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

...tallations/src/main/java/com/google/firebase/installations/local/PersistedFidEntryValue.java

… into create_fis

- Make registerAnsSaveFid non blocking call in getId() - PersistedFidEntry builder with default values

andirayo · 2019-08-17T03:27:05Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-      return persistedFidEntryValue.getFirebaseInstallationId();
+    if (persistedFidExistsAndUnregistered(persistedFidEntry)) {
+      Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry.getFirebaseInstallationId()));
+      return persistedFidEntry.getFirebaseInstallationId();
    }

    return null;


How can this method ever return NULL?

If delete() is called and there is no network availability, we mark registration status as deleted. In that case, it returns NULL.

Is that not right?

@andirayo Can you please check this? I think the issue is, we have introduced a state DELETED as per fis-android-sdk-design doc. But from your comments on my doc, seems like you don't want the DELETED state.

We can change the above function as :

@workerthread

private String getFirebaseInstallationId() throws FirebaseInstallationsException {

PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); if (persistedFidExistsAndRegistered(persistedFidEntry)) { return persistedFidEntry.getFirebaseInstallationId(); } else if (persistedFidExistsAndUnregistered(persistedFidEntry)) { Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry.getFirebaseInstallationId())); return persistedFidEntry.getFirebaseInstallationId(); } return createFidAndPersistFidEntry(); }

Wdyt?

Except for the "else if" in there (an else is not necessary after you "return" in the if-statement, I like it better yes.

How about
// not registered with FIS yet
if (fidExistsAndUnregistered(persistedFidEntry)) {
Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry.getFirebaseInstallationId()));
}
// return FID from in local storage
if (fidExists(persistedFidEntry)) {
return persistedFidEntry.getFirebaseInstallationId();
}

return createFidAndPersistFidEntry();

https://help.github.com/en/articles/creating-and-highlighting-code-blocks

what do you all think about something like this? I haven't checked that it'll compile, but it seems to clearly express what we're trying to do:

return the existing fid if it's there

create one if it didn't exist

trigger a background registration if it wasn't previously registered

public Task<FID> getFid() { return getExistingFid() .continueWith(orElse(this::createAndPersistNewFid)) .onSuccessTask(this::registerFidIfNecessary); } public static <F, T> Continuation<F, T> orElse(Supplier<T> supplier) { return t -> { if (t.isSuccessful()) { return t.getResult(); } return supplier.get(); }; } private FID createAndPersistNewFid() { FID fid = Utils.createRandomFid(); persist(fid); return fid; } private void registerFidIfNecessary(FID fid) { // do network stuff }

@ciarand I tried incorporating the changes you mentioned below:

@RequiresApi(api = Build.VERSION_CODES.N) private Task<String> getFid() { return Tasks.call(executor, this::getExistingFid) .continueWith(orElse(new NewFidSupplier())) .onSuccessTask(this::registerFidIfNecessary); } private PersistedFidEntry getExistingFid() throws FirebaseInstallationsException { PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); if (persistedFidMissingOrInErrorState(persistedFidEntry)) { throw new FirebaseInstallationsException( "Failed to get existing fid.", FirebaseInstallationsException.Status.CLIENT_ERROR); } return persistedFidEntry; } @RequiresApi(api = Build.VERSION_CODES.N) @NonNull public static <F, T> Continuation<F, T> orElse(@NonNull Supplier<T> supplier) { return t -> { if (t.isSuccessful()) { return (T) t.getResult(); } return supplier.get(); }; } @RequiresApi(api = Build.VERSION_CODES.N) class NewFidSupplier implements Supplier<PersistedFidEntry> { @Override public PersistedFidEntry get() { return createAndPersistNewFid(); } } private PersistedFidEntry createAndPersistNewFid() { String fid = Utils.createRandomFid(); try { persistFid(fid); PersistedFidEntry persistedFidEntry = persistedFid.readPersistedFidEntryValue(); return persistedFidEntry; } catch (FirebaseInstallationsException e) { // Exception handling } return PersistedFidEntry.builder().build(); } private void persistFid(String fid) throws FirebaseInstallationsException { boolean firstUpdateCacheResult = persistedFid.insertOrUpdatePersistedFidEntry( PersistedFidEntry.builder() .setFirebaseInstallationId(fid) .setRegistrationStatus(PersistedFid.RegistrationStatus.UNREGISTERED) .build()); if (!firstUpdateCacheResult) { throw new FirebaseInstallationsException( "Failed to update client side cache.", FirebaseInstallationsException.Status.CLIENT_ERROR); } } private Task<String> registerFidIfNecessary(PersistedFidEntry persistedFidEntry) { if (persistedFidEntry.getRegistrationStatus() != PersistedFid.RegistrationStatus.REGISTERED) { Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry)); } return Tasks.forResult(persistedFidEntry.getFirebaseInstallationId()); } /** Registers the created Fid with FIS Servers and update the shared prefs. */ private Void registerAndSaveFid(PersistedFidEntry persistedFidEntry) throws FirebaseInstallationsException { // network call }

I have 2 concerns:

Supplier get call requires API level 24, Is this right way of handling it?

Exception handling by the Supplier while persisting the newly created fid

Yeah we can't use the standard java.util.functional.Supplier interface. We can just define one in the parent class:

interface Supplier<T> { T get() throws Exception; }

When you say "Exception handling by the Supplier while persisting the newly created fid" do you mean you're worried that you can't propagate exceptions or you're worried that they will be propagated?

Updated. Thanks for the comments.

...allations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java

ciarand · 2019-08-20T21:26:07Z

...allations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java

+  }
+
+  @Test
+  public void testUpdateAndReadCacheEntry() throws Exception {


can you add some comments explaining what this is testing?

Done. Thanks. PTAL.

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

ciarand · 2019-08-20T21:27:58Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+      return persistedFidEntry.getFirebaseInstallationId();
+    }
+
+    return null;


mark it @Nullable if it's nullable

firebase-installations/src/main/java/com/google/firebase/installations/Utils.java

ciarand · 2019-08-20T21:32:19Z

firebase-installations/src/main/java/com/google/firebase/installations/Utils.java

+    return System.currentTimeMillis() / 1000;
+  }
+
+  public static boolean isNetworkAvailable(Context context) {


as above, I'd inject a "NetworkChecker" or similar so that you're not forced to muck around with a real ConnectivityManager in tests

Skipped the connectivity check here. Fred was of the opinion - the developer should check and handle the cases of network connectivity instead of us doing it.

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

ciarand · 2019-08-20T21:38:17Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+
+      InstallationResponse installationResponse =
+          serviceClient.createFirebaseInstallation(
+              firebaseApp.getOptions().getApiKey(),


can you add /* paramName= */ style comments here? otherwise it looks really easy to transpose one of the string params into another

Done. Thanks.

I'm not seeing them in the diff, am I missing something?

Done. f8e75fd

ciarand · 2019-08-20T21:42:29Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

    this.firebaseApp = firebaseApp;
+    this.serviceClient = serviceClient;
+    this.executor = Executors.newFixedThreadPool(THREAD_COUNT);


a fixed thread pool never GC's idle threads ("The threads in the pool will exist until it is explicitly shutdown.")

are you sure we need 6 threads? that seems like a huuuuge amount

can we not inject the executor in this constructor? it's usually an antipattern for the instantiating class to also instantiate its own thread pool

I was referring to FLoC sdk while implementing the executor. I am not sure whats the best approach to decide the thread pool count.

If I just inject the executor, the user of the sdk has to instantiate the executor class. This might be a wrong question, but, how will they know the right number of threads required? Or, how does this improve the performance?

do you actually need multiple threads? a cached executor service w/ a cap of one thread and ~30s idle time should be sufficient, no? You're never actually running more than one thing at a time right?

If I just inject the executor, the user of the sdk has to instantiate the executor class.

I'm suggesting that you move the instantiation of the executor service into the delegating constructor, which ensures there's at least one constructor where you can inject a test-controllable executor.

I will be running more than one task at a time( Line 96 and Line 170 ). Task for getId and another task for registering FID.

Do you recommend using newCachedThreadPool() or have a custom executor

new ThreadPoolExecutor(0, 2,
30L, TimeUnit.SECONDS,
new SynchronousQueue());

You're loading two separate jobs onto the same executor, but they don't run in parallel right? The getId call runs first, then the registration call runs asynchronously after the getId call finishes.

A custom executor is probably fine. new ThreadPoolExecutor(0, 1, 30L, TimeUnit.SECONDS, new SynchronousQueue<>());

Updated. PTAL.

andirayo

I think, there is a question around the stati of requests to FIS API.
Aren't me missing a status for "PENDING" to avoid race conditions?
Also, what is the difference between "UNREGISTERED" and "REGISTER_ERROR"?

andirayo · 2019-08-21T21:31:31Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

    this.firebaseApp = firebaseApp;
+    this.serviceClient = serviceClient;
+    this.executor = Executors.newFixedThreadPool(THREAD_COUNT);


andirayo · 2019-08-21T21:34:53Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-      return persistedFidEntryValue.getFirebaseInstallationId();
+    if (persistedFidExistsAndUnregistered(persistedFidEntry)) {
+      Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry.getFirebaseInstallationId()));
+      return persistedFidEntry.getFirebaseInstallationId();
    }

    return null;


Except for the "else if" in there (an else is not necessary after you "return" in the if-statement, I like it better yes.

How about
// not registered with FIS yet
if (fidExistsAndUnregistered(persistedFidEntry)) {
Tasks.call(executor, () -> registerAndSaveFid(persistedFidEntry.getFirebaseInstallationId()));
}
// return FID from in local storage
if (fidExists(persistedFidEntry)) {
return persistedFidEntry.getFirebaseInstallationId();
}

return createFidAndPersistFidEntry();

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

andirayo · 2019-08-21T21:43:37Z

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

    UNREGISTERED,
-    // Cache entry is in error state when syncing with Firebase backend
+    // Persisted Fid entry is in error state when syncing with Firebase backend
    REGISTER_ERROR,


what's the difference between UNREGISTERED AND REGISTER_ERROR?

andirayo · 2019-08-21T21:47:27Z

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

    DELETED
  }

-  private static final String SHARED_PREFS_NAME = "FiidCache";
+  private static final String SHARED_PREFS_NAME = "PersistedFid";

  private static final String FIREBASE_INSTALLATION_ID_KEY = "Fid";
  private static final String AUTH_TOKEN_KEY = "AuthToken";
  private static final String REFRESH_TOKEN_KEY = "RefreshToken";
  private static final String TOKEN_CREATION_TIME_IN_SECONDS_KEY = "TokenCreationEpochInSecs";


This is usually called a "Timestamp".
Any reason to prefer "Creation Time in Seconds" over "Timestamp"?

This is the creation timestamp when registering the FID on FIS Servers and not any value returned by the server.
This is used to calculate if token is valid or not.

andirayo · 2019-08-21T21:47:55Z

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

    String authToken = prefs.getString(getSharedPreferencesKey(AUTH_TOKEN_KEY), null);
    String refreshToken = prefs.getString(getSharedPreferencesKey(REFRESH_TOKEN_KEY), null);
    long tokenCreationTime =
        prefs.getLong(getSharedPreferencesKey(TOKEN_CREATION_TIME_IN_SECONDS_KEY), 0);
    long expiresIn = prefs.getLong(getSharedPreferencesKey(EXPIRES_IN_SECONDS_KEY), 0);

-    if (iid == null || status == -1) {
+    if (fid == null || status == -1) {


Why is this not a constant?
What does -1 mean?

we pass -1 as a default in the call to prefs.getInt

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

andirayo · 2019-08-21T21:51:53Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+import com.google.firebase.FirebaseApp;
+import com.google.firebase.FirebaseOptions;
+import com.google.firebase.installations.local.PersistedFid;
+import com.google.firebase.installations.local.PersistedFidEntry;


What is the difference between PersistedFid and PersistedFidEntry?

andirayo · 2019-08-21T21:53:06Z

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

-public class FiidCache {
-  // Status of each cache entry
-  // NOTE: never change the ordinal of the enum values because the enum values are stored in cache
+public class PersistedFid {


How about calling this LocalStorage or FisStorage or FirebaseInstallationsStorage?

andirayo · 2019-08-21T21:57:14Z

...e-installations/src/main/java/com/google/firebase/installations/local/PersistedFidEntry.java

+ * Firebase Installation attributes and the persisted status of this entry.
+ */
+@AutoValue
+public abstract class PersistedFidEntry {


Please feel free to ignore this comment.
Maybe, it's easier to discuss in person and just submit this PR with this name.
I'm just wondering:
Why can't we just call this "FirebaseInstallation"?
(or "PersistedFirebaseInstallation" in case you want to make it obvious that it's received from local storage)
Let's discuss in person after this is submitted.

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

...allations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java

… into create_fis

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

...allations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java

ciarand · 2019-08-29T18:00:56Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+        .isEqualTo(TEST_FID_1);
+    assertWithMessage("Registration status doesn't match")
+        .that(entryValue.getRegistrationStatus())
+        .isEqualTo(PersistedFid.RegistrationStatus.PENDING);


what prevents the FID from being registered at this point in the test code?

Honestly, nothing. I thought about it. But the test runs have been consistent with registration taking some time. Is there a better way to test it or do i skip the status check altogether.

@ciarand Your thoughts.

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

ciarand · 2019-08-29T18:07:46Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+
+      InstallationResponse installationResponse =
+          serviceClient.createFirebaseInstallation(
+              firebaseApp.getOptions().getApiKey(),


ciarand · 2019-08-29T18:10:32Z

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java

@@ -78,7 +89,7 @@ public synchronized PersistedFidEntry readPersistedFidEntryValue() {

    return PersistedFidEntry.builder()
        .setFirebaseInstallationId(fid)
-        .setPersistedStatus(PersistedStatus.values()[status])
+        .setRegistrationStatus(RegistrationStatus.values()[status])


No, technically you're just verifying that there is a number in shared prefs with that key. I'm saying we should check that the number is >= 0 and < RegistrationStatus.values().length as well

ankitaj224 · 2019-08-29T18:18:23Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

@@ -126,7 +130,7 @@ public void testGetId_PersistedFidOk_BackendOk() throws Exception {
        .isEqualTo(PersistedFid.RegistrationStatus.PENDING);

    // Waiting for Task that registers FID on the FIS Servers
-    Thread.sleep(500);
+    executor.awaitTermination(500, TimeUnit.MILLISECONDS);


@ciarand I was able to eliminate sleep entirely. Instead awaiting for the executor to complete. I am not sure if you saw these changes.

ciarand · 2019-08-29T18:22:44Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

@@ -126,7 +130,7 @@ public void testGetId_PersistedFidOk_BackendOk() throws Exception {
        .isEqualTo(PersistedFid.RegistrationStatus.PENDING);

    // Waiting for Task that registers FID on the FIS Servers
-    Thread.sleep(500);
+    executor.awaitTermination(500, TimeUnit.MILLISECONDS);


ankitaj224 · 2019-08-29T18:26:13Z

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

+  /**
+   * Creates a FID on the FIS Servers by calling FirebaseInstallations API create method.
+   *
+   * @param apiKey API Key that has access to FIS APIs


@ciarand Added the param javadocs. Is that not sufficient.

application in AndroidManifest.xml

...allations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java

… into create_fis

ankitaj224 · 2019-08-30T18:30:39Z

/retest

google-oss-bot · 2019-08-30T18:37:06Z

@ankitaj224: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
api-information	`16544a6`	link	`/test api-information`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

* Adding Firebase Installations module * Readding .idea files that were deleted in previous commit * Revert "Adding Firebase Installations module" This reverts commit 2ec4aef. * Revert "Readding .idea files that were deleted in previous commit" This reverts commit 7b4ebcf. * Adding Firebase Installations module. * Readding .idea files that were deleted in previous commit * Revert "Readding .idea files that were deleted in previous commit" This reverts commit 7b4ebcf. * Revert "Adding Firebase Installations module" with hidden files This reverts commit 2ec4aef. * Addressing review comments. * Http client to call FIS backend service. * Http client to call FIS backend service. * Http client to call FIS backend service. * Adding Firebase Installations module * Adding Firebase Installations module. * Readding .idea files that were deleted in previous commit * Readding .idea files that were deleted in previous commit * Revert "Adding Firebase Installations module" This reverts commit 2ec4aef. * Revert "Readding .idea files that were deleted in previous commit" This reverts commit 7b4ebcf. * Revert "Readding .idea files that were deleted in previous commit" This reverts commit 7b4ebcf. * Revert "Adding Firebase Installations module" with hidden files This reverts commit 2ec4aef. * Addressing review comments. * Http client to call FIS backend service. * Http client to call FIS backend service. * Initial Code structure for FIS Android SDK (#648) * Adding an interface library for Firebase Installations SDK * Adding Firebase Installations module * Adding Firebase Installations module. * Readding .idea files that were deleted in previous commit * Revert "Adding Firebase Installations module" This reverts commit 2ec4aef. * Revert "Readding .idea files that were deleted in previous commit" This reverts commit 7b4ebcf. * Add firebase installations project path * Adding Firebase Installations module. * Readding .idea files that were deleted in previous commit * Revert "Adding Firebase Installations module" This reverts commit 2ec4aef. * Revert "Readding .idea files that were deleted in previous commit" This reverts commit 7b4ebcf. * Add firebase installations project path * Fixing formattinf issues. * Revert "Adding Firebase Installations module" with hidden files This reverts commit 2ec4aef. * Addressing review comments. * Making InstallationTokenResult an AutoValue class. * Http client to call FIS backend service. * Addresing comments and introducing new FirebaseInstallationService Exception. * Implementing getId method in FirebaseInstallation to call backend. * 1. Adding instrumentation tests. 2. Introducing serviceClient level and main class exceptions. * Addressing Di's comments. * Addressing Rayo's comments * Updating parameters order in ServiceClient. * Updating createRandomFid as per Rayo's comments. * getId() implementation with instrumented tests. (#703) * getId() implementation with instrumented tests. * Addressing rayo's comments. - Detailed JavaDocs - Renaming FiidCache as PersistedFid * Addressing rayo's comments. - Detailed JavaDocs - Renaming FiidCache as PersistedFid * Addresing comments to resoleve the following: - Make registerAnsSaveFid non blocking call in getId() - PersistedFidEntry builder with default values * Addressing Ciaran and Rayo's comments. * Addressing Ciaran's comments * Addressing Ciaran's comments * Adding param comments and checking if registration status is valid. * Correcting lint warning: uses-permission should be declared before application in AndroidManifest.xml * Adding custom assertThat with more readable assertions * Correcting instrumented tests to be reliable.

getId() implementation with instrumented tests.

30cf6ed

ankitaj224 requested review from andirayo and diwu-arete August 13, 2019 18:16

googlebot added the cla: yes Override cla label Aug 13, 2019

google-oss-bot added the size/L label Aug 13, 2019

andirayo approved these changes Aug 13, 2019

View reviewed changes

Addressing rayo's comments.

d034bbb

- Detailed JavaDocs - Renaming FiidCache as PersistedFid

google-oss-bot added size/XL and removed size/L labels Aug 14, 2019

ankitaj224 added 3 commits August 14, 2019 13:52

Addressing rayo's comments.

f8b468c

- Detailed JavaDocs - Renaming FiidCache as PersistedFid

Merge branch 'ankita_fis' of github.com:firebase/firebase-android-sdk…

8db7967

… into create_fis

Merge branch 'create_fis' of github.com:firebase/firebase-android-sdk…

b507a3c

… into create_fis

andirayo approved these changes Aug 14, 2019

View reviewed changes

ankitaj224 added 2 commits August 14, 2019 15:44

Merge branch 'ankita_fis' of github.com:firebase/firebase-android-sdk…

f5a7c05

… into create_fis

Addresing comments to resoleve the following:

90cdbaa

- Make registerAnsSaveFid non blocking call in getId() - PersistedFidEntry builder with default values

andirayo approved these changes Aug 17, 2019

View reviewed changes

ciarand reviewed Aug 20, 2019

View reviewed changes

...allations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java Show resolved Hide resolved

ciarand reviewed Aug 20, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Outdated Show resolved Hide resolved

ciarand reviewed Aug 20, 2019

View reviewed changes

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java Outdated Show resolved Hide resolved

ciarand reviewed Aug 20, 2019

View reviewed changes

firebase-installations/src/main/java/com/google/firebase/installations/Utils.java Outdated Show resolved Hide resolved

ciarand reviewed Aug 20, 2019

View reviewed changes

firebase-installations/src/main/java/com/google/firebase/installations/local/PersistedFid.java Outdated Show resolved Hide resolved

ciarand reviewed Aug 20, 2019

View reviewed changes

andirayo reviewed Aug 21, 2019

View reviewed changes

Addressing Ciaran and Rayo's comments.

0dfa987

ankitaj224 self-assigned this Aug 26, 2019

ciarand reviewed Aug 26, 2019

View reviewed changes

ankitaj224 added 4 commits August 27, 2019 15:38

Addressing Ciaran's comments

a71a1f3

Addressing Ciaran's comments

0a92e2d

Merge branch 'create_fis' of github.com:firebase/firebase-android-sdk…

4cc0cee

… into create_fis

Merge branch 'ankita_fis' of github.com:firebase/firebase-android-sdk…

f16db3f

… into create_fis

ciarand reviewed Aug 29, 2019

View reviewed changes

ankitaj224 commented Aug 29, 2019

View reviewed changes

ciarand approved these changes Aug 29, 2019

View reviewed changes

ankitaj224 commented Aug 29, 2019

View reviewed changes

ankitaj224 added 3 commits August 30, 2019 09:57

Adding param comments and checking if registration status is valid.

f8e75fd

Correcting lint warning: uses-permission should be declared before

227bf3b

application in AndroidManifest.xml

Adding custom assertThat with more readable assertions

4be3847

ciarand approved these changes Aug 30, 2019

View reviewed changes

...allations/src/androidTest/java/com/google/firebase/installations/local/PersistedFidTest.java Show resolved Hide resolved

ankitaj224 added 2 commits August 30, 2019 11:08

Correcting instrumented tests to be reliable.

68acde5

Merge branch 'ankita_fis' of github.com:firebase/firebase-android-sdk…

16544a6

… into create_fis

ankitaj224 merged commit 429581e into ankita_fis Aug 30, 2019

ankitaj224 deleted the create_fis branch August 30, 2019 21:48

ankitaj224 mentioned this pull request Sep 17, 2019

Refresh Auth token if expired during getId call. #809

Closed

firebase locked and limited conversation to collaborators Oct 8, 2019

getId() implementation with instrumented tests. #703

getId() implementation with instrumented tests. #703

Uh oh!

Conversation

ankitaj224 commented Aug 13, 2019

Uh oh!

andirayo left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

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!

ankitaj224 Aug 21, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankitaj224 Aug 21, 2019 •

edited

Loading