Merge "fix flaky behavior in FakeLatencyTracker" into udc-dev am: 819b087abc am: 54e02794ad

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/22637208

Change-Id: I62218ab852abe03fd2db745e4ae48202a88088c5
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

core/java/com/android/internal/util/LatencyTracker.java

@@ -305,10 +305,17 @@ public class LatencyTracker {
     private final SparseArray<ActionProperties> mActionPropertiesMap = new SparseArray<>();
     @GuardedBy("mLock")
     private boolean mEnabled;
+    private final DeviceConfig.OnPropertiesChangedListener mOnPropertiesChangedListener =
+            this::updateProperties;
 
     // Wrapping this in a holder class achieves lazy loading behavior
     private static final class SLatencyTrackerHolder {
-        private static final LatencyTracker sLatencyTracker = new LatencyTracker();
+        private static final LatencyTracker sLatencyTracker;
+
+        static {
+            sLatencyTracker = new LatencyTracker();
+            sLatencyTracker.startListeningForLatencyTrackerConfigChanges();
+        }
     }
 
     public static LatencyTracker getInstance(Context context) {
@@ -319,31 +326,16 @@ public class LatencyTracker {
      * Constructor for LatencyTracker
      *
      * <p>This constructor is only visible for test classes to inject their own consumer callbacks
+     *
+     * @param startListeningForPropertyChanges If set, constructor will register for device config
+     *                      property updates prior to returning. If not set,
+     *                      {@link #startListeningForLatencyTrackerConfigChanges} must be called
+     *                      to start listening.
      */
     @RequiresPermission(Manifest.permission.READ_DEVICE_CONFIG)
     @VisibleForTesting
     public LatencyTracker() {
         mEnabled = DEFAULT_ENABLED;
-
-        final Context context = ActivityThread.currentApplication();
-        if (context != null
-                && context.checkCallingOrSelfPermission(READ_DEVICE_CONFIG) == PERMISSION_GRANTED) {
-            // Post initialization to the background in case we're running on the main thread.
-            BackgroundThread.getHandler().post(() -> this.updateProperties(
-                    DeviceConfig.getProperties(NAMESPACE_LATENCY_TRACKER)));
-            DeviceConfig.addOnPropertiesChangedListener(NAMESPACE_LATENCY_TRACKER,
-                    BackgroundThread.getExecutor(), this::updateProperties);
-        } else {
-            if (DEBUG) {
-                if (context == null) {
-                    Log.d(TAG, "No application for " + ActivityThread.currentActivityThread());
-                } else {
-                    Log.d(TAG, "Initialized the LatencyTracker."
-                            + " (No READ_DEVICE_CONFIG permission to change configs)"
-                            + " enabled=" + mEnabled + ", package=" + context.getPackageName());
-                }
-            }
-        }
     }
 
     private void updateProperties(DeviceConfig.Properties properties) {
@@ -365,6 +357,54 @@ public class LatencyTracker {
         }
     }
 
+    /**
+     * Test method to start listening to {@link DeviceConfig} properties changes.
+     *
+     * <p>During testing, a {@link LatencyTracker} it is desired to stop and start listening for
+     * config updates.
+     *
+     * <p>This is not used for production usages of this class outside of testing as we are
+     * using a single static object.
+     */
+    @VisibleForTesting
+    public void startListeningForLatencyTrackerConfigChanges() {
+        final Context context = ActivityThread.currentApplication();
+        if (context != null
+                && context.checkCallingOrSelfPermission(READ_DEVICE_CONFIG) == PERMISSION_GRANTED) {
+            // Post initialization to the background in case we're running on the main thread.
+            BackgroundThread.getHandler().post(() -> this.updateProperties(
+                    DeviceConfig.getProperties(NAMESPACE_LATENCY_TRACKER)));
+            DeviceConfig.addOnPropertiesChangedListener(NAMESPACE_LATENCY_TRACKER,
+                    BackgroundThread.getExecutor(), mOnPropertiesChangedListener);
+        } else {
+            if (DEBUG) {
+                if (context == null) {
+                    Log.d(TAG, "No application for " + ActivityThread.currentActivityThread());
+                } else {
+                    synchronized (mLock) {
+                        Log.d(TAG, "Initialized the LatencyTracker."
+                                + " (No READ_DEVICE_CONFIG permission to change configs)"
+                                + " enabled=" + mEnabled + ", package=" + context.getPackageName());
+                    }
+                }
+            }
+        }
+    }
+
+    /**
+     * Test method to stop listening to {@link DeviceConfig} properties changes.
+     *
+     * <p>During testing, a {@link LatencyTracker} it is desired to stop and start listening for
+     * config updates.
+     *
+     * <p>This is not used for production usages of this class outside of testing as we are
+     * using a single static object.
+     */
+    @VisibleForTesting
+    public void stopListeningForLatencyTrackerConfigChanges() {
+        DeviceConfig.removeOnPropertiesChangedListener(mOnPropertiesChangedListener);
+    }
+
     /**
      * A helper method to translate action type to name.
      *

core/tests/coretests/src/com/android/internal/util/LatencyTrackerTest.java

@@ -28,12 +28,12 @@ import static com.google.common.truth.Truth.assertWithMessage;
 import android.provider.DeviceConfig;
 
 import androidx.test.ext.junit.runners.AndroidJUnit4;
-import androidx.test.filters.SmallTest;
 
 import com.android.internal.util.LatencyTracker.ActionProperties;
 
 import com.google.common.truth.Expect;
 
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -48,7 +48,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
 
-@SmallTest
 @RunWith(AndroidJUnit4.class)
 public class LatencyTrackerTest {
     private static final String ENUM_NAME_PREFIX = "UIACTION_LATENCY_REPORTED__ACTION__";
@@ -65,6 +64,11 @@ public class LatencyTrackerTest {
         mLatencyTracker = FakeLatencyTracker.create();
     }
 
+    @After
+    public void tearDown() {
+        mLatencyTracker.stopListeningForLatencyTrackerConfigChanges();
+    }
+
     @Test
     public void testCujsMapToEnumsCorrectly() {
         List<Field> actions = getAllActionFields();

core/tests/coretests/testdoubles/src/com/android/internal/util/FakeLatencyTracker.java

@@ -25,8 +25,6 @@ import android.provider.DeviceConfig;
 import android.util.Log;
 import android.util.SparseArray;
 
-import androidx.annotation.Nullable;
-
 import com.android.internal.annotations.GuardedBy;
 
 import com.google.common.collect.ImmutableMap;
@@ -51,15 +49,17 @@ public final class FakeLatencyTracker extends LatencyTracker {
     private final List<String> mPerfettoTraceNamesTriggered;
     private final AtomicReference<SparseArray<ActionProperties>> mLastPropertiesUpdate =
             new AtomicReference<>();
-    @Nullable
+    private final AtomicReference<Callable<Boolean>> mShouldClosePropertiesUpdatedCallable =
-    @GuardedBy("mLock")
+            new AtomicReference<>();
-    private Callable<Boolean> mShouldClosePropertiesUpdatedCallable = null;
     private final ConditionVariable mDeviceConfigPropertiesUpdated = new ConditionVariable();
 
     public static FakeLatencyTracker create() throws Exception {
         Log.i(TAG, "create");
         disableForAllActions();
+        Log.i(TAG, "done disabling all actions");
         FakeLatencyTracker fakeLatencyTracker = new FakeLatencyTracker();
+        Log.i(TAG, "done creating tracker object");
+        fakeLatencyTracker.startListeningForLatencyTrackerConfigChanges();
         // always return the fake in the disabled state and let the client control the desired state
         fakeLatencyTracker.waitForGlobalEnabledState(false);
         fakeLatencyTracker.waitForAllPropertiesEnableState(false);
@@ -131,17 +131,16 @@ public final class FakeLatencyTracker extends LatencyTracker {
     @Override
     public void onDeviceConfigPropertiesUpdated(SparseArray<ActionProperties> actionProperties) {
         Log.d(TAG, "onDeviceConfigPropertiesUpdated: " + actionProperties);
+
         mLastPropertiesUpdate.set(actionProperties);
-        synchronized (mLock) {
+        Callable<Boolean> shouldClosePropertiesUpdated =
-            if (mShouldClosePropertiesUpdatedCallable != null) {
+                mShouldClosePropertiesUpdatedCallable.get();
+        if (shouldClosePropertiesUpdated != null) {
             try {
-                    boolean shouldClosePropertiesUpdated =
+                boolean result = shouldClosePropertiesUpdated.call();
-                            mShouldClosePropertiesUpdatedCallable.call();
+                Log.i(TAG, "shouldClosePropertiesUpdatedCallable callable result=" + result);
-                    Log.i(TAG, "shouldClosePropertiesUpdatedCallable callable result="
+                if (result) {
-                            + shouldClosePropertiesUpdated);
+                    mShouldClosePropertiesUpdatedCallable.set(null);
-                    if (shouldClosePropertiesUpdated) {
-                        Log.i(TAG, "shouldClosePropertiesUpdatedCallable=true, opening condition");
-                        mShouldClosePropertiesUpdatedCallable = null;
                     mDeviceConfigPropertiesUpdated.open();
                 }
             } catch (Exception e) {
@@ -153,7 +152,6 @@ public final class FakeLatencyTracker extends LatencyTracker {
             mDeviceConfigPropertiesUpdated.open();
         }
     }
-    }
 
     @Override
     public void onTriggerPerfetto(String triggerName) {
@@ -175,13 +173,10 @@ public final class FakeLatencyTracker extends LatencyTracker {
 
     public void waitForAllPropertiesEnableState(boolean enabledState) throws Exception {
         Log.i(TAG, "waitForAllPropertiesEnableState: enabledState=" + enabledState);
-        synchronized (mLock) {
-            Log.i(TAG, "closing condition");
-            mDeviceConfigPropertiesUpdated.close();
         // Update the callable to only close the properties updated condition when all the
         // desired properties have been updated. The DeviceConfig callbacks may happen multiple
         // times so testing the resulting updates is required.
-            mShouldClosePropertiesUpdatedCallable = () -> {
+        waitForPropertiesCondition(() -> {
             Log.i(TAG, "verifying if last properties update has all properties enable="
                     + enabledState);
             SparseArray<ActionProperties> newProperties = mLastPropertiesUpdate.get();
@@ -193,25 +188,16 @@ public final class FakeLatencyTracker extends LatencyTracker {
                 }
             }
             return true;
-            };
+        });
-            if (mShouldClosePropertiesUpdatedCallable.call()) {
-                return;
-            }
-        }
-        Log.i(TAG, "waiting for condition");
-        mDeviceConfigPropertiesUpdated.block();
     }
 
     public void waitForMatchingActionProperties(ActionProperties actionProperties)
             throws Exception {
         Log.i(TAG, "waitForMatchingActionProperties: actionProperties=" + actionProperties);
-        synchronized (mLock) {
-            Log.i(TAG, "closing condition");
-            mDeviceConfigPropertiesUpdated.close();
         // Update the callable to only close the properties updated condition when all the
         // desired properties have been updated. The DeviceConfig callbacks may happen multiple
         // times so testing the resulting updates is required.
-            mShouldClosePropertiesUpdatedCallable = () -> {
+        waitForPropertiesCondition(() -> {
             Log.i(TAG, "verifying if last properties update contains matching property ="
                     + actionProperties);
             SparseArray<ActionProperties> newProperties = mLastPropertiesUpdate.get();
@@ -222,25 +208,16 @@ public final class FakeLatencyTracker extends LatencyTracker {
                 }
             }
             return false;
-            };
+        });
-            if (mShouldClosePropertiesUpdatedCallable.call()) {
-                return;
-            }
-        }
-        Log.i(TAG, "waiting for condition");
-        mDeviceConfigPropertiesUpdated.block();
     }
 
     public void waitForActionEnabledState(int action, boolean enabledState) throws Exception {
         Log.i(TAG, "waitForActionEnabledState:"
                 + " action=" + action + ", enabledState=" + enabledState);
-        synchronized (mLock) {
-            Log.i(TAG, "closing condition");
-            mDeviceConfigPropertiesUpdated.close();
         // Update the callable to only close the properties updated condition when all the
         // desired properties have been updated. The DeviceConfig callbacks may happen multiple
         // times so testing the resulting updates is required.
-            mShouldClosePropertiesUpdatedCallable = () -> {
+        waitForPropertiesCondition(() -> {
             Log.i(TAG, "verifying if last properties update contains action=" + action
                     + ", enabledState=" + enabledState);
             SparseArray<ActionProperties> newProperties = mLastPropertiesUpdate.get();
@@ -250,32 +227,28 @@ public final class FakeLatencyTracker extends LatencyTracker {
                 }
             }
             return false;
-            };
+        });
-            if (mShouldClosePropertiesUpdatedCallable.call()) {
-                return;
-            }
-        }
-        Log.i(TAG, "waiting for condition");
-        mDeviceConfigPropertiesUpdated.block();
     }
 
     public void waitForGlobalEnabledState(boolean enabledState) throws Exception {
         Log.i(TAG, "waitForGlobalEnabledState: enabledState=" + enabledState);
-        synchronized (mLock) {
-            Log.i(TAG, "closing condition");
-            mDeviceConfigPropertiesUpdated.close();
         // Update the callable to only close the properties updated condition when all the
         // desired properties have been updated. The DeviceConfig callbacks may happen multiple
         // times so testing the resulting updates is required.
-            mShouldClosePropertiesUpdatedCallable = () -> {
+        waitForPropertiesCondition(() -> {
             //noinspection deprecation
             return isEnabled() == enabledState;
-            };
+        });
-            if (mShouldClosePropertiesUpdatedCallable.call()) {
-                return;
     }
-        }
+
-        Log.i(TAG, "waiting for condition");
+    public void waitForPropertiesCondition(Callable<Boolean> shouldClosePropertiesUpdatedCallable)
+            throws Exception {
+        mShouldClosePropertiesUpdatedCallable.set(shouldClosePropertiesUpdatedCallable);
+        mDeviceConfigPropertiesUpdated.close();
+        if (!shouldClosePropertiesUpdatedCallable.call()) {
+            Log.i(TAG, "waiting for mDeviceConfigPropertiesUpdated condition");
             mDeviceConfigPropertiesUpdated.block();
         }
+        Log.i(TAG, "waitForPropertiesCondition: returning");
+    }
 }

services/tests/voiceinteractiontests/src/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareLoggingLatencyTest.java

@@ -38,7 +38,6 @@ import android.os.BatteryStatsInternal;
 import android.os.Process;
 import android.os.RemoteException;
 
-import androidx.test.filters.FlakyTest;
 import androidx.test.platform.app.InstrumentationRegistry;
 
 import com.android.internal.util.FakeLatencyTracker;
@@ -92,12 +91,10 @@ public class SoundTriggerMiddlewareLoggingLatencyTest {
     }
 
     @Test
-    @FlakyTest(bugId = 275113847)
     public void testSetUpAndTearDown() {
     }
 
     @Test
-    @FlakyTest(bugId = 275113847)
     public void testOnPhraseRecognitionStartsLatencyTrackerWithSuccessfulPhraseIdTrigger()
             throws RemoteException {
         ArgumentCaptor<ISoundTriggerCallback> soundTriggerCallbackCaptor = ArgumentCaptor.forClass(
@@ -114,7 +111,6 @@ public class SoundTriggerMiddlewareLoggingLatencyTest {
     }
 
     @Test
-    @FlakyTest(bugId = 275113847)
     public void testOnPhraseRecognitionRestartsActiveSession() throws RemoteException {
         ArgumentCaptor<ISoundTriggerCallback> soundTriggerCallbackCaptor = ArgumentCaptor.forClass(
                 ISoundTriggerCallback.class);
@@ -135,7 +131,6 @@ public class SoundTriggerMiddlewareLoggingLatencyTest {
     }
 
     @Test
-    @FlakyTest(bugId = 275113847)
     public void testOnPhraseRecognitionNeverStartsLatencyTrackerWithNonSuccessEvent()
             throws RemoteException {
         ArgumentCaptor<ISoundTriggerCallback> soundTriggerCallbackCaptor = ArgumentCaptor.forClass(
@@ -153,7 +148,6 @@ public class SoundTriggerMiddlewareLoggingLatencyTest {
     }
 
     @Test
-    @FlakyTest(bugId = 275113847)
     public void testOnPhraseRecognitionNeverStartsLatencyTrackerWithNoKeyphraseId()
             throws RemoteException {
         ArgumentCaptor<ISoundTriggerCallback> soundTriggerCallbackCaptor = ArgumentCaptor.forClass(