ConnectivityServiceTest: fix flakyness

This patch attempts to fix the remaining spurious failures in
ConnectivityServiceTest, which have two causes:
 - waitForIdle() does not take into account the NetworkAgents handlers.
 - the deadlines in testRequestBenchmark are sometimes exceeded.

To fix the first issue, waitForIdle() is moved to a test level instance
method and also calls waitForIdleHandler on any non null
MockNetworkAgent. This is expected to fix spurious errors for the
following tests:
  - testMobildeDataAlwaysOn
  - testLingering
  - testPacketKeepAlive
  - testMMSonWiFi

To fix the second issue, the deadlines for testRequestBenchmark are
extended by 10ms. Also, the failure message is made more actionable by
providing the total time it took for the operation, instead of printing
the number of dispatches that were achieved before the deadline.

Bug: 32561414
Test: tests pass many times in a row (~500).
Change-Id: Id33c6ac1edfb0b89634fa7789dccb2da237e2709

tests/net/java/com/android/server/ConnectivityServiceTest.java

@@ -115,7 +115,7 @@ import java.util.function.BooleanSupplier;
  * Tests for {@link ConnectivityService}.
  *
  * Build, install and run with:
- *  runtest frameworks-services -c com.android.server.ConnectivityServiceTest
+ *  runtest frameworks-net -c com.android.server.ConnectivityServiceTest
  */
 public class ConnectivityServiceTest extends AndroidTestCase {
     private static final String TAG = "ConnectivityServiceTest";
@@ -223,13 +223,32 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         }
     }
 
+    public void waitForIdle(int timeoutMs) {
+        waitForIdleHandler(mService.mHandlerThread, timeoutMs);
+        waitForIdle(mCellNetworkAgent, timeoutMs);
+        waitForIdle(mWiFiNetworkAgent, timeoutMs);
+        waitForIdle(mEthernetNetworkAgent, timeoutMs);
+        waitForIdleHandler(mService.mHandlerThread, timeoutMs);
+    }
+
+    public void waitForIdle(MockNetworkAgent agent, int timeoutMs) {
+        if (agent == null) {
+            return;
+        }
+        waitForIdleHandler(agent.mHandlerThread, timeoutMs);
+    }
+
+    private void waitForIdle() {
+        waitForIdle(TIMEOUT_MS);
+    }
+
     @SmallTest
     public void testWaitForIdle() {
         final int attempts = 50;  // Causes the test to take about 200ms on bullhead-eng.
 
         // Tests that waitForIdle returns immediately if the service is already idle.
         for (int i = 0; i < attempts; i++) {
-            mService.waitForIdle();
+            waitForIdle();
         }
 
         // Bring up a network that we can use to send messages to ConnectivityService.
@@ -243,7 +262,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         // Tests that calling waitForIdle waits for messages to be processed.
         for (int i = 0; i < attempts; i++) {
             mWiFiNetworkAgent.setSignalStrength(i);
-            mService.waitForIdle();
+            waitForIdle();
             assertEquals(i, mCm.getNetworkCapabilities(n).getSignalStrength());
         }
     }
@@ -344,18 +363,10 @@ public class ConnectivityServiceTest extends AndroidTestCase {
             };
             // Waits for the NetworkAgent to be registered, which includes the creation of the
             // NetworkMonitor.
-            mService.waitForIdle();
+            waitForIdle();
             mWrappedNetworkMonitor = mService.getLastCreatedWrappedNetworkMonitor();
         }
 
-        public void waitForIdle(int timeoutMs) {
-            waitForIdleHandler(mHandlerThread, timeoutMs);
-        }
-
-        public void waitForIdle() {
-            waitForIdle(TIMEOUT_MS);
-        }
-
         public void adjustScore(int change) {
             mScore += change;
             mNetworkAgent.sendNetworkScore(mScore);
@@ -802,13 +813,22 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 
     public void tearDown() throws Exception {
         setMobileDataAlwaysOn(false);
-        if (mCellNetworkAgent != null) { mCellNetworkAgent.disconnect(); }
+        if (mCellNetworkAgent != null) {
-        if (mWiFiNetworkAgent != null) { mWiFiNetworkAgent.disconnect(); }
+            mCellNetworkAgent.disconnect();
-        mCellNetworkAgent = mWiFiNetworkAgent = null;
+            mCellNetworkAgent = null;
+        }
+        if (mWiFiNetworkAgent != null) {
+            mWiFiNetworkAgent.disconnect();
+            mWiFiNetworkAgent = null;
+        }
+        if (mEthernetNetworkAgent != null) {
+            mEthernetNetworkAgent.disconnect();
+            mEthernetNetworkAgent = null;
+        }
         super.tearDown();
     }
 
-    private int transportToLegacyType(int transport) {
+    private static int transportToLegacyType(int transport) {
         switch (transport) {
             case TRANSPORT_ETHERNET:
                 return TYPE_ETHERNET;
@@ -840,7 +860,8 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         }
         // Test getNetworkInfo(Network)
         assertNotNull(mCm.getNetworkInfo(mCm.getActiveNetwork()));
-        assertEquals(transportToLegacyType(transport), mCm.getNetworkInfo(mCm.getActiveNetwork()).getType());
+        assertEquals(transportToLegacyType(transport),
+                mCm.getNetworkInfo(mCm.getActiveNetwork()).getType());
         // Test getNetworkCapabilities(Network)
         assertNotNull(mCm.getNetworkCapabilities(mCm.getActiveNetwork()));
         assertTrue(mCm.getNetworkCapabilities(mCm.getActiveNetwork()).hasTransport(transport));
@@ -911,7 +932,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
                 mCm.getAllNetworks()[1].equals(mCellNetworkAgent.getNetwork()));
         // Test cellular linger timeout.
         waitFor(mCellNetworkAgent.getDisconnectedCV());
-        mService.waitForIdle();
+        waitForIdle();
         assertEquals(1, mCm.getAllNetworks().length);
         verifyActiveNetwork(TRANSPORT_WIFI);
         assertEquals(1, mCm.getAllNetworks().length);
@@ -934,11 +955,11 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         // Test bringing up unvalidated cellular
         mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
         mCellNetworkAgent.connect(false);
-        mService.waitForIdle();
+        waitForIdle();
         verifyActiveNetwork(TRANSPORT_WIFI);
         // Test cellular disconnect.
         mCellNetworkAgent.disconnect();
-        mService.waitForIdle();
+        waitForIdle();
         verifyActiveNetwork(TRANSPORT_WIFI);
         // Test bringing up validated cellular
         mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
@@ -1290,7 +1311,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         }
 
         void assertNoCallback() {
-            mService.waitForIdle();
+            waitForIdle();
             CallbackInfo c = mCallbacks.peek();
             assertNull("Unexpected callback: " + c, c);
         }
@@ -1331,7 +1352,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 
         // This should not trigger spurious onAvailable() callbacks, b/21762680.
         mCellNetworkAgent.adjustScore(-1);
-        mService.waitForIdle();
+        waitForIdle();
         assertNoCallbacks(genericNetworkCallback, wifiNetworkCallback, cellNetworkCallback);
         assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
 
@@ -1369,7 +1390,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 
         // This should not trigger spurious onAvailable() callbacks, b/21762680.
         mCellNetworkAgent.adjustScore(-1);
-        mService.waitForIdle();
+        waitForIdle();
         assertNoCallbacks(genericNetworkCallback, wifiNetworkCallback, cellNetworkCallback);
         assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
 
@@ -1481,7 +1502,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         defaultCallback.expectCallback(CallbackState.LOST, mCellNetworkAgent);
 
         mCm.unregisterNetworkCallback(callback);
-        mService.waitForIdle();
+        waitForIdle();
 
         // Check that a network is only lingered or torn down if it would not satisfy a request even
         // if it validated.
@@ -1842,26 +1863,30 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         ConditionVariable cv = mCellNetworkAgent.getDisconnectedCV();
         mCellNetworkAgent.connectWithoutInternet();
         waitFor(cv);
-        mService.waitForIdle();
+        waitForIdle();
         assertEquals(0, mCm.getAllNetworks().length);
         verifyNoNetwork();
+
         // Test bringing up validated WiFi.
         mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
         cv = waitForConnectivityBroadcasts(1);
         mWiFiNetworkAgent.connect(true);
         waitFor(cv);
         verifyActiveNetwork(TRANSPORT_WIFI);
+
         // Register MMS NetworkRequest
         NetworkRequest.Builder builder = new NetworkRequest.Builder();
         builder.addCapability(NetworkCapabilities.NET_CAPABILITY_MMS);
         final TestNetworkCallback networkCallback = new TestNetworkCallback();
         mCm.requestNetwork(builder.build(), networkCallback);
+
         // Test bringing up unvalidated cellular with MMS
         mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
         mCellNetworkAgent.addCapability(NET_CAPABILITY_MMS);
         mCellNetworkAgent.connectWithoutInternet();
         networkCallback.expectAvailableCallbacks(mCellNetworkAgent);
         verifyActiveNetwork(TRANSPORT_WIFI);
+
         // Test releasing NetworkRequest disconnects cellular with MMS
         cv = mCellNetworkAgent.getDisconnectedCV();
         mCm.unregisterNetworkCallback(networkCallback);
@@ -1877,17 +1902,20 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         mCellNetworkAgent.connect(false);
         waitFor(cv);
         verifyActiveNetwork(TRANSPORT_CELLULAR);
+
         // Register MMS NetworkRequest
         NetworkRequest.Builder builder = new NetworkRequest.Builder();
         builder.addCapability(NetworkCapabilities.NET_CAPABILITY_MMS);
         final TestNetworkCallback networkCallback = new TestNetworkCallback();
         mCm.requestNetwork(builder.build(), networkCallback);
+
         // Test bringing up MMS cellular network
         MockNetworkAgent mmsNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
         mmsNetworkAgent.addCapability(NET_CAPABILITY_MMS);
         mmsNetworkAgent.connectWithoutInternet();
         networkCallback.expectAvailableCallbacks(mmsNetworkAgent);
         verifyActiveNetwork(TRANSPORT_CELLULAR);
+
         // Test releasing MMS NetworkRequest does not disconnect main cellular NetworkAgent
         cv = mmsNetworkAgent.getDisconnectedCV();
         mCm.unregisterNetworkCallback(networkCallback);
@@ -2294,7 +2322,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         ContentResolver cr = mServiceContext.getContentResolver();
         Settings.Global.putInt(cr, Settings.Global.MOBILE_DATA_ALWAYS_ON, enable ? 1 : 0);
         mService.updateMobileDataAlwaysOn();
-        mService.waitForIdle();
+        waitForIdle();
     }
 
     private boolean isForegroundNetwork(MockNetworkAgent network) {
@@ -2336,7 +2364,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         assertTrue(isForegroundNetwork(mWiFiNetworkAgent));
 
         // When lingering is complete, cell is still there but is now in the background.
-        mService.waitForIdle();
+        waitForIdle();
         int timeoutMs = TEST_LINGER_DELAY_MS + TEST_LINGER_DELAY_MS / 4;
         fgCallback.expectCallback(CallbackState.LOST, mCellNetworkAgent, timeoutMs);
         // Expect a network capabilities update sans FOREGROUND.
@@ -2392,6 +2420,11 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         //    and NUM_REQUESTS onAvailable callbacks to fire.
         // See how long it took.
         final int NUM_REQUESTS = 90;
+        final int REGISTER_TIME_LIMIT_MS = 180;
+        final int CONNECT_TIME_LIMIT_MS = 50;
+        final int SWITCH_TIME_LIMIT_MS = 50;
+        final int UNREGISTER_TIME_LIMIT_MS = 20;
+
         final NetworkRequest request = new NetworkRequest.Builder().clearCapabilities().build();
         final NetworkCallback[] callbacks = new NetworkCallback[NUM_REQUESTS];
         final CountDownLatch availableLatch = new CountDownLatch(NUM_REQUESTS);
@@ -2404,14 +2437,12 @@ public class ConnectivityServiceTest extends AndroidTestCase {
             };
         }
 
-        final int REGISTER_TIME_LIMIT_MS = 180;
         assertTimeLimit("Registering callbacks", REGISTER_TIME_LIMIT_MS, () -> {
             for (NetworkCallback cb : callbacks) {
                 mCm.registerNetworkCallback(request, cb);
             }
         });
 
-        final int CONNECT_TIME_LIMIT_MS = 40;
         mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
         // Don't request that the network validate, because otherwise connect() will block until
         // the network gets NET_CAPABILITY_VALIDATED, after all the callbacks below have fired,
@@ -2419,31 +2450,29 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         mCellNetworkAgent.connect(false);
 
         long onAvailableDispatchingDuration = durationOf(() -> {
-            if (!awaitLatch(availableLatch, CONNECT_TIME_LIMIT_MS)) {
+            awaitLatch(availableLatch, 10 * CONNECT_TIME_LIMIT_MS);
-                fail(String.format("Only dispatched %d/%d onAvailable callbacks in %dms",
-                        NUM_REQUESTS - availableLatch.getCount(), NUM_REQUESTS,
-                        CONNECT_TIME_LIMIT_MS));
-            }
         });
-        Log.d(TAG, String.format("Connect, %d callbacks: %dms, acceptable %dms",
+        Log.d(TAG, String.format("Dispatched %d of %d onAvailable callbacks in %dms",
-                NUM_REQUESTS, onAvailableDispatchingDuration, CONNECT_TIME_LIMIT_MS));
+                NUM_REQUESTS - availableLatch.getCount(), NUM_REQUESTS,
+                onAvailableDispatchingDuration));
+        assertTrue(String.format("Dispatching %d onAvailable callbacks in %dms, expected %dms",
+                NUM_REQUESTS, onAvailableDispatchingDuration, CONNECT_TIME_LIMIT_MS),
+                onAvailableDispatchingDuration <= CONNECT_TIME_LIMIT_MS);
 
-        final int SWITCH_TIME_LIMIT_MS = 40;
-        mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
         // Give wifi a high enough score that we'll linger cell when wifi comes up.
+        mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
         mWiFiNetworkAgent.adjustScore(40);
         mWiFiNetworkAgent.connect(false);
 
         long onLostDispatchingDuration = durationOf(() -> {
-            if (!awaitLatch(losingLatch, SWITCH_TIME_LIMIT_MS)) {
+            awaitLatch(losingLatch, 10 * SWITCH_TIME_LIMIT_MS);
-                fail(String.format("Only dispatched %d/%d onLosing callbacks in %dms",
-                        NUM_REQUESTS - losingLatch.getCount(), NUM_REQUESTS, SWITCH_TIME_LIMIT_MS));
-            }
         });
-        Log.d(TAG, String.format("Linger, %d callbacks: %dms, acceptable %dms",
+        Log.d(TAG, String.format("Dispatched %d of %d onLosing callbacks in %dms",
-                NUM_REQUESTS, onLostDispatchingDuration, SWITCH_TIME_LIMIT_MS));
+                NUM_REQUESTS - losingLatch.getCount(), NUM_REQUESTS, onLostDispatchingDuration));
+        assertTrue(String.format("Dispatching %d onLosing callbacks in %dms, expected %dms",
+                NUM_REQUESTS, onLostDispatchingDuration, SWITCH_TIME_LIMIT_MS),
+                onLostDispatchingDuration <= SWITCH_TIME_LIMIT_MS);
 
-        final int UNREGISTER_TIME_LIMIT_MS = 10;
         assertTimeLimit("Unregistering callbacks", UNREGISTER_TIME_LIMIT_MS, () -> {
             for (NetworkCallback cb : callbacks) {
                 mCm.unregisterNetworkCallback(cb);
@@ -2466,9 +2495,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 
     private boolean awaitLatch(CountDownLatch l, long timeoutMs) {
         try {
-            if (l.await(timeoutMs, TimeUnit.MILLISECONDS)) {
+            return l.await(timeoutMs, TimeUnit.MILLISECONDS);
-                return true;
-            }
         } catch (InterruptedException e) {}
         return false;
     }
@@ -2520,7 +2547,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         assertFalse(testFactory.getMyStartRequested());  // Because the cell network outscores us.
 
         // Check that cell data stays up.
-        mService.waitForIdle();
+        waitForIdle();
         verifyActiveNetwork(TRANSPORT_WIFI);
         assertEquals(2, mCm.getAllNetworks().length);
 
@@ -2549,7 +2576,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         for (int i = 0; i < values.length; i++) {
             Settings.Global.putInt(cr, settingName, 1);
             tracker.reevaluate();
-            mService.waitForIdle();
+            waitForIdle();
             String msg = String.format("config=false, setting=%s", values[i]);
             assertTrue(mService.avoidBadWifi());
             assertFalse(msg, tracker.shouldNotifyWifiUnvalidated());
@@ -2559,19 +2586,19 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 
         Settings.Global.putInt(cr, settingName, 0);
         tracker.reevaluate();
-        mService.waitForIdle();
+        waitForIdle();
         assertFalse(mService.avoidBadWifi());
         assertFalse(tracker.shouldNotifyWifiUnvalidated());
 
         Settings.Global.putInt(cr, settingName, 1);
         tracker.reevaluate();
-        mService.waitForIdle();
+        waitForIdle();
         assertTrue(mService.avoidBadWifi());
         assertFalse(tracker.shouldNotifyWifiUnvalidated());
 
         Settings.Global.putString(cr, settingName, null);
         tracker.reevaluate();
-        mService.waitForIdle();
+        waitForIdle();
         assertFalse(mService.avoidBadWifi());
         assertTrue(tracker.shouldNotifyWifiUnvalidated());
     }
@@ -2714,7 +2741,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
                 tracker.configMeteredMultipathPreference = config;
                 Settings.Global.putString(cr, settingName, setting);
                 tracker.reevaluate();
-                mService.waitForIdle();
+                waitForIdle();
 
                 final int expected = (setting != null) ? Integer.parseInt(setting) : config;
                 String msg = String.format("config=%d, setting=%s", config, setting);
@@ -2902,7 +2929,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         waitFor(cv);
         verifyActiveNetwork(TRANSPORT_WIFI);
         mWiFiNetworkAgent.sendLinkProperties(lp);
-        mService.waitForIdle();
+        waitForIdle();
         return mWiFiNetworkAgent.getNetwork();
     }
 
@@ -2981,7 +3008,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         callback.expectError(PacketKeepalive.ERROR_INVALID_NETWORK);
 
         // ... and that stopping it after that has no adverse effects.
-        mService.waitForIdle();
+        waitForIdle();
         final Network myNetAlias = myNet;
         assertNull(mCm.getNetworkCapabilities(myNetAlias));
         ka.stop();
@@ -2996,7 +3023,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         ka.stop();
         mWiFiNetworkAgent.disconnect();
         waitFor(mWiFiNetworkAgent.getDisconnectedCV());
-        mService.waitForIdle();
+        waitForIdle();
         callback.expectStopped();
 
         // Reconnect.
@@ -3197,7 +3224,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
             mCm.unregisterNetworkCallback(pendingIntent);
         }
         pendingIntents.clear();
-        mService.waitForIdle(5000);
+        waitForIdle(5000);
 
         // Test that the limit is not hit when MAX_REQUESTS requests are added and removed.
         for (int i = 0; i < MAX_REQUESTS; i++) {
@@ -3205,20 +3232,20 @@ public class ConnectivityServiceTest extends AndroidTestCase {
             mCm.requestNetwork(networkRequest, networkCallback);
             mCm.unregisterNetworkCallback(networkCallback);
         }
-        mService.waitForIdle();
+        waitForIdle();
         for (int i = 0; i < MAX_REQUESTS; i++) {
             NetworkCallback networkCallback = new NetworkCallback();
             mCm.registerNetworkCallback(networkRequest, networkCallback);
             mCm.unregisterNetworkCallback(networkCallback);
         }
-        mService.waitForIdle();
+        waitForIdle();
         for (int i = 0; i < MAX_REQUESTS; i++) {
             PendingIntent pendingIntent =
                     PendingIntent.getBroadcast(mContext, 0, new Intent("b" + i), 0);
             mCm.requestNetwork(networkRequest, pendingIntent);
             mCm.unregisterNetworkCallback(pendingIntent);
         }
-        mService.waitForIdle();
+        waitForIdle();
         for (int i = 0; i < MAX_REQUESTS; i++) {
             PendingIntent pendingIntent =
                     PendingIntent.getBroadcast(mContext, 0, new Intent("c" + i), 0);