Clean up DownloadManager threading tests.
Jeff Sharkey [Fri, 4 Jan 2013 06:59:50 +0000 (22:59 -0800)]
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:
src/com/android/providers/downloads/DownloadHandler.java
src/com/android/providers/downloads/DownloadInfo.java
src/com/android/providers/downloads/DownloadService.java
src/com/android/providers/downloads/DownloadThread.java
src/com/android/providers/downloads/RealSystemFacade.java
src/com/android/providers/downloads/SystemFacade.java
tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java
tests/src/com/android/providers/downloads/AbstractPublicApiTest.java
tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java
tests/src/com/android/providers/downloads/FakeSystemFacade.java
tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
tests/src/com/android/providers/downloads/ThreadingTest.java

index 2f02864..c376ff1 100644 (file)
@@ -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);
-    }
 }
index 2ea7d84..e64db28 100644 (file)
@@ -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();
     }
 
     /**
index e0fe4c5..4a1b40d 100644 (file)
@@ -259,7 +259,7 @@ public class DownloadService extends Service {
             mPendingUpdate = true;
             if (mUpdateThread == null) {
                 mUpdateThread = new UpdateThread();
-                mSystemFacade.startThread(mUpdateThread);
+                mUpdateThread.start();
             }
         }
     }
index eb59d3f..ae27926 100644 (file)
@@ -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);
index 228c716..fa4f348 100644 (file)
@@ -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();
-    }
 }
index fda97e0..15fc31f 100644 (file)
@@ -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);
 }
index a65693f..48eb0b4 100644 (file)
@@ -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 {
index cda607a..12c04f6 100644 (file)
 
 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;
index 23d300f..d524937 100644 (file)
 
 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) {
index 481b5cb..d54c122 100644 (file)
@@ -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;
     }
 }
index 1e6b705..4159beb 100644 (file)
@@ -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 {
index 8605c76..e73bae2 100644 (file)
@@ -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