Correct tests for verifying data stall metrics

NetworkMonitor sends data stall metrics data via static method.
The DataStallStatsUtils object from its constructor is never
used. Tests in NetworkMonitorTest that verify the interaction
with the mock object are incorrect.

Bug: 152374582
Test: atest NetworkStackTests NetworkStackNextTests
Merged-In: I308344a80deef6aaf3ed4fb57723f2f210b30483
Change-Id: I308344a80deef6aaf3ed4fb57723f2f210b30483

2 files changed, 47 insertions, 10 deletions

diff --git a/src/com/android/server/connectivity/NetworkMonitor.java b/src/com/android/server/connectivity/NetworkMonitor.java
index fcdd507..d16fcf5 100644
--- a/src/com/android/server/connectivity/NetworkMonitor.java
+++ b/src/com/android/server/connectivity/NetworkMonitor.java
@@ -355,7 +355,6 @@ public class NetworkMonitor extends StateMachine {
     private final NetworkStackNotifier mNotifier;
     private final IpConnectivityLog mMetricsLog;
     private final Dependencies mDependencies;
-    private final DataStallStatsUtils mDetectionStatsUtils;
     private final TcpSocketTracker mTcpTracker;
     // Configuration values for captive portal detection probes.
     private final String mCaptivePortalUserAgent;
@@ -438,15 +437,14 @@ public class NetworkMonitor extends StateMachine {
     public NetworkMonitor(Context context, INetworkMonitorCallbacks cb, Network network,
             SharedLog validationLog, @NonNull NetworkStackServiceManager serviceManager) {
         this(context, cb, network, new IpConnectivityLog(), validationLog, serviceManager,
-                Dependencies.DEFAULT, new DataStallStatsUtils(),
-                getTcpSocketTrackerOrNull(context, network));
+                Dependencies.DEFAULT, getTcpSocketTrackerOrNull(context, network));
     }
 
     @VisibleForTesting
     public NetworkMonitor(Context context, INetworkMonitorCallbacks cb, Network network,
             IpConnectivityLog logger, SharedLog validationLogs,
             @NonNull NetworkStackServiceManager serviceManager, Dependencies deps,
-            DataStallStatsUtils detectionStatsUtils, @Nullable TcpSocketTracker tst) {
+            @Nullable TcpSocketTracker tst) {
         // Add suffix indicating which NetworkMonitor we're talking about.
         super(TAG + "/" + network.toString());
 
@@ -460,7 +458,6 @@ public class NetworkMonitor extends StateMachine {
         mCallback = cb;
         mCallbackVersion = getCallbackVersion(cb);
         mDependencies = deps;
-        mDetectionStatsUtils = detectionStatsUtils;
         mNetwork = network;
         mCleartextDnsNetwork = deps.getPrivateDnsBypassNetwork(network);
         mTelephonyManager = (TelephonyManager) context.getSystemService(Context.TELEPHONY_SERVICE);
@@ -897,7 +894,8 @@ public class NetworkMonitor extends StateMachine {
         final int[] transports = mNetworkCapabilities.getTransportTypes();
 
         for (int i = 0; i < transports.length; i++) {
-            DataStallStatsUtils.write(buildDataStallDetectionStats(transports[i]), result);
+            final DataStallDetectionStats stats = buildDataStallDetectionStats(transports[i]);
+            mDependencies.writeDataStallDetectionStats(stats, result);
         }
         mCollectDataStallMetrics = false;
     }
@@ -2419,6 +2417,18 @@ public class NetworkMonitor extends StateMachine {
             return NetworkStackUtils.isFeatureEnabled(context, namespace, name, defaultEnabled);
         }
 
+        /**
+         * Collect data stall detection level information for each transport type. Write metrics
+         * data to statsd pipeline.
+         * @param stats a {@link DataStallDetectionStats} that contains the detection level
+         *              information.
+         * @para result the network reevaluation result.
+         */
+        public void writeDataStallDetectionStats(@NonNull final DataStallDetectionStats stats,
+                @NonNull final CaptivePortalProbeResult result) {
+            DataStallStatsUtils.write(stats, result);
+        }
+
         public static final Dependencies DEFAULT = new Dependencies();
     }
 
diff --git a/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java b/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
index bbedf59..ab1ebd6 100644
--- a/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
+++ b/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
@@ -524,7 +524,7 @@ public class NetworkMonitorTest {
 
         WrappedNetworkMonitor() {
             super(mContext, mCallbacks, mNetwork, mLogger, mValidationLogger, mServiceManager,
-                    mDependencies, mDataStallStatsUtils, mTst);
+                    mDependencies, mTst);
         }
 
         @Override
@@ -1332,21 +1332,48 @@ public class NetworkMonitorTest {
     }
 
     @Test
-    public void testDataStall_StallSuspectedAndSendMetrics() throws IOException {
+    public void testDataStall_StallDnsSuspectedAndSendMetrics() throws IOException {
+        // Connect a VALID network to simulate the data stall detection because data stall
+        // evaluation will only start from validated state.
+        setStatus(mHttpsConnection, 204);
         WrappedNetworkMonitor wrappedMonitor = makeNotMeteredNetworkMonitor();
+        wrappedMonitor.notifyNetworkConnected(TEST_LINK_PROPERTIES, METERED_CAPABILITIES);
+        verifyNetworkTested(VALIDATION_RESULT_VALID);
+        // Setup dns data stall signal.
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 1000);
         makeDnsTimeoutEvent(wrappedMonitor, 5);
         assertTrue(wrappedMonitor.isDataStall());
-        verify(mDataStallStatsUtils, times(1)).write(makeEmptyDataStallDetectionStats(), any());
+        // Trigger a dns signal to start evaluate data stall and upload metrics.
+        wrappedMonitor.notifyDnsResponse(RETURN_CODE_DNS_TIMEOUT);
+        // Setup information to prevent null data and cause NPE during testing.
+        when(mTelephony.getDataNetworkType()).thenReturn(TelephonyManager.NETWORK_TYPE_LTE);
+        when(mTelephony.getNetworkOperator()).thenReturn(TEST_MCCMNC);
+        when(mTelephony.getSimOperator()).thenReturn(TEST_MCCMNC);
+        // Verify data sent as expectation.
+        final DataStallDetectionStats stats = wrappedMonitor.buildDataStallDetectionStats(
+                NetworkCapabilities.TRANSPORT_CELLULAR);
+        final ArgumentCaptor<CaptivePortalProbeResult> probeResultCaptor =
+                ArgumentCaptor.forClass(CaptivePortalProbeResult.class);
+        verify(mDependencies, timeout(HANDLER_TIMEOUT_MS).times(1))
+                .writeDataStallDetectionStats(eq(stats), probeResultCaptor.capture());
+        assertTrue(probeResultCaptor.getValue().isSuccessful());
     }
 
     @Test
     public void testDataStall_NoStallSuspectedAndSendMetrics() throws IOException {
+        // Connect a VALID network to simulate the data stall detection because data stall
+        // evaluation will only start from validated state.
+        setStatus(mHttpsConnection, 204);
         WrappedNetworkMonitor wrappedMonitor = makeNotMeteredNetworkMonitor();
+        wrappedMonitor.notifyNetworkConnected(TEST_LINK_PROPERTIES, METERED_CAPABILITIES);
+        verifyNetworkTested(VALIDATION_RESULT_VALID);
+        // Setup no data stall dns signal.
         wrappedMonitor.setLastProbeTime(SystemClock.elapsedRealtime() - 1000);
         makeDnsTimeoutEvent(wrappedMonitor, 3);
         assertFalse(wrappedMonitor.isDataStall());
-        verify(mDataStallStatsUtils, never()).write(makeEmptyDataStallDetectionStats(), any());
+        // Trigger a dns signal to start evaluate data stall.
+        wrappedMonitor.notifyDnsResponse(RETURN_CODE_DNS_SUCCESS);
+        verify(mDependencies, never()).writeDataStallDetectionStats(any(), any());
     }
 
     @Test