Address Manny's feedback

Author: lfkellogg

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/AabUpdater.java

@@ -97,14 +97,11 @@ UpdateTaskImpl updateAab(@NonNull AppDistributionReleaseInternal newRelease) {
 
       // On a background thread, fetch the redirect URL and open it in the Play app
       runAsyncInTask(executor, () -> fetchDownloadRedirectUrl(newRelease.getDownloadUrl()))
-          .onSuccessTask(
-              executor,
-              combineWithResultOf(executor, () -> lifecycleNotifier.getForegroundActivity()))
+          .onSuccessTask(combineWithResultOf(() -> lifecycleNotifier.getForegroundActivity()))
           .addOnSuccessListener(
-              executor,
               urlAndActivity ->
                   openRedirectUrlInPlay(urlAndActivity.first(), urlAndActivity.second()))
-          .addOnFailureListener(executor, this::setUpdateTaskCompletionError);
+          .addOnFailureListener(this::setUpdateTaskCompletionError);
 
       return cachedUpdateTask;
     }
@@ -159,8 +156,8 @@ private void openRedirectUrlInPlay(String redirectUrl, Activity hostActivity) {
     updateIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
     LogWrapper.getInstance().v(TAG + "Redirecting to play");
 
-    // Launch the intent outside of the synchronized block because we don't want to risk the
-    // activity leaving the foreground while we wait for the lock.
+    // Launch the intent before the synchronized block to avoid failing to update in the rare
+    // scenario where the activity moves to the background while we're awaiting the lock.
     hostActivity.startActivity(updateIntent);
 
     synchronized (updateAabLock) {

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/TaskUtils.java

@@ -122,35 +122,18 @@ static <T1, T2> CombinedTaskResults<T1, T2> create(T1 first, T2 second) {
    *
    * <pre>{@code
    * runFirstAsyncTask()
-   *   .onSuccessTask(executor, combineWithResultOf(executor, () -> startSecondAsyncTask())
+   *   .onSuccessTask(combineWithResultOf(executor, () -> startSecondAsyncTask())
    *   .addOnSuccessListener(
-   *       executor,
    *       results ->
    *           doSomethingWithBothResults(results.result1(), results.result2()));
    * }</pre>
    *
-   * @param executor The {@link Executor} to use to call the listener that combines the results
    * @param secondTaskSource A {@link TaskSource} providing the next task to run
    * @param <T1> The result type of the first task
    * @param <T2> The result type of the second task
    * @return A {@link SuccessContinuation} that will return a new task with result type {@link
    *     CombinedTaskResults}, combining the results of both tasks
    */
-  static <T1, T2> SuccessContinuation<T1, CombinedTaskResults<T1, T2>> combineWithResultOf(
-      Executor executor, TaskSource<T2> secondTaskSource) {
-    return firstResult ->
-        secondTaskSource
-            .get()
-            .onSuccessTask(
-                executor,
-                secondResult ->
-                    Tasks.forResult(CombinedTaskResults.create(firstResult, secondResult)));
-  }
-
-  /**
-   * A version of #combineWithResultsOf that runs the listener that combines the results on the main
-   * thread.
-   */
   static <T1, T2> SuccessContinuation<T1, CombinedTaskResults<T1, T2>> combineWithResultOf(
       TaskSource<T2> secondTaskSource) {
     return firstResult ->

firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/AabUpdaterTest.java

@@ -15,9 +15,11 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.firebase.appdistribution.TestUtils.assertTaskFailure;
+import static com.google.firebase.appdistribution.TestUtils.awaitAsyncOperations;
 import static org.junit.Assert.assertEquals;
 import static org.mockito.Mockito.when;
 import static org.robolectric.Shadows.shadowOf;
+import static org.robolectric.annotation.LooperMode.Mode.PAUSED;
 
 import android.app.Activity;
 import android.net.Uri;
@@ -31,7 +33,6 @@
 import java.util.List;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
-import java.util.concurrent.TimeUnit;
 import javax.net.ssl.HttpsURLConnection;
 import org.junit.Before;
 import org.junit.Test;
@@ -41,9 +42,11 @@
 import org.mockito.MockitoAnnotations;
 import org.robolectric.Robolectric;
 import org.robolectric.RobolectricTestRunner;
+import org.robolectric.annotation.LooperMode;
 import org.robolectric.shadows.ShadowActivity;
 
 @RunWith(RobolectricTestRunner.class)
+@LooperMode(PAUSED)
 public class AabUpdaterTest {
   private static final String TEST_URL = "https://test-url";
   private static final String REDIRECT_TO_PLAY = "https://redirect-to-play-url";
@@ -95,7 +98,7 @@ public void updateAppTask_whenOpenConnectionFails_setsNetworkFailure()
     when(mockHttpsUrlConnectionFactory.openConnection(TEST_URL)).thenThrow(caughtException);
 
     UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
-    testExecutor.awaitTermination(100, TimeUnit.MILLISECONDS);
+    awaitAsyncOperations(testExecutor);
 
     assertTaskFailure(
         updateTask, Status.NETWORK_FAILURE, "Failed to open connection", caughtException);
@@ -107,7 +110,7 @@ public void updateAppTask_isNotRedirectResponse_setsDownloadFailure()
     when(mockHttpsUrlConnection.getResponseCode()).thenReturn(200);
 
     UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
-    testExecutor.awaitTermination(100, TimeUnit.MILLISECONDS);
+    awaitAsyncOperations(testExecutor);
 
     assertTaskFailure(updateTask, Status.DOWNLOAD_FAILURE, "Expected redirect");
   }
@@ -118,7 +121,7 @@ public void updateAppTask_missingLocationHeader_setsDownloadFailure()
     when(mockHttpsUrlConnection.getHeaderField("Location")).thenReturn(null);
 
     UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
-    testExecutor.awaitTermination(100, TimeUnit.MILLISECONDS);
+    awaitAsyncOperations(testExecutor);
 
     assertTaskFailure(updateTask, Status.DOWNLOAD_FAILURE, "No Location header");
   }
@@ -128,7 +131,7 @@ public void updateAppTask_emptyLocationHeader_setsDownloadFailure() throws Inter
     when(mockHttpsUrlConnection.getHeaderField("Location")).thenReturn("");
 
     UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
-    testExecutor.awaitTermination(100, TimeUnit.MILLISECONDS);
+    awaitAsyncOperations(testExecutor);
 
     assertTaskFailure(updateTask, Status.DOWNLOAD_FAILURE, "Empty Location header");
   }
@@ -139,7 +142,7 @@ public void updateAppTask_whenAabReleaseAvailable_redirectsToPlay() throws Excep
 
     UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
     updateTask.addOnProgressListener(testExecutor, progressEvents::add);
-    testExecutor.awaitTermination(100, TimeUnit.MILLISECONDS);
+    awaitAsyncOperations(testExecutor);
 
     assertThat(shadowActivity.getNextStartedActivity().getData())
         .isEqualTo(Uri.parse(REDIRECT_TO_PLAY));
@@ -156,12 +159,11 @@ public void updateAppTask_whenAabReleaseAvailable_redirectsToPlay() throws Excep
   @Test
   public void updateAppTask_onAppResume_setsUpdateCancelled() throws InterruptedException {
     UpdateTask updateTask = aabUpdater.updateAab(TEST_RELEASE_NEWER_AAB_INTERNAL);
-    testExecutor.awaitTermination(100, TimeUnit.MILLISECONDS);
+    awaitAsyncOperations(testExecutor);
     aabUpdater.onActivityStarted(activity);
 
     FirebaseAppDistributionException exception =
         assertTaskFailure(updateTask, Status.INSTALLATION_CANCELED, ErrorMessages.UPDATE_CANCELED);
-
     assertEquals(
         ReleaseUtils.convertToAppDistributionRelease(TEST_RELEASE_NEWER_AAB_INTERNAL),
         exception.getRelease());

firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/TestUtils.java

@@ -14,10 +14,14 @@
 
 package com.google.firebase.appdistribution;
 
+import static android.os.Looper.getMainLooper;
 import static com.google.common.truth.Truth.assertThat;
+import static org.robolectric.Shadows.shadowOf;
 
 import com.google.android.gms.tasks.Task;
 import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeUnit;
 
 final class TestUtils {
   private TestUtils() {}
@@ -37,4 +41,13 @@ static void assertTaskFailure(
     assertTaskFailure(task, status, messageSubstring);
     assertThat(task.getException()).hasCauseThat().isEqualTo(cause);
   }
+
+  static void awaitAsyncOperations(ExecutorService executorService) throws InterruptedException {
+    // Await anything enqueued to the executor
+    executorService.awaitTermination(100, TimeUnit.MILLISECONDS);
+
+    // Idle the main looper, which is also running these tests, so any Task or lifecycle callbacks
+    // can be handled. See http://robolectric.org/blog/2019/06/04/paused-looper/ for more info.
+    shadowOf(getMainLooper()).idle();
+  }
 }