From 8c14f77cb28d6c76312a765b08433c09d81194a6 Mon Sep 17 00:00:00 2001 From: Susi Kharraz-Post Date: Wed, 17 Apr 2019 16:33:41 -0400 Subject: [PATCH] Add logging field for direct share selection To help evaluate the new ranking algo we want to know if the package for a direct share was also offered as one of the ranked apps. Logging the position if it is part of the top apps, otherwise logging -1. Also added a test case I missed for HashedStringCache and removing the test for hashing resulting in the same hashed string when called twice from the ChooserActivityTest since we now cover that in the HashedStringCacheTest. Bug: 130658734 Test: Added more test cases in ChooserActivityTest and did manual testing Change-Id: I0e34a6bf64114d94197f62b8219652c33c03a410 --- .../android/internal/app/ChooserActivity.java | 21 +++ .../android/util/HashedStringCacheTest.java | 11 ++ .../internal/app/ChooserActivityTest.java | 139 ++++++++++++++---- 3 files changed, 141 insertions(+), 30 deletions(-) diff --git a/core/java/com/android/internal/app/ChooserActivity.java b/core/java/com/android/internal/app/ChooserActivity.java index 54338bf6a1763..aac8fa46ce356 100644 --- a/core/java/com/android/internal/app/ChooserActivity.java +++ b/core/java/com/android/internal/app/ChooserActivity.java @@ -218,6 +218,8 @@ public class ChooserActivity extends ResolverActivity { private static final int SHORTCUT_MANAGER_SHARE_TARGET_RESULT_COMPLETED = 4; private static final int LIST_VIEW_UPDATE_MESSAGE = 5; + private static final int MAX_LOG_RANK_POSITION = 12; + @VisibleForTesting public static final int LIST_VIEW_UPDATE_INTERVAL_IN_MILLIS = 250; @@ -1014,6 +1016,7 @@ public class ChooserActivity extends ResolverActivity { // Lower values mean the ranking was better. int cat = 0; int value = which; + int directTargetAlsoRanked = -1; HashedStringCache.HashResult directTargetHashed = null; switch (mChooserListAdapter.getPositionTargetType(which)) { case ChooserListAdapter.TARGET_CALLER: @@ -1033,6 +1036,7 @@ public class ChooserActivity extends ResolverActivity { target.getComponentName().getPackageName() + target.getTitle().toString(), mMaxHashSaltDays); + directTargetAlsoRanked = getRankedPosition((SelectableTargetInfo) targetInfo); break; case ChooserListAdapter.TARGET_STANDARD: cat = MetricsEvent.ACTION_ACTIVITY_CHOOSER_PICKED_STANDARD_TARGET; @@ -1055,6 +1059,8 @@ public class ChooserActivity extends ResolverActivity { targetLogMaker.addTaggedData( MetricsEvent.FIELD_HASHED_TARGET_SALT_GEN, directTargetHashed.saltGeneration); + targetLogMaker.addTaggedData(MetricsEvent.FIELD_RANKED_POSITION, + directTargetAlsoRanked); } getMetricsLogger().write(targetLogMaker); MetricsLogger.action(this, cat, value); @@ -1073,6 +1079,21 @@ public class ChooserActivity extends ResolverActivity { } } + private int getRankedPosition(SelectableTargetInfo targetInfo) { + String targetPackageName = + targetInfo.getChooserTarget().getComponentName().getPackageName(); + int maxRankedResults = Math.min(mChooserListAdapter.mDisplayList.size(), + MAX_LOG_RANK_POSITION); + + for (int i = 0; i < maxRankedResults; i++) { + if (mChooserListAdapter.mDisplayList.get(i) + .getResolveInfo().activityInfo.packageName.equals(targetPackageName)) { + return i; + } + } + return -1; + } + void queryTargetServices(ChooserListAdapter adapter) { final PackageManager pm = getPackageManager(); ShortcutManager sm = (ShortcutManager) getSystemService(ShortcutManager.class); diff --git a/core/tests/coretests/src/android/util/HashedStringCacheTest.java b/core/tests/coretests/src/android/util/HashedStringCacheTest.java index 333db246d6371..229247353a528 100644 --- a/core/tests/coretests/src/android/util/HashedStringCacheTest.java +++ b/core/tests/coretests/src/android/util/HashedStringCacheTest.java @@ -80,6 +80,17 @@ public class HashedStringCacheTest { assertThat(cachedResult2.hashedString, is(cachedResult.hashedString)); } + + @Test + public void testThatMultipleInputResultInDifferentHash() { + HashedStringCache cache = HashedStringCache.getInstance(); + HashedStringCache.HashResult cachedResult = + cache.hashString(mContext, TAG, TEST_STRING, 7); + HashedStringCache.HashResult cachedResult2 = + cache.hashString(mContext, TAG, "different_test", 7); + assertThat(cachedResult2.hashedString, is(not(cachedResult.hashedString))); + } + @Test public void testThatZeroDaysResultsInNewHash() { HashedStringCache cache = HashedStringCache.getInstance(); diff --git a/core/tests/coretests/src/com/android/internal/app/ChooserActivityTest.java b/core/tests/coretests/src/com/android/internal/app/ChooserActivityTest.java index ac039dddb66c8..29e94e8b4bf6b 100644 --- a/core/tests/coretests/src/com/android/internal/app/ChooserActivityTest.java +++ b/core/tests/coretests/src/com/android/internal/app/ChooserActivityTest.java @@ -62,9 +62,6 @@ import com.android.internal.app.ResolverActivity.ResolvedComponentInfo; import com.android.internal.logging.MetricsLogger; import com.android.internal.logging.nano.MetricsProto.MetricsEvent; -import java.util.Arrays; -import java.util.Collection; -import java.util.function.Function; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -74,7 +71,10 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mockito; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; import java.util.List; +import java.util.function.Function; /** * Chooser activity instrumentation tests @@ -770,8 +770,6 @@ public class ChooserActivityTest { } // This test is too long and too slow and should not be taken as an example for future tests. - // This is necessary because it tests that multiple calls result in the same result but - // normally a test this long should be broken into smaller tests testing individual components. @Test public void testDirectTargetSelectionLogging() throws InterruptedException { Intent sendIntent = createSendTextIntent(); @@ -785,7 +783,7 @@ public class ChooserActivityTest { MetricsLogger mockLogger = sOverrides.metricsLogger; ArgumentCaptor logMakerCaptor = ArgumentCaptor.forClass(LogMaker.class); // Create direct share target - List serviceTargets = createDirectShareTargets(1); + List serviceTargets = createDirectShareTargets(1, ""); ResolveInfo ri = ResolverDataProvider.createResolveInfo(3, 0); // Start activity @@ -808,7 +806,7 @@ public class ChooserActivityTest { // TODO: restructure the tests b/129870719 Thread.sleep(ChooserActivity.LIST_VIEW_UPDATE_INTERVAL_IN_MILLIS); - assertThat("Chooser should have 3 targets (2apps, 1 direct)", + assertThat("Chooser should have 3 targets (2 apps, 1 direct)", activity.getAdapter().getCount(), is(3)); assertThat("Chooser should have exactly one selectable direct target", activity.getAdapter().getSelectableServiceTargetCount(), is(1)); @@ -832,20 +830,36 @@ public class ChooserActivityTest { .getAllValues().get(2).getTaggedData(MetricsEvent.FIELD_HASHED_TARGET_NAME); assertThat("Hash is not predictable but must be obfuscated", hashedName, is(not(name))); + assertThat("The packages shouldn't match for app target and direct target", logMakerCaptor + .getAllValues().get(2).getTaggedData(MetricsEvent.FIELD_RANKED_POSITION), is(-1)); + } - // Running the same again to check if the hashed name is the same as before. + // This test is too long and too slow and should not be taken as an example for future tests. + @Test + public void testDirectTargetLoggingWithRankedAppTarget() throws InterruptedException { + Intent sendIntent = createSendTextIntent(); + // We need app targets for direct targets to get displayed + List resolvedComponentInfos = createResolvedComponentsForTest(2); + when(sOverrides.resolverListController.getResolversForIntent(Mockito.anyBoolean(), + Mockito.anyBoolean(), + Mockito.isA(List.class))).thenReturn(resolvedComponentInfos); - Intent sendIntent2 = createSendTextIntent(); + // Set up resources + MetricsLogger mockLogger = sOverrides.metricsLogger; + ArgumentCaptor logMakerCaptor = ArgumentCaptor.forClass(LogMaker.class); + // Create direct share target + List serviceTargets = createDirectShareTargets(1, + resolvedComponentInfos.get(0).getResolveInfoAt(0).activityInfo.packageName); + ResolveInfo ri = ResolverDataProvider.createResolveInfo(3, 0); // Start activity - final ChooserWrapperActivity activity2 = mActivityRule - .launchActivity(Intent.createChooser(sendIntent2, null)); - waitForIdle(); + final ChooserWrapperActivity activity = mActivityRule + .launchActivity(Intent.createChooser(sendIntent, null)); // Insert the direct share target InstrumentationRegistry.getInstrumentation().runOnMainSync( - () -> activity2.getAdapter().addServiceResults( - activity2.createTestDisplayResolveInfo(sendIntent, + () -> activity.getAdapter().addServiceResults( + activity.createTestDisplayResolveInfo(sendIntent, ri, "testLabel", "testInfo", @@ -858,29 +872,89 @@ public class ChooserActivityTest { // TODO: restructure the tests b/129870719 Thread.sleep(ChooserActivity.LIST_VIEW_UPDATE_INTERVAL_IN_MILLIS); - assertThat("Chooser should have 3 targets (2apps, 1 direct)", - activity2.getAdapter().getCount(), is(3)); + assertThat("Chooser should have 3 targets (2 apps, 1 direct)", + activity.getAdapter().getCount(), is(3)); assertThat("Chooser should have exactly one selectable direct target", - activity2.getAdapter().getSelectableServiceTargetCount(), is(1)); + activity.getAdapter().getSelectableServiceTargetCount(), is(1)); assertThat("The resolver info must match the resolver info used to create the target", - activity2.getAdapter().getItem(0).getResolveInfo(), is(ri)); + activity.getAdapter().getItem(0).getResolveInfo(), is(ri)); // Click on the direct target + String name = serviceTargets.get(0).getTitle().toString(); onView(withText(name)) .perform(click()); waitForIdle(); - // Currently we're seeing 6 invocations (3 from above, doubled up) - // 4. ChooserActivity.onCreate() - // 5. ChooserActivity$ChooserRowAdapter.createContentPreviewView() - // 6. ChooserActivity.startSelected -- which is the one we're after - verify(mockLogger, Mockito.times(6)).write(logMakerCaptor.capture()); - assertThat(logMakerCaptor.getAllValues().get(5).getCategory(), + // Currently we're seeing 3 invocations + // 1. ChooserActivity.onCreate() + // 2. ChooserActivity$ChooserRowAdapter.createContentPreviewView() + // 3. ChooserActivity.startSelected -- which is the one we're after + verify(mockLogger, Mockito.times(3)).write(logMakerCaptor.capture()); + assertThat(logMakerCaptor.getAllValues().get(2).getCategory(), is(MetricsEvent.ACTION_ACTIVITY_CHOOSER_PICKED_SERVICE_TARGET)); - String hashedName2 = (String) logMakerCaptor - .getAllValues().get(5).getTaggedData(MetricsEvent.FIELD_HASHED_TARGET_NAME); - assertThat("Hashing the same name should result in the same hashed value", - hashedName2, is(hashedName)); + assertThat("The packages should match for app target and direct target", logMakerCaptor + .getAllValues().get(2).getTaggedData(MetricsEvent.FIELD_RANKED_POSITION), is(0)); + } + + // This test is too long and too slow and should not be taken as an example for future tests. + @Test + public void testDirectTargetLoggingWithAppTargetNotRanked() throws InterruptedException { + Intent sendIntent = createSendTextIntent(); + // We need app targets for direct targets to get displayed + List resolvedComponentInfos = createResolvedComponentsForTest(15); + when(sOverrides.resolverListController.getResolversForIntent(Mockito.anyBoolean(), + Mockito.anyBoolean(), + Mockito.isA(List.class))).thenReturn(resolvedComponentInfos); + + // Set up resources + MetricsLogger mockLogger = sOverrides.metricsLogger; + ArgumentCaptor logMakerCaptor = ArgumentCaptor.forClass(LogMaker.class); + // Create direct share target + List serviceTargets = createDirectShareTargets(1, + resolvedComponentInfos.get(14).getResolveInfoAt(0).activityInfo.packageName); + ResolveInfo ri = ResolverDataProvider.createResolveInfo(16, 0); + + // Start activity + final ChooserWrapperActivity activity = mActivityRule + .launchActivity(Intent.createChooser(sendIntent, null)); + // Insert the direct share target + InstrumentationRegistry.getInstrumentation().runOnMainSync( + () -> activity.getAdapter().addServiceResults( + activity.createTestDisplayResolveInfo(sendIntent, + ri, + "testLabel", + "testInfo", + sendIntent), + serviceTargets, + false) + ); + // Thread.sleep shouldn't be a thing in an integration test but it's + // necessary here because of the way the code is structured + // TODO: restructure the tests b/129870719 + Thread.sleep(ChooserActivity.LIST_VIEW_UPDATE_INTERVAL_IN_MILLIS); + + assertThat("Chooser should have 20 targets (4 apps, 1 direct, 15 A-Z)", + activity.getAdapter().getCount(), is(20)); + assertThat("Chooser should have exactly one selectable direct target", + activity.getAdapter().getSelectableServiceTargetCount(), is(1)); + assertThat("The resolver info must match the resolver info used to create the target", + activity.getAdapter().getItem(0).getResolveInfo(), is(ri)); + + // Click on the direct target + String name = serviceTargets.get(0).getTitle().toString(); + onView(withText(name)) + .perform(click()); + waitForIdle(); + + // Currently we're seeing 3 invocations + // 1. ChooserActivity.onCreate() + // 2. ChooserActivity$ChooserRowAdapter.createContentPreviewView() + // 3. ChooserActivity.startSelected -- which is the one we're after + verify(mockLogger, Mockito.times(3)).write(logMakerCaptor.capture()); + assertThat(logMakerCaptor.getAllValues().get(2).getCategory(), + is(MetricsEvent.ACTION_ACTIVITY_CHOOSER_PICKED_SERVICE_TARGET)); + assertThat("The packages shouldn't match for app target and direct target", logMakerCaptor + .getAllValues().get(2).getTaggedData(MetricsEvent.FIELD_RANKED_POSITION), is(-1)); } private Intent createSendTextIntent() { @@ -946,12 +1020,17 @@ public class ChooserActivityTest { return infoList; } - private List createDirectShareTargets(int numberOfResults) { + private List createDirectShareTargets(int numberOfResults, String packageName) { Icon icon = Icon.createWithBitmap(createBitmap()); String testTitle = "testTitle"; List targets = new ArrayList<>(); for (int i = 0; i < numberOfResults; i++) { - ComponentName componentName = ResolverDataProvider.createComponentName(i); + ComponentName componentName; + if (packageName.isEmpty()) { + componentName = ResolverDataProvider.createComponentName(i); + } else { + componentName = new ComponentName(packageName, packageName + ".class"); + } ChooserTarget tempTarget = new ChooserTarget( testTitle + i, icon,