Clean up DownloadManager threading tests.

Change runUntilStatus() methods to polling with timeout instead of
requiring internal knowledge about threading.

Fix notification tests, and move opening of InputStream until after
handling headers to avoid FNFE.  Always reset facade to defaults
before each test.

Change-Id: I6b2d6cfc4e685d2090c1133b1b2e89ae12760f8b

12 files changed, 84 insertions, 127 deletions

diff --git a/src/com/android/providers/downloads/DownloadHandler.java b/src/com/android/providers/downloads/DownloadHandler.java
index 2f02864e..c376ff1e 100644
--- a/src/com/android/providers/downloads/DownloadHandler.java
+++ b/src/com/android/providers/downloads/DownloadHandler.java
@@ -96,28 +96,4 @@ public class DownloadHandler {
     public synchronized long getCurrentSpeed(long id) {
         return mCurrentSpeed.get(id, -1L);
     }
-
-    // right now this is only used by tests. but there is no reason why it can't be used
-    // by any module using DownloadManager (TODO add API to DownloadManager.java)
-    public synchronized void waitUntilDownloadsTerminate() throws InterruptedException {
-        if (mDownloadsInProgress.size() == 0 && mDownloadsQueue.size() == 0) {
-            if (Constants.LOGVV) {
-                Log.i(TAG, "nothing to wait on");
-            }
-            return;
-        }
-        if (Constants.LOGVV) {
-            for (DownloadInfo info : mDownloadsInProgress.values()) {
-                Log.i(TAG, "** progress: " + info.mId + ", " + info.mUri);
-            }
-            for (DownloadInfo info : mDownloadsQueue.values()) {
-                Log.i(TAG, "** in Q: " + info.mId + ", " + info.mUri);
-            }
-        }
-        if (Constants.LOGVV) {
-            Log.i(TAG, "waiting for 5 sec");
-        }
-        // wait upto 5 sec
-        wait(5 * 1000);
-    }
 }
diff --git a/src/com/android/providers/downloads/DownloadInfo.java b/src/com/android/providers/downloads/DownloadInfo.java
index 2ea7d84d..e64db289 100644
--- a/src/com/android/providers/downloads/DownloadInfo.java
+++ b/src/com/android/providers/downloads/DownloadInfo.java
@@ -575,9 +575,8 @@ public class DownloadInfo {
     }
 
     void startDownloadThread() {
-        DownloadThread downloader = new DownloadThread(mContext, mSystemFacade, this,
-                mStorageManager);
-        mSystemFacade.startThread(downloader);
+        // TODO: keep this thread strongly referenced
+        new DownloadThread(mContext, mSystemFacade, this, mStorageManager).start();
     }
 
     /**
diff --git a/src/com/android/providers/downloads/DownloadService.java b/src/com/android/providers/downloads/DownloadService.java
index e0fe4c55..4a1b40d5 100644
--- a/src/com/android/providers/downloads/DownloadService.java
+++ b/src/com/android/providers/downloads/DownloadService.java
@@ -259,7 +259,7 @@ public class DownloadService extends Service {
             mPendingUpdate = true;
             if (mUpdateThread == null) {
                 mUpdateThread = new UpdateThread();
-                mSystemFacade.startThread(mUpdateThread);
+                mUpdateThread.start();
             }
         }
     }
diff --git a/src/com/android/providers/downloads/DownloadThread.java b/src/com/android/providers/downloads/DownloadThread.java
index eb59d3f7..ae279260 100644
--- a/src/com/android/providers/downloads/DownloadThread.java
+++ b/src/com/android/providers/downloads/DownloadThread.java
@@ -261,10 +261,9 @@ public class DownloadThread extends Thread {
             try {
                 // Asking for response code will execute the request
                 final int statusCode = conn.getResponseCode();
-                in = conn.getInputStream();
-
                 handleExceptionalStatus(state, conn, statusCode);
                 processResponseHeaders(state, conn);
+                in = conn.getInputStream();
             } catch (IOException e) {
                 throw new StopRequestException(
                         getFinalStatusForHttpError(state), "Request failed: " + e, e);
diff --git a/src/com/android/providers/downloads/RealSystemFacade.java b/src/com/android/providers/downloads/RealSystemFacade.java
index 228c7165..fa4f3488 100644
--- a/src/com/android/providers/downloads/RealSystemFacade.java
+++ b/src/com/android/providers/downloads/RealSystemFacade.java
@@ -32,10 +32,12 @@ class RealSystemFacade implements SystemFacade {
         mContext = context;
     }
 
+    @Override
     public long currentTimeMillis() {
         return System.currentTimeMillis();
     }
 
+    @Override
     public NetworkInfo getActiveNetworkInfo(int uid) {
         ConnectivityManager connectivity =
                 (ConnectivityManager) mContext.getSystemService(Context.CONNECTIVITY_SERVICE);
@@ -57,6 +59,7 @@ class RealSystemFacade implements SystemFacade {
         return conn.isActiveNetworkMetered();
     }
 
+    @Override
     public boolean isNetworkRoaming() {
         ConnectivityManager connectivity =
             (ConnectivityManager) mContext.getSystemService(Context.CONNECTIVITY_SERVICE);
@@ -74,6 +77,7 @@ class RealSystemFacade implements SystemFacade {
         return isRoaming;
     }
 
+    @Override
     public Long getMaxBytesOverMobile() {
         return DownloadManager.getMaxBytesOverMobile(mContext);
     }
@@ -92,9 +96,4 @@ class RealSystemFacade implements SystemFacade {
     public boolean userOwnsPackage(int uid, String packageName) throws NameNotFoundException {
         return mContext.getPackageManager().getApplicationInfo(packageName, 0).uid == uid;
     }
-
-    @Override
-    public void startThread(Thread thread) {
-        thread.start();
-    }
 }
diff --git a/src/com/android/providers/downloads/SystemFacade.java b/src/com/android/providers/downloads/SystemFacade.java
index fda97e08..15fc31f9 100644
--- a/src/com/android/providers/downloads/SystemFacade.java
+++ b/src/com/android/providers/downloads/SystemFacade.java
@@ -61,9 +61,4 @@ interface SystemFacade {
      * Returns true if the specified UID owns the specified package name.
      */
     public boolean userOwnsPackage(int uid, String pckg) throws NameNotFoundException;
-
-    /**
-     * Start a thread.
-     */
-    public void startThread(Thread thread);
 }
diff --git a/tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java b/tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java
index a65693fa..48eb0b47 100644
--- a/tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java
+++ b/tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java
@@ -161,6 +161,7 @@ public abstract class AbstractDownloadProviderFunctionalTest extends
         setContext(mTestContext);
         setupService();
         getService().mSystemFacade = mSystemFacade;
+        mSystemFacade.setUp();
         assertTrue(isDatabaseEmpty()); // ensure we're not messing with real data
         mServer = new MockWebServer();
         mServer.play();
@@ -246,11 +247,6 @@ public abstract class AbstractDownloadProviderFunctionalTest extends
         return mServer.getUrl(path).toString();
     }
 
-    public void runService() throws Exception {
-        startService(null);
-        mSystemFacade.runAllThreads();
-    }
-
     protected String readStream(InputStream inputStream) throws IOException {
         BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream));
         try {
diff --git a/tests/src/com/android/providers/downloads/AbstractPublicApiTest.java b/tests/src/com/android/providers/downloads/AbstractPublicApiTest.java
index cda607aa..12c04f6a 100644
--- a/tests/src/com/android/providers/downloads/AbstractPublicApiTest.java
+++ b/tests/src/com/android/providers/downloads/AbstractPublicApiTest.java
@@ -16,17 +16,22 @@
 
 package com.android.providers.downloads;
 
+import static android.app.DownloadManager.STATUS_FAILED;
+import static android.app.DownloadManager.STATUS_SUCCESSFUL;
+import static android.text.format.DateUtils.SECOND_IN_MILLIS;
+
 import android.app.DownloadManager;
 import android.database.Cursor;
 import android.net.Uri;
 import android.os.ParcelFileDescriptor;
-import android.provider.Downloads;
+import android.os.SystemClock;
 import android.util.Log;
 
 import java.io.FileInputStream;
 import java.io.InputStream;
 import java.net.MalformedURLException;
 import java.net.UnknownHostException;
+import java.util.concurrent.TimeoutException;
 
 /**
  * Code common to tests that use the download manager public API.
@@ -94,9 +99,28 @@ public abstract class AbstractPublicApiTest extends AbstractDownloadProviderFunc
             }
         }
 
-        void runUntilStatus(int status) throws Exception {
-            runService();
-            assertEquals(status, getStatus());
+        void runUntilStatus(int status) throws TimeoutException {
+            startService(null);
+            waitForStatus(status);
+        }
+
+        void waitForStatus(int expected) throws TimeoutException {
+            int actual = -1;
+
+            final long timeout = SystemClock.elapsedRealtime() + (15 * SECOND_IN_MILLIS);
+            while (SystemClock.elapsedRealtime() < timeout) {
+                actual = getStatus();
+                if (actual == STATUS_SUCCESSFUL || actual == STATUS_FAILED) {
+                    assertEquals(expected, actual);
+                    return;
+                } else if (actual == expected) {
+                    return;
+                }
+
+                SystemClock.sleep(100);
+            }
+
+            throw new TimeoutException("Expected status " + expected + "; only reached " + actual);
         }
 
         // max time to wait before giving up on the current download operation.
@@ -105,22 +129,10 @@ public abstract class AbstractPublicApiTest extends AbstractDownloadProviderFunc
         // download thread
         private static final int TIME_TO_SLEEP = 1000;
 
-        int runUntilDone() throws InterruptedException {
-            int sleepCounter = MAX_TIME_TO_WAIT_FOR_OPERATION * 1000 / TIME_TO_SLEEP;
-            for (int i = 0; i < sleepCounter; i++) {
-                int status = getStatusIfExists();
-                if (status == -1 || Downloads.Impl.isStatusCompleted(getStatus())) {
-                    // row doesn't exist or the download is done
-                    return status;
-                }
-                // download not done yet. sleep a while and try again
-                Thread.sleep(TIME_TO_SLEEP);
-            }
-            return 0; // failed
-        }
-
         // waits until progress_so_far is >= (progress)%
         boolean runUntilProgress(int progress) throws InterruptedException {
+            startService(null);
+
             int sleepCounter = MAX_TIME_TO_WAIT_FOR_OPERATION * 1000 / TIME_TO_SLEEP;
             int numBytesReceivedSoFar = 0;
             int totalBytes = 0;
diff --git a/tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java b/tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java
index 23d300f8..d5249378 100644
--- a/tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java
+++ b/tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java
@@ -16,14 +16,16 @@
 
 package com.android.providers.downloads;
 
+import static android.text.format.DateUtils.SECOND_IN_MILLIS;
+
 import android.content.ContentValues;
 import android.database.Cursor;
 import android.net.ConnectivityManager;
 import android.net.Uri;
 import android.os.Environment;
+import android.os.SystemClock;
 import android.provider.Downloads;
 import android.test.suitebuilder.annotation.LargeTest;
-import android.util.Log;
 
 import com.google.mockwebserver.MockWebServer;
 import com.google.mockwebserver.RecordedRequest;
@@ -31,6 +33,7 @@ import com.google.mockwebserver.RecordedRequest;
 import java.io.InputStream;
 import java.net.MalformedURLException;
 import java.net.UnknownHostException;
+import java.util.concurrent.TimeoutException;
 
 /**
  * This test exercises the entire download manager working together -- it requests downloads through
@@ -109,20 +112,22 @@ public class DownloadProviderFunctionalTest extends AbstractDownloadProviderFunc
         }
     }
 
-    private void runUntilStatus(Uri downloadUri, int status) throws Exception {
-        runService();
-        boolean done = false;
-        while (!done) {
-            int rslt = getDownloadStatus(downloadUri);
-            if (rslt == Downloads.Impl.STATUS_RUNNING || rslt == Downloads.Impl.STATUS_PENDING) {
-                Log.i(TAG, "status is: " + rslt + ", for: " + downloadUri);
-                DownloadHandler.getInstance().waitUntilDownloadsTerminate();
-                Thread.sleep(100);
-            } else {
-                done = true;
+    private void runUntilStatus(Uri downloadUri, int expected) throws Exception {
+        startService(null);
+        
+        int actual = -1;
+
+        final long timeout = SystemClock.elapsedRealtime() + (15 * SECOND_IN_MILLIS);
+        while (SystemClock.elapsedRealtime() < timeout) {
+            actual = getDownloadStatus(downloadUri);
+            if (expected == actual) {
+                return;
             }
+
+            SystemClock.sleep(100);
         }
-        assertEquals(status, getDownloadStatus(downloadUri));
+
+        throw new TimeoutException("Expected status " + expected + "; only reached " + actual);
     }
 
     protected int getDownloadStatus(Uri downloadUri) {
diff --git a/tests/src/com/android/providers/downloads/FakeSystemFacade.java b/tests/src/com/android/providers/downloads/FakeSystemFacade.java
index 481b5cba..d54c1224 100644
--- a/tests/src/com/android/providers/downloads/FakeSystemFacade.java
+++ b/tests/src/com/android/providers/downloads/FakeSystemFacade.java
@@ -7,10 +7,7 @@ import android.net.NetworkInfo;
 import android.net.NetworkInfo.DetailedState;
 
 import java.util.ArrayList;
-import java.util.LinkedList;
 import java.util.List;
-import java.util.Queue;
-
 public class FakeSystemFacade implements SystemFacade {
     long mTimeMillis = 0;
     Integer mActiveNetworkType = ConnectivityManager.TYPE_WIFI;
@@ -19,20 +16,32 @@ public class FakeSystemFacade implements SystemFacade {
     Long mMaxBytesOverMobile = null;
     Long mRecommendedMaxBytesOverMobile = null;
     List<Intent> mBroadcastsSent = new ArrayList<Intent>();
-    Queue<Thread> mStartedThreads = new LinkedList<Thread>();
-    private boolean returnActualTime = false;
+    private boolean mReturnActualTime = false;
+
+    public void setUp() {
+        mTimeMillis = 0;
+        mActiveNetworkType = ConnectivityManager.TYPE_WIFI;
+        mIsRoaming = false;
+        mIsMetered = false;
+        mMaxBytesOverMobile = null;
+        mRecommendedMaxBytesOverMobile = null;
+        mBroadcastsSent.clear();
+        mReturnActualTime = false;
+    }
 
     void incrementTimeMillis(long delta) {
         mTimeMillis += delta;
     }
 
+    @Override
     public long currentTimeMillis() {
-        if (returnActualTime) {
+        if (mReturnActualTime) {
             return System.currentTimeMillis();
         }
         return mTimeMillis;
     }
 
+    @Override
     public NetworkInfo getActiveNetworkInfo(int uid) {
         if (mActiveNetworkType == null) {
             return null;
@@ -48,14 +57,17 @@ public class FakeSystemFacade implements SystemFacade {
         return mIsMetered;
     }
 
+    @Override
     public boolean isNetworkRoaming() {
         return mIsRoaming;
     }
 
+    @Override
     public Long getMaxBytesOverMobile() {
         return mMaxBytesOverMobile ;
     }
 
+    @Override
     public Long getRecommendedMaxBytesOverMobile() {
         return mRecommendedMaxBytesOverMobile ;
     }
@@ -70,27 +82,7 @@ public class FakeSystemFacade implements SystemFacade {
         return true;
     }
 
-    public boolean startThreadsWithoutWaiting = false;
-    public void setStartThreadsWithoutWaiting(boolean flag) {
-        this.startThreadsWithoutWaiting = flag;
-    }
-
-    @Override
-    public void startThread(Thread thread) {
-        if (startThreadsWithoutWaiting) {
-            thread.start();
-        } else {
-            mStartedThreads.add(thread);
-        }
-    }
-
-    public void runAllThreads() {
-        while (!mStartedThreads.isEmpty()) {
-            mStartedThreads.poll().run();
-        }
-    }
-
     public void setReturnActualTime(boolean flag) {
-        returnActualTime = flag;
+        mReturnActualTime = flag;
     }
 }
diff --git a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
index 1e6b7053..4159beb6 100644
--- a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
+++ b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
@@ -47,7 +47,6 @@ import java.io.InputStream;
 import java.net.MalformedURLException;
 import java.util.List;
 
-
 @LargeTest
 public class PublicApiFunctionalTest extends AbstractPublicApiTest {
     private static final String REDIRECTED_PATH = "/other_path";
@@ -76,7 +75,6 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         } else {
             mTestDirectory.mkdir();
         }
-        mSystemFacade.setStartThreadsWithoutWaiting(false);
     }
 
     @Override
@@ -412,22 +410,18 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
     }
 
     public void testCancel() throws Exception {
-        mSystemFacade.setStartThreadsWithoutWaiting(true);
         // return 'real time' from FakeSystemFacade so that DownloadThread will report progress
         mSystemFacade.setReturnActualTime(true);
         enqueueResponse(buildContinuingResponse());
         Download download = enqueueRequest(getRequest());
-        startService(null);
         // give the download time to get started and progress to 1% completion
         // before cancelling it.
         boolean rslt = download.runUntilProgress(1);
         assertTrue(rslt);
         mManager.remove(download.mId);
-        startService(null);
-        int status = download.runUntilDone();
+
         // make sure the row is gone from the database
-        assertEquals(-1, status);
-        mSystemFacade.setReturnActualTime(false);
+        download.waitForStatus(-1);
     }
 
     public void testDownloadCompleteBroadcast() throws Exception {
@@ -512,9 +506,9 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
 
     public void testContentObserver() throws Exception {
         enqueueResponse(buildEmptyResponse(HTTP_OK));
-        enqueueRequest(getRequest());
         mResolver.resetNotified();
-        runService();
+        final Download download = enqueueRequest(getRequest());
+        download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL);
         assertTrue(mResolver.mNotifyWasCalled);
     }
 
@@ -524,10 +518,9 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         final Download download = enqueueRequest(
                 getRequest().setNotificationVisibility(DownloadManager.Request.VISIBILITY_HIDDEN));
         download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL);
-        runService();
 
+        verify(mNotifManager, times(1)).cancelAll();
         verify(mNotifManager, never()).notify(anyString(), anyInt(), isA(Notification.class));
-        // TODO: verify that it never cancels
     }
 
     public void testNotificationVisible() throws Exception {
@@ -536,11 +529,10 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         // only shows in-progress notifications
         final Download download = enqueueRequest(getRequest());
         download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL);
-        runService();
 
         // TODO: verify different notif types with tags
+        verify(mNotifManager, times(1)).cancelAll();
         verify(mNotifManager, atLeastOnce()).notify(anyString(), anyInt(), isA(Notification.class));
-        verify(mNotifManager, times(1)).cancel(anyString(), anyInt());
     }
 
     public void testNotificationVisibleComplete() throws Exception {
@@ -549,11 +541,10 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         final Download download = enqueueRequest(getRequest().setNotificationVisibility(
                 DownloadManager.Request.VISIBILITY_VISIBLE_NOTIFY_COMPLETED));
         download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL);
-        runService();
 
         // TODO: verify different notif types with tags
+        verify(mNotifManager, times(1)).cancelAll();
         verify(mNotifManager, atLeastOnce()).notify(anyString(), anyInt(), isA(Notification.class));
-        verify(mNotifManager, times(1)).cancel(anyString(), anyInt());
     }
 
     public void testRetryAfter() throws Exception {
diff --git a/tests/src/com/android/providers/downloads/ThreadingTest.java b/tests/src/com/android/providers/downloads/ThreadingTest.java
index 8605c76b..e73bae2a 100644
--- a/tests/src/com/android/providers/downloads/ThreadingTest.java
+++ b/tests/src/com/android/providers/downloads/ThreadingTest.java
@@ -24,15 +24,8 @@ import android.test.suitebuilder.annotation.LargeTest;
  */
 @LargeTest
 public class ThreadingTest extends AbstractPublicApiTest {
-    private static class FakeSystemFacadeWithThreading extends FakeSystemFacade {
-        @Override
-        public void startThread(Thread thread) {
-            thread.start();
-        }
-    }
-
     public ThreadingTest() {
-        super(new FakeSystemFacadeWithThreading());
+        super(new FakeSystemFacade());
     }
 
     @Override