From baea70055458d30214cc84d5f93ad75fc8483815 Mon Sep 17 00:00:00 2001
From: Lorenzo Colitti <lorenzo@google.com>
Date: Wed, 10 Feb 2021 22:32:24 +0900
Subject: [PATCH] Improve testing of registerSystemDefaultNetworkCallback.

Addresses comments on aosp/1570840.

Bug: 179774433
Test: test-only change
Change-Id: I71a376631503e5b50ada3f7bb3dca6dbae9ebc27
---
 .../com/android/cts/net/hostside/VpnTest.java | 39 +++++++++++++------
 .../net/cts/ConnectivityManagerTest.java      |  7 +++-
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/tests/cts/hostside/app/src/com/android/cts/net/hostside/VpnTest.java b/tests/cts/hostside/app/src/com/android/cts/net/hostside/VpnTest.java
index 4668ba3670..a663cd6f73 100755
--- a/tests/cts/hostside/app/src/com/android/cts/net/hostside/VpnTest.java
+++ b/tests/cts/hostside/app/src/com/android/cts/net/hostside/VpnTest.java
@@ -698,16 +698,30 @@ public class VpnTest extends InstrumentationTestCase {
     }
 
     private class NeverChangeNetworkCallback extends NetworkCallback {
-        private volatile Network mLastNetwork;
+        private CountDownLatch mLatch = new CountDownLatch(1);
+        private volatile Network mFirstNetwork;
+        private volatile Network mOtherNetwork;
 
         public void onAvailable(Network n) {
-            assertNull("Callback got onAvailable more than once: " + mLastNetwork + ", " + n,
-                    mLastNetwork);
-            mLastNetwork = n;
+            // Don't assert here, as it crashes the test with a hard to debug message.
+            if (mFirstNetwork == null) {
+                mFirstNetwork = n;
+                mLatch.countDown();
+            } else if (mOtherNetwork == null) {
+                mOtherNetwork = n;
+            }
         }
 
-        public Network getLastNetwork() {
-            return mLastNetwork;
+        public Network getFirstNetwork() throws Exception {
+            assertTrue(
+                    "System default callback got no network after " + TIMEOUT_MS + "ms. "
+                    + "Please ensure the device has a working Internet connection.",
+                    mLatch.await(TIMEOUT_MS, TimeUnit.MILLISECONDS));
+            return mFirstNetwork;
+        }
+
+        public void assertNeverChanged() {
+            assertNull(mOtherNetwork);
         }
     }
 
@@ -753,13 +767,14 @@ public class VpnTest extends InstrumentationTestCase {
 
         expectVpnTransportInfo(mCM.getActiveNetwork());
 
-        // Check that system default network callback has not seen any network changes, but the app
-        // default network callback has. This needs to be done before testing private DNS because
-        // checkStrictModePrivateDns will set the private DNS server to a nonexistent name, which
-        // will cause validation to fail could cause the default network to switch (e.g., from wifi
-        // to cellular).
-        assertEquals(defaultNetwork, neverChangeCallback.getLastNetwork());
+        // Check that system default network callback has not seen any network changes, even though
+        // the app's default network changed. This needs to be done before testing private
+        // DNS because checkStrictModePrivateDns will set the private DNS server to a nonexistent
+        // name, which will cause validation to fail and cause the default network to switch (e.g.,
+        // from wifi to cellular).
+        assertEquals(defaultNetwork, neverChangeCallback.getFirstNetwork());
         assertNotEqual(defaultNetwork, mCM.getActiveNetwork());
+        neverChangeCallback.assertNeverChanged();
         runWithShellPermissionIdentity(
                 () ->  mCM.unregisterNetworkCallback(neverChangeCallback),
                 NETWORK_SETTINGS);
diff --git a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java
index 0a948a09d2..831810c42a 100644
--- a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java
+++ b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java
@@ -533,11 +533,14 @@ public class ConnectivityManagerTest {
             // network even if it was already connected as a state-based action when the callback
             // is registered.
             wifiNetwork = callback.waitForAvailable();
-            assertNotNull("Did not receive NetworkCallback.onAvailable for TRANSPORT_WIFI",
+            assertNotNull("Did not receive onAvailable for TRANSPORT_WIFI request",
                     wifiNetwork);
 
-            assertNotNull("Did not receive NetworkCallback.onAvailable for any default network",
+            assertNotNull("Did not receive onAvailable on default network callback",
                     defaultTrackingCallback.waitForAvailable());
+
+            assertNotNull("Did not receive onAvailable on system default network callback",
+                    systemDefaultTrackingCallback.waitForAvailable());
         } catch (InterruptedException e) {
             fail("Broadcast receiver or NetworkCallback wait was interrupted.");
         } finally {