From 52e7d23f537f545c2dff7ce11e772c227d651542 Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Tue, 9 Jun 2020 19:36:18 +0800 Subject: [PATCH 1/5] Do not buzzing for each progress of bugreport notification Only alert bugreport notification at the begining and end. Bug: 146135200 Bug: 175287931 Test: Manually changing the notification settings to default from silent. Change-Id: Ie955266ad8a7a27a9dc74743e276b5e9c7a2d6fb Merged-In: Ie955266ad8a7a27a9dc74743e276b5e9c7a2d6fb --- .../Shell/src/com/android/shell/BugreportProgressService.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java index 63b9bb39cba0e..c9daf538d2805 100644 --- a/packages/Shell/src/com/android/shell/BugreportProgressService.java +++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java @@ -1248,6 +1248,7 @@ public class BugreportProgressService extends Service { .setContentText(content) .setContentIntent(PendingIntent.getService(mContext, info.id, shareIntent, PendingIntent.FLAG_UPDATE_CURRENT)) + .setOnlyAlertOnce(false) .setDeleteIntent(newCancelIntent(mContext, info)); if (!TextUtils.isEmpty(info.getName())) { @@ -1287,6 +1288,7 @@ public class BugreportProgressService extends Service { .setLocalOnly(true) .setColor(context.getColor( com.android.internal.R.color.system_notification_accent_color)) + .setOnlyAlertOnce(true) .extend(new Notification.TvExtender()); } From 2150ce56dba111c6ec87aabecb0c78e7c891219a Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Tue, 16 Jun 2020 12:31:32 +0800 Subject: [PATCH 2/5] Fix BugreportReceiverTest - Implements a local binder and #onBind in the service to return the service instance. - Mocks BugreportManager in the service to avoid service interacting with the dumpstate. - Fixes dialog name field did not disable after service is finished. - Fixes screenshot did not remove if the name is empty. - Extends screenshot delay timeout in tests. - Fine tune the UiBot to fit new bugreport notification. - Removes obsolete cases in the tests. Bug: 143130523 Bug: 175287931 Test: atest BugreportReceiverTest Change-Id: Iae89206da1d08a10891503869bbbf1ce18d4e31f Merged-In: Iae89206da1d08a10891503869bbbf1ce18d4e31f --- .../shell/BugreportProgressService.java | 66 +- .../android/shell/BugreportReceiverTest.java | 644 +++++++----------- .../tests/src/com/android/shell/UiBot.java | 61 +- 3 files changed, 362 insertions(+), 409 deletions(-) diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java index c9daf538d2805..f7b1c5bf9c5b1 100644 --- a/packages/Shell/src/com/android/shell/BugreportProgressService.java +++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java @@ -47,6 +47,7 @@ import android.content.res.Configuration; import android.graphics.Bitmap; import android.net.Uri; import android.os.AsyncTask; +import android.os.Binder; import android.os.BugreportManager; import android.os.BugreportManager.BugreportCallback; import android.os.BugreportManager.BugreportCallback.BugreportErrorCode; @@ -186,7 +187,7 @@ public class BugreportProgressService extends Service { static final int SCREENSHOT_DELAY_SECONDS = 3; /** System property where dumpstate stores last triggered bugreport id */ - private static final String PROPERTY_LAST_ID = "dumpstate.last_id"; + static final String PROPERTY_LAST_ID = "dumpstate.last_id"; private static final String BUGREPORT_SERVICE = "bugreport"; @@ -233,7 +234,7 @@ public class BugreportProgressService extends Service { private File mBugreportsDir; - private BugreportManager mBugreportManager; + @VisibleForTesting BugreportManager mBugreportManager; /** * id of the notification used to set service on foreground. @@ -248,6 +249,11 @@ public class BugreportProgressService extends Service { */ private boolean mTakingScreenshot; + /** + * The delay timeout before taking a screenshot. + */ + @VisibleForTesting int mScreenshotDelaySec = SCREENSHOT_DELAY_SECONDS; + @GuardedBy("sNotificationBundle") private static final Bundle sNotificationBundle = new Bundle(); @@ -282,6 +288,7 @@ public class BugreportProgressService extends Service { mContext.getString(R.string.bugreport_notification_channel), isTv(this) ? NotificationManager.IMPORTANCE_DEFAULT : NotificationManager.IMPORTANCE_LOW)); + mBugreportManager = mContext.getSystemService(BugreportManager.class); } @Override @@ -305,7 +312,7 @@ public class BugreportProgressService extends Service { @Override public IBinder onBind(Intent intent) { - return null; + return new LocalBinder(); } @Override @@ -375,6 +382,7 @@ public class BugreportProgressService extends Service { mInfo.renameScreenshots(); synchronized (mLock) { sendBugreportFinishedBroadcastLocked(); + mMainThreadHandler.post(() -> mInfoDialog.onBugreportFinished(mInfo)); } } @@ -627,8 +635,6 @@ public class BugreportProgressService extends Service { } } - mBugreportManager = (BugreportManager) mContext.getSystemService( - Context.BUGREPORT_SERVICE); final Executor executor = ActivityThread.currentActivityThread().getExecutor(); Log.i(TAG, "bugreport type = " + bugreportType @@ -888,12 +894,12 @@ public class BugreportProgressService extends Service { collapseNotificationBar(); final String msg = mContext.getResources() .getQuantityString(com.android.internal.R.plurals.bugreport_countdown, - SCREENSHOT_DELAY_SECONDS, SCREENSHOT_DELAY_SECONDS); + mScreenshotDelaySec, mScreenshotDelaySec); Log.i(TAG, msg); // Show a toast just once, otherwise it might be captured in the screenshot. Toast.makeText(mContext, msg, Toast.LENGTH_SHORT).show(); - takeScreenshot(id, SCREENSHOT_DELAY_SECONDS); + takeScreenshot(id, mScreenshotDelaySec); } /** @@ -1622,6 +1628,16 @@ public class BugreportProgressService extends Service { || c == '_' || c == '-'; } + /** + * A local binder with interface to return an instance of BugreportProgressService for the + * purpose of testing. + */ + final class LocalBinder extends Binder { + @VisibleForTesting BugreportProgressService getService() { + return BugreportProgressService.this; + } + } + /** * Helper class encapsulating the UI elements and logic used to display a dialog where user * can change the details of a bugreport. @@ -1751,6 +1767,22 @@ public class BugreportProgressService extends Service { } } + /** + * Notifies the dialog that the bugreport has finished so it disables the {@code name} + * field. + *

Once the bugreport is finished dumpstate has already generated the final files, so + * changing the name would have no effect. + */ + void onBugreportFinished(BugreportInfo info) { + if (mId == info.id && mInfoName != null) { + mInfoName.setEnabled(false); + mInfoName.setText(null); + if (!TextUtils.isEmpty(info.getName())) { + mInfoName.setText(info.getName()); + } + } + } + void cancel() { if (mDialog != null) { mDialog.cancel(); @@ -1995,12 +2027,21 @@ public class BugreportProgressService extends Service { Log.i(TAG, "Deleting empty bugreport file: " + bugreportFile); bugreportFile.delete(); } - for (File file : screenshotFiles) { - if (file.length() == 0) { + deleteEmptyScreenshots(); + } + + /** + * Deletes empty screenshot files. + */ + private void deleteEmptyScreenshots() { + screenshotFiles.removeIf(file -> { + final long length = file.length(); + if (length == 0) { Log.i(TAG, "Deleting empty screenshot file: " + file); file.delete(); } - } + return length == 0; + }); } /** @@ -2008,7 +2049,8 @@ public class BugreportProgressService extends Service { * {@code initialName} if user has changed it. */ void renameScreenshots() { - if (TextUtils.isEmpty(name)) { + deleteEmptyScreenshots(); + if (TextUtils.isEmpty(name) || screenshotFiles.isEmpty()) { return; } final List renamedFiles = new ArrayList<>(screenshotFiles.size()); @@ -2027,7 +2069,7 @@ public class BugreportProgressService extends Service { if (newFile.length() > 0) { renamedFiles.add(newFile); } else if (newFile.delete()) { - Log.d(TAG, "screenshot file: " + newFile + "deleted successfully."); + Log.d(TAG, "screenshot file: " + newFile + " deleted successfully."); } } screenshotFiles = renamedFiles; diff --git a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java index b8cfa1e800439..3b02e3b465577 100644 --- a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java +++ b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java @@ -25,19 +25,22 @@ import static com.android.shell.BugreportPrefs.STATE_SHOW; import static com.android.shell.BugreportPrefs.STATE_UNKNOWN; import static com.android.shell.BugreportPrefs.getWarningState; import static com.android.shell.BugreportPrefs.setWarningState; -import static com.android.shell.BugreportProgressService.EXTRA_BUGREPORT; -import static com.android.shell.BugreportProgressService.EXTRA_ID; -import static com.android.shell.BugreportProgressService.EXTRA_NAME; -import static com.android.shell.BugreportProgressService.EXTRA_SCREENSHOT; -import static com.android.shell.BugreportProgressService.INTENT_BUGREPORT_FINISHED; +import static com.android.shell.BugreportProgressService.INTENT_BUGREPORT_REQUESTED; +import static com.android.shell.BugreportProgressService.PROPERTY_LAST_ID; import static com.android.shell.BugreportProgressService.SCREENSHOT_DELAY_SECONDS; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.timeout; +import static org.mockito.Mockito.verify; import android.app.ActivityManager; import android.app.ActivityManager.RunningServiceInfo; @@ -46,13 +49,18 @@ import android.app.NotificationManager; import android.content.Context; import android.content.Intent; import android.net.Uri; +import android.os.BugreportManager; import android.os.Build; import android.os.Bundle; +import android.os.IDumpstate; +import android.os.IDumpstateListener; +import android.os.ParcelFileDescriptor; import android.os.SystemClock; import android.os.SystemProperties; import android.service.notification.StatusBarNotification; import android.support.test.uiautomator.UiDevice; import android.support.test.uiautomator.UiObject; +import android.support.test.uiautomator.UiObject2; import android.support.test.uiautomator.UiObjectNotFoundException; import android.text.TextUtils; import android.text.format.DateUtils; @@ -60,10 +68,12 @@ import android.util.Log; import androidx.test.InstrumentationRegistry; import androidx.test.filters.LargeTest; +import androidx.test.rule.ServiceTestRule; import androidx.test.runner.AndroidJUnit4; import com.android.shell.ActionSendMultipleConsumerActivity.CustomActionSendMultipleListener; +import libcore.io.IoUtils; import libcore.io.Streams; import org.junit.After; @@ -72,17 +82,19 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.TestName; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; import java.io.BufferedOutputStream; import java.io.BufferedWriter; import java.io.ByteArrayOutputStream; -import java.io.File; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStreamWriter; import java.io.Writer; -import java.text.NumberFormat; import java.util.ArrayList; import java.util.List; import java.util.SortedSet; @@ -92,10 +104,10 @@ import java.util.zip.ZipInputStream; import java.util.zip.ZipOutputStream; /** - * Integration tests for {@link BugreportReceiver}. + * Integration tests for {@link BugreportProgressService}. *

- * These tests don't mock any component and rely on external UI components (like the notification - * bar and activity chooser), which can make them unreliable and slow. + * These tests rely on external UI components (like the notificatio bar and activity chooser), + * which can make them unreliable and slow. *

* The general workflow is: *