Fixed corner-case scenario where a screenshot is finished after the share

notification is sent.

Prior to this change, if a screenshot finished after the share
notification was sent, it would replace the share notification with a
progress notification, and the share notification would never be sent
again.

Also improved the test cases that automatically generate a screenshot
but don't use it to wait for the screenshot to finish before proceeding,
otherwise it could cause a future test to fail (if the screenshot is
finished after the initial test is completed).

Change-Id: I6e2a6549ebb48e5bebf5aa78d1bda94404c1812b

diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java
index d31088c..f7a2d75 100644 (file)
--- a/packages/Shell/src/com/android/shell/BugreportProgressService.java
+++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java
@@ -449,6 +449,11 @@ public class BugreportProgressService extends Service {
                 .addAction(cancelAction)
                 .build();
 
+        if (info.finished) {
+            Log.w(TAG, "Not sending progress notification because bugreport has finished already ("
+                    + info + ")");
+            return;
+        }
         NotificationManager.from(mContext).notify(TAG, info.pid, notification);
     }
 
@@ -628,7 +633,12 @@ public class BugreportProgressService extends Service {
         synchronized (BugreportProgressService.this) {
             mTakingScreenshot = flag;
             for (int i = 0; i < mProcesses.size(); i++) {
-                updateProgress(mProcesses.valueAt(i));
+                final BugreportInfo info = mProcesses.valueAt(i);
+                if (info.finished) {
+                    Log.d(TAG, "Not updating progress because share notification was already sent");
+                    continue;
+                }
+                updateProgress(info);
             }
         }
     }

diff --git a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java
index 6bee767..8e8924a 100644 (file)
--- a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java
+++ b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java
@@ -142,6 +142,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
     public void testProgress() throws Exception {
         resetProperties();
         sendBugreportStarted(1000);
+        waitForScreenshotButtonEnabled(true);
 
         assertProgressNotification(NAME, "0.00%");
 
@@ -157,7 +158,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
         Bundle extras =
                 sendBugreportFinishedAndGetSharedIntent(PID, mPlainTextPath, mScreenshotPath);
         assertActionSendMultiple(extras, BUGREPORT_FILE, BUGREPORT_CONTENT, PID, NAME, ZIP_FILE,
-                null, 1);
+                null, 1, true);
 
         assertServiceNotRunning();
     }
@@ -174,7 +175,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
         Bundle extras =
                 sendBugreportFinishedAndGetSharedIntent(PID, mPlainTextPath, mScreenshotPath);
         assertActionSendMultiple(extras, BUGREPORT_FILE, BUGREPORT_CONTENT, PID, NAME, ZIP_FILE,
-                null, 2);
+                null, 2, true);
 
         assertServiceNotRunning();
     }
@@ -182,6 +183,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
     public void testProgress_changeDetails() throws Exception {
         resetProperties();
         sendBugreportStarted(1000);
+        waitForScreenshotButtonEnabled(true);
 
         DetailsUi detailsUi = new DetailsUi(mUiBot);
 
@@ -218,14 +220,33 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
         Bundle extras = sendBugreportFinishedAndGetSharedIntent(PID, mPlainTextPath,
                 mScreenshotPath);
         assertActionSendMultiple(extras, BUGREPORT_FILE, BUGREPORT_CONTENT, PID, NEW_NAME, TITLE,
-                mDescription, 1);
+                mDescription, 1, true);
 
         assertServiceNotRunning();
     }
 
+    /**
+     * Tests the scenario where the initial screenshot and dumpstate are finished while the user
+     * is changing the info in the details screen.
+     */
+    public void testProgress_bugreportAndScreenshotFinishedWhileChangingDetails() throws Exception {
+        bugreportFinishedWhileChangingDetailsTest(false);
+    }
+
+    /**
+     * Tests the scenario where dumpstate is finished while the user is changing the info in the
+     * details screen, but the initial screenshot finishes afterwards.
+     */
     public void testProgress_bugreportFinishedWhileChangingDetails() throws Exception {
+        bugreportFinishedWhileChangingDetailsTest(true);
+    }
+
+    private void bugreportFinishedWhileChangingDetailsTest(boolean waitScreenshot) throws Exception {
         resetProperties();
         sendBugreportStarted(1000);
+        if (waitScreenshot) {
+            waitForScreenshotButtonEnabled(true);
+        }
 
         DetailsUi detailsUi = new DetailsUi(mUiBot);
 
@@ -233,7 +254,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
         detailsUi.nameField.setText(NEW_NAME);
         sendBugreportFinished(PID, mPlainTextPath, mScreenshotPath);
 
-        // Wait until the share notifcation is received...
+        // Wait until the share notification is received...
         mUiBot.getNotification(mContext.getString(R.string.bugreport_finished_title));
         // ...then close notification bar.
         mContext.sendBroadcast(new Intent(Intent.ACTION_CLOSE_SYSTEM_DIALOGS));
@@ -250,7 +271,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
         // Finally, share bugreport.
         Bundle extras = acceptBugreportAndGetSharedIntent();
         assertActionSendMultiple(extras, BUGREPORT_FILE, BUGREPORT_CONTENT, PID, NAME, TITLE,
-                mDescription, 1);
+                mDescription, 1, waitScreenshot);
 
         assertServiceNotRunning();
     }
@@ -406,7 +427,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
     private void assertActionSendMultiple(Bundle extras, String bugreportContent,
             String screenshotContent) throws IOException {
         assertActionSendMultiple(extras, bugreportContent, screenshotContent, PID, null, ZIP_FILE,
-                null, 0);
+                null, 0, false);
     }
 
     /**
@@ -420,10 +441,11 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
      * @param title bugreport name as provided by the user (or received by dumpstate)
      * @param description bugreport description as provided by the user
      * @param numberScreenshots expected number of screenshots taken by Shell.
+     * @param renamedScreenshots whether the screenshots are expected to be renamed
      */
     private void assertActionSendMultiple(Bundle extras, String bugreportContent,
             String screenshotContent, int pid, String name, String title, String description,
-            int numberScreenshots) throws IOException {
+            int numberScreenshots, boolean renamedScreenshots) throws IOException {
         String body = extras.getString(Intent.EXTRA_TEXT);
         assertContainsRegex("missing build info",
                 SystemProperties.get("ro.build.description"), body);
@@ -480,7 +502,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
         // Check internal screenshots.
         SortedSet<String> expectedNames = new TreeSet<>();
         for (int i = 1 ; i <= numberScreenshots; i++) {
-            String prefix = name != null ? name : Integer.toString(pid);
+            String prefix = renamedScreenshots  ? name : Integer.toString(pid);
             String expectedName = "screenshot-" + prefix + "-" + i + ".png";
             expectedNames.add(expectedName);
         }
@@ -592,7 +614,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase {
 
     private UiObject waitForScreenshotButtonEnabled(boolean expectedEnabled) throws Exception {
         UiObject screenshotButton = getScreenshotButton();
-        int maxAttempts = SCREENSHOT_DELAY_SECONDS + 2;
+        int maxAttempts = SCREENSHOT_DELAY_SECONDS + 5;
         int i = 0;
         do {
             boolean enabled = screenshotButton.isEnabled();