Refactor CountryDetectorService

Clean-up the test to use Mockito instead of the deprecated
AndroidTestCase, also getting rid of 1.5s of Thread.sleep.
No logic change.

Bug: 141626568
Test: atest CountryDetectorServiceTest
Change-Id: Ic0782b1ffcd1dcb30b9f1d91b37521d1f9887f03

services/core/java/com/android/server/CountryDetectorService.java

@@ -16,14 +16,6 @@
 
 package com.android.server;
 
-import java.io.FileDescriptor;
-import java.io.PrintWriter;
-import java.util.HashMap;
-
-import com.android.internal.os.BackgroundThread;
-import com.android.internal.util.DumpUtils;
-import com.android.server.location.ComprehensiveCountryDetector;
-
 import android.content.Context;
 import android.location.Country;
 import android.location.CountryListener;
@@ -36,17 +28,25 @@ import android.util.PrintWriterPrinter;
 import android.util.Printer;
 import android.util.Slog;
 
+import com.android.internal.annotations.VisibleForTesting;
+import com.android.internal.os.BackgroundThread;
+import com.android.internal.util.DumpUtils;
+import com.android.server.location.ComprehensiveCountryDetector;
+
+import java.io.FileDescriptor;
+import java.io.PrintWriter;
+import java.util.HashMap;
+
 /**
- * This class detects the country that the user is in through
+ * This class detects the country that the user is in through {@link ComprehensiveCountryDetector}.
- * {@link ComprehensiveCountryDetector}.
  *
  * @hide
  */
-public class CountryDetectorService extends ICountryDetector.Stub implements Runnable {
+public class CountryDetectorService extends ICountryDetector.Stub {
 
     /**
-     * The class represents the remote listener, it will also removes itself
+     * The class represents the remote listener, it will also removes itself from listener list when
-     * from listener list when the remote process was died.
+     * the remote process was died.
      */
     private final class Receiver implements IBinder.DeathRecipient {
         private final ICountryListener mListener;
@@ -79,9 +79,11 @@ public class CountryDetectorService extends ICountryDetector.Stub implements Run
         }
     }
 
-    private final static String TAG = "CountryDetector";
+    private static final String TAG = "CountryDetector";
 
-    /** Whether to dump the state of the country detector service to bugreports */
+    /**
+     * Whether to dump the state of the country detector service to bugreports
+     */
     private static final boolean DEBUG = false;
 
     private final HashMap<IBinder, Receiver> mReceivers;
@@ -92,15 +94,21 @@ public class CountryDetectorService extends ICountryDetector.Stub implements Run
     private CountryListener mLocationBasedDetectorListener;
 
     public CountryDetectorService(Context context) {
+        this(context, BackgroundThread.getHandler());
+    }
+
+    @VisibleForTesting
+    CountryDetectorService(Context context, Handler handler) {
         super();
-        mReceivers = new HashMap<IBinder, Receiver>();
+        mReceivers = new HashMap<>();
         mContext = context;
+        mHandler = handler;
     }
 
     @Override
     public Country detectCountry() {
         if (!mSystemReady) {
-            return null;   // server not yet active
+            return null; // server not yet active
         } else {
             return mCountryDetector.detectCountry();
         }
@@ -154,9 +162,8 @@ public class CountryDetectorService extends ICountryDetector.Stub implements Run
         }
     }
 
-
     protected void notifyReceivers(Country country) {
-        synchronized(mReceivers) {
+        synchronized (mReceivers) {
             for (Receiver receiver : mReceivers.values()) {
                 try {
                     receiver.getListener().onCountryDetected(country);
@@ -170,38 +177,23 @@ public class CountryDetectorService extends ICountryDetector.Stub implements Run
 
     void systemRunning() {
         // Shall we wait for the initialization finish.
-        BackgroundThread.getHandler().post(this);
+        mHandler.post(
+                () -> {
+                    initialize();
+                    mSystemReady = true;
+                });
     }
 
     private void initialize() {
         mCountryDetector = new ComprehensiveCountryDetector(mContext);
-        mLocationBasedDetectorListener = new CountryListener() {
+        mLocationBasedDetectorListener = country -> mHandler.post(() -> notifyReceivers(country));
-            public void onCountryDetected(final Country country) {
-                mHandler.post(new Runnable() {
-                    public void run() {
-                        notifyReceivers(country);
-                    }
-                });
-            }
-        };
-    }
-
-    public void run() {
-        mHandler = new Handler();
-        initialize();
-        mSystemReady = true;
     }
 
     protected void setCountryListener(final CountryListener listener) {
-        mHandler.post(new Runnable() {
+        mHandler.post(() -> mCountryDetector.setCountryListener(listener));
-            @Override
-            public void run() {
-                mCountryDetector.setCountryListener(listener);
-            }
-        });
     }
 
-    // For testing
+    @VisibleForTesting
     boolean isSystemReady() {
         return mSystemReady;
     }

services/tests/servicestests/Android.bp

@@ -28,6 +28,7 @@ android_test {
         "services.net",
         "services.usage",
         "guava",
+        "androidx.test.core",
         "androidx.test.runner",
         "androidx.test.rules",
         "mockito-target-minus-junit4",

services/tests/servicestests/src/com/android/server/CountryDetectorServiceTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010 The Android Open Source Project
+ * Copyright (C) 2019 The Android Open Source Project
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -11,28 +11,48 @@
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
- * limitations under the License
+ * limitations under the License.
  */
 
 package com.android.server;
 
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doAnswer;
+
 import android.content.Context;
 import android.location.Country;
 import android.location.CountryListener;
 import android.location.ICountryListener;
+import android.os.Handler;
+import android.os.Looper;
+import android.os.Message;
 import android.os.RemoteException;
-import android.test.AndroidTestCase;
 
-public class CountryDetectorServiceTest extends AndroidTestCase {
+import androidx.test.core.app.ApplicationProvider;
-    private class CountryListenerTester extends ICountryListener.Stub {
+
+import com.google.common.truth.Expect;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class CountryDetectorServiceTest {
+
+    private static class CountryListenerTester extends ICountryListener.Stub {
         private Country mCountry;
 
         @Override
-        public void onCountryDetected(Country country) throws RemoteException {
+        public void onCountryDetected(Country country) {
             mCountry = country;
         }
 
-        public Country getCountry() {
+        Country getCountry() {
             return mCountry;
         }
 
@@ -41,12 +61,11 @@ public class CountryDetectorServiceTest extends AndroidTestCase {
         }
     }
 
-    private class CountryDetectorServiceTester extends CountryDetectorService {
+    private static class CountryDetectorServiceTester extends CountryDetectorService {
-
         private CountryListener mListener;
 
-        public CountryDetectorServiceTester(Context context) {
+        CountryDetectorServiceTester(Context context, Handler handler) {
-            super(context);
+            super(context, handler);
         }
 
         @Override
@@ -59,51 +78,77 @@ public class CountryDetectorServiceTest extends AndroidTestCase {
             mListener = listener;
         }
 
-        public boolean isListenerSet() {
+        boolean isListenerSet() {
             return mListener != null;
         }
     }
 
-    public void testAddRemoveListener() throws RemoteException {
+    @Rule
-        CountryDetectorServiceTester serviceTester = new CountryDetectorServiceTester(getContext());
+    public final Expect expect = Expect.create();
-        serviceTester.systemRunning();
+    @Spy
-        waitForSystemReady(serviceTester);
+    private Context mContext = ApplicationProvider.getApplicationContext();
-        CountryListenerTester listenerTester = new CountryListenerTester();
+    @Spy
-        serviceTester.addCountryListener(listenerTester);
+    private Handler mHandler = new Handler(Looper.myLooper());
-        assertTrue(serviceTester.isListenerSet());
+    private CountryDetectorServiceTester mCountryDetectorService;
-        serviceTester.removeCountryListener(listenerTester);
-        assertFalse(serviceTester.isListenerSet());
-    }
 
-    public void testNotifyListeners() throws RemoteException {
+    @BeforeClass
-        CountryDetectorServiceTester serviceTester = new CountryDetectorServiceTester(getContext());
+    public static void oneTimeInitialization() {
-        CountryListenerTester listenerTesterA = new CountryListenerTester();
+        if (Looper.myLooper() == null) {
-        CountryListenerTester listenerTesterB = new CountryListenerTester();
+            Looper.prepare();
-        Country country = new Country("US", Country.COUNTRY_SOURCE_NETWORK);
-        serviceTester.systemRunning();
-        waitForSystemReady(serviceTester);
-        serviceTester.addCountryListener(listenerTesterA);
-        serviceTester.addCountryListener(listenerTesterB);
-        serviceTester.notifyReceivers(country);
-        assertTrue(serviceTester.isListenerSet());
-        assertTrue(listenerTesterA.isNotified());
-        assertTrue(listenerTesterB.isNotified());
-        serviceTester.removeCountryListener(listenerTesterA);
-        serviceTester.removeCountryListener(listenerTesterB);
-        assertFalse(serviceTester.isListenerSet());
-    }
-
-    private void waitForSystemReady(CountryDetectorService service) {
-        int count = 5;
-        while (count-- > 0) {
-            try {
-                Thread.sleep(500);
-            } catch (Exception e) {
-            }
-            if (service.isSystemReady()) {
-                return;
-            }
         }
-        throw new RuntimeException("Wait System Ready timeout");
+    }
+
+    @Before
+    public void setUp() {
+        mCountryDetectorService = new CountryDetectorServiceTester(mContext, mHandler);
+
+        // Immediately invoke run on the Runnable posted to the handler
+        doAnswer(invocation -> {
+            Message message = invocation.getArgument(0);
+            message.getCallback().run();
+            return true;
+        }).when(mHandler).sendMessageAtTime(any(Message.class), anyLong());
+    }
+
+    @Test
+    public void countryListener_add_successful() throws RemoteException {
+        CountryListenerTester countryListener = new CountryListenerTester();
+
+        mCountryDetectorService.systemRunning();
+        expect.that(mCountryDetectorService.isListenerSet()).isFalse();
+        mCountryDetectorService.addCountryListener(countryListener);
+
+        expect.that(mCountryDetectorService.isListenerSet()).isTrue();
+    }
+
+    @Test
+    public void countryListener_remove_successful() throws RemoteException {
+        CountryListenerTester countryListener = new CountryListenerTester();
+
+        mCountryDetectorService.systemRunning();
+        mCountryDetectorService.addCountryListener(countryListener);
+        expect.that(mCountryDetectorService.isListenerSet()).isTrue();
+        mCountryDetectorService.removeCountryListener(countryListener);
+
+        expect.that(mCountryDetectorService.isListenerSet()).isFalse();
+    }
+
+    @Test
+    public void countryListener_notify_successful() throws RemoteException {
+        CountryListenerTester countryListenerA = new CountryListenerTester();
+        CountryListenerTester countryListenerB = new CountryListenerTester();
+        Country country = new Country("US", Country.COUNTRY_SOURCE_NETWORK);
+
+        mCountryDetectorService.systemRunning();
+        mCountryDetectorService.addCountryListener(countryListenerA);
+        mCountryDetectorService.addCountryListener(countryListenerB);
+        expect.that(countryListenerA.isNotified()).isFalse();
+        expect.that(countryListenerB.isNotified()).isFalse();
+        mCountryDetectorService.notifyReceivers(country);
+
+        expect.that(countryListenerA.isNotified()).isTrue();
+        expect.that(countryListenerB.isNotified()).isTrue();
+        expect.that(countryListenerA.getCountry().equalsIgnoreSource(country)).isTrue();
+        expect.that(countryListenerB.getCountry().equalsIgnoreSource(country)).isTrue();
     }
 }