From d4c521ab43f2778b4ef0971b2efa2f3308c50b57 Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Wed, 4 Jun 2025 23:26:10 +0800 Subject: [PATCH] Test refactoring of DefaultNetworkRematchMetrics This change includes: 1. addEvent Parameter Update: Added satisfied duration to addEvent, making duration testable in CSTest. 2. CSDefaultNetworkRematchMetricsTest Refactor: Tests now use expectAddEvent with argThat and InOrder for more deterministic verification. 3. New Unit Test: Added a test to demonstrate a metrics reporting issue during per-app default network request updates. Test: atest ConnectivityCoverageTests --test-filter 'DefaultNetworkRematch*' \ --update-device Flag: EXEMPT Test only refactoring Bug: 419123367 Change-Id: I3c73b445674991f85e37b3f980fa80fb918713ac --- .../metrics/DefaultNetworkRematchMetrics.java | 18 +- .../android/server/ConnectivityService.java | 6 +- .../DefaultNetworkRematchMetricsTest.kt | 23 +-- .../CSDefaultNetworkRematchMetricsTest.kt | 178 +++++++++++++----- 4 files changed, 148 insertions(+), 77 deletions(-) diff --git a/service/src/com/android/metrics/DefaultNetworkRematchMetrics.java b/service/src/com/android/metrics/DefaultNetworkRematchMetrics.java index df32044b58..cced96c3c5 100644 --- a/service/src/com/android/metrics/DefaultNetworkRematchMetrics.java +++ b/service/src/com/android/metrics/DefaultNetworkRematchMetrics.java @@ -30,7 +30,6 @@ import static com.android.server.ConnectivityStatsLog.DEFAULT_NETWORK_REMATCH__R import android.annotation.NonNull; import android.annotation.Nullable; import android.net.NetworkCapabilities; -import android.os.SystemClock; import android.stats.connectivity.MeteredState; import android.stats.connectivity.ValidatedState; @@ -73,13 +72,6 @@ public class DefaultNetworkRematchMetrics { ConnectivityStatsLog.write(DEFAULT_NETWORK_REMATCH, sessionId, rematchReason, list.toByteArray()); } - - /** - * Returns SystemClock.elapsedRealtime() - */ - public long getElapsedRealtime() { - return SystemClock.elapsedRealtime(); - } } /** @@ -91,7 +83,8 @@ public class DefaultNetworkRematchMetrics { */ public void addEvent(@NonNull ConnectivityService.NetworkRequestInfo nri, @Nullable NetworkAgentInfo oldNetwork, - @Nullable NetworkAgentInfo newNetwork) { + @Nullable NetworkAgentInfo newNetwork, + long satisfiedDurationMs) { // TODO: Record event for network other than satellite after figuring out // how to deal with the amount of data single device reports. // Only logs for satellite multilayer requests. @@ -103,7 +96,7 @@ public class DefaultNetworkRematchMetrics { return; } mBuilder.addDefaultNetworkRematchInfo( - getDefaultNetworkRematchInfo(nri, oldNetwork, newNetwork)); + getDefaultNetworkRematchInfo(nri, oldNetwork, newNetwork, satisfiedDurationMs)); } /** @@ -125,13 +118,14 @@ public class DefaultNetworkRematchMetrics { @NonNull private DefaultNetworkRematchInfo getDefaultNetworkRematchInfo( @NonNull ConnectivityService.NetworkRequestInfo nri, - @Nullable NetworkAgentInfo oldNetwork, @Nullable NetworkAgentInfo newNetwork) { + @Nullable NetworkAgentInfo oldNetwork, @Nullable NetworkAgentInfo newNetwork, + long satisfiedDurationMs) { final DefaultNetworkRematchInfo.Builder infoBuilder = DefaultNetworkRematchInfo.newBuilder(); if (oldNetwork != null) { infoBuilder.setOldNetwork(getNetworkDescription(oldNetwork)); infoBuilder.setTimeDurationOnOldNetworkSec( - (int) ((mDeps.getElapsedRealtime() - nri.getSatisfiedTime()) / 1000)); + (int) (satisfiedDurationMs / 1000)); } if (newNetwork != null) { infoBuilder.setNewNetwork(getNetworkDescription(newNetwork)); diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 2646be0e48..ccda9c503e 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -5821,7 +5821,8 @@ public class ConnectivityService extends IConnectivityManager.Stub if (mConstrainedDataSatelliteMetrics && mDefaultNetworkRequests.contains(nri)) { // Must be called before satisfier changes to get satisfied time. // See NRI#getSatisfiedTime(). - mDefaultNetworkRematchMetrics.addEvent(nri, currentNetwork, null); + mDefaultNetworkRematchMetrics.addEvent(nri, currentNetwork, null, + mDeps.getElapsedRealtime() - nri.getSatisfiedTime()); } nri.setSatisfier(null, null); @@ -11703,7 +11704,8 @@ public class ConnectivityService extends IConnectivityManager.Stub continue; } mDefaultNetworkRematchMetrics.addEvent(defaultRequestInfo, - reassignment.mOldNetwork, reassignment.mNewNetwork); + reassignment.mOldNetwork, reassignment.mNewNetwork, + mDeps.getElapsedRealtime() - defaultRequestInfo.getSatisfiedTime()); } // TODO: fill rematch reason. mDefaultNetworkRematchMetrics.writeStatsAndClear(); diff --git a/tests/unit/java/com/android/metrics/DefaultNetworkRematchMetricsTest.kt b/tests/unit/java/com/android/metrics/DefaultNetworkRematchMetricsTest.kt index 9595be297a..967a59be00 100644 --- a/tests/unit/java/com/android/metrics/DefaultNetworkRematchMetricsTest.kt +++ b/tests/unit/java/com/android/metrics/DefaultNetworkRematchMetricsTest.kt @@ -94,7 +94,7 @@ class DefaultNetworkRematchMetricsTest { fun testAddEvent_wrongPreference_isIgnored() { doReturn(PREFERENCE_ORDER_NONE).`when`(nri).preferenceOrderForNetd - metrics.addEvent(nri, oldNai, newNai) + metrics.addEvent(nri, oldNai, newNai, 0L) metrics.writeStatsAndClear() verify(deps, never()).writeStats(anyLong(), anyInt(), any()) @@ -109,7 +109,7 @@ class DefaultNetworkRematchMetricsTest { doReturn(nonSatelliteCaps).`when`(oldNai).capsNoCopy // Action: Attempt to add the event and write stats. - metrics.addEvent(nri, oldNai, newNai) + metrics.addEvent(nri, oldNai, newNai, 0L) metrics.writeStatsAndClear() // Verification: The event should be ignored, so writeStats should never be called. @@ -118,7 +118,7 @@ class DefaultNetworkRematchMetricsTest { @Test fun testAddEvent_validRequest_isAdded() { - metrics.addEvent(nri, oldNai, newNai) + metrics.addEvent(nri, oldNai, newNai, 0L) metrics.writeStatsAndClear() verify(deps, times(1)).writeStats(anyLong(), anyInt(), any()) @@ -133,26 +133,17 @@ class DefaultNetworkRematchMetricsTest { @Test fun testWriteStats_withEvents_writesAndClears() { // Setup: Define the time the network satisfied time and the current time. - val satisfiedTimeMs = 100_000L - val currentTimeMs = 220_000L // 120 seconds after satisfied - val expectedDurationSec = ((currentTimeMs - satisfiedTimeMs) / 1000).toInt() - - // Mock the dependencies to return the defined times. - doReturn(satisfiedTimeMs).`when`(oldNai).connectedTime - doReturn(currentTimeMs).`when`(deps).elapsedRealtime - - // Mock the dependencies to return the defined times. - doReturn(satisfiedTimeMs).`when`(nri).satisfiedTime + val satisfiedDurationMs = 120_000L // First call: Add event and write - metrics.addEvent(nri, oldNai, newNai) + metrics.addEvent(nri, oldNai, newNai, satisfiedDurationMs) metrics.writeStatsAndClear() verify(deps, times(1)).writeStats(anyLong(), anyInt(), infoListCaptor.capture()) val capturedList = infoListCaptor.value assertEquals(1, capturedList.defaultNetworkRematchInfoList.size) val info = capturedList.defaultNetworkRematchInfoList[0] - assertEquals(expectedDurationSec, info.timeDurationOnOldNetworkSec) + assertEquals((satisfiedDurationMs / 1000).toInt(), info.timeDurationOnOldNetworkSec) // Second call: Should do nothing as events are cleared clearInvocations(deps) @@ -214,7 +205,7 @@ class DefaultNetworkRematchMetricsTest { val uids = setOf(UidRange(1000, 1000), UidRange(2000, 3000)) doReturn(uids).`when`(nri).uids - metrics.addEvent(nri, oldNai, null) + metrics.addEvent(nri, oldNai, null, 0L) metrics.writeStatsAndClear() verify(deps).writeStats(anyLong(), anyInt(), infoListCaptor.capture()) diff --git a/tests/unit/java/com/android/server/connectivityservice/CSDefaultNetworkRematchMetricsTest.kt b/tests/unit/java/com/android/server/connectivityservice/CSDefaultNetworkRematchMetricsTest.kt index 80b78836bd..d95fdf42f2 100644 --- a/tests/unit/java/com/android/server/connectivityservice/CSDefaultNetworkRematchMetricsTest.kt +++ b/tests/unit/java/com/android/server/connectivityservice/CSDefaultNetworkRematchMetricsTest.kt @@ -16,106 +16,190 @@ package com.android.server +import android.net.INetd import android.net.Network import android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET import android.net.NetworkCapabilities.TRANSPORT_CELLULAR import android.net.NetworkCapabilities.TRANSPORT_WIFI +import android.net.UidRange import android.os.Build -import com.android.server.ConnectivityService.NetworkRequestInfo +import com.android.server.ConnectivityService.PREFERENCE_ORDER_SATELLITE_FALLBACK import com.android.server.ConnectivityStatsLog.DEFAULT_NETWORK_REMATCH__REMATCH_REASON__RMR_NETWORK_DISCONNECTED -import com.android.server.connectivity.NetworkAgentInfo import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo import com.android.testutils.DevSdkIgnoreRunner -import org.junit.Assert.fail +import com.android.testutils.postAndWait import org.junit.Test import org.junit.runner.RunWith -import org.mockito.ArgumentCaptor +import org.mockito.ArgumentMatchers.argThat +import org.mockito.ArgumentMatchers.eq +import org.mockito.InOrder import org.mockito.Mockito.atLeastOnce -import org.mockito.Mockito.reset +import org.mockito.Mockito.inOrder import org.mockito.Mockito.verify private const val DEFAULT_REQUEST_ID = 1 -private inline fun argumentCaptor() = ArgumentCaptor.forClass(T::class.java) +private const val TEST_UID = 1234 +private const val TEST_UID2 = 5678 + +// Timestamp constants. They need to be chosen to prevent a difference between +// any two of them from being equal to a difference between another two +// (or more) of them. This ensures that wrong duration calculations won't pass +// the tests accidentally. +private const val t0 = 0L +private const val t1 = 11L +private const val t2 = 23L +private const val t3 = 38L +private const val t4 = 56L @IgnoreUpTo(Build.VERSION_CODES.TIRAMISU) @RunWith(DevSdkIgnoreRunner::class) class CSDefaultNetworkRematchMetricsTest : CSTest() { + companion object { + private val NO_SERVICE_NETWORK = Network(INetd.UNREACHABLE_NET_ID) + } + @Test fun testRematchWritesStats() { - waitForIdle() - reset(defaultNetworkRematchMetrics) + val inOrder = inOrder(defaultNetworkRematchMetrics) // 1. Connect a cellular network. It becomes the default. + deps.elapsedRealtime = t1 val naiCell = Agent(nc = nc(TRANSPORT_CELLULAR, NET_CAPABILITY_INTERNET)) naiCell.connect(true) - waitForIdle() - eventuallyExpectAddEvent(DEFAULT_REQUEST_ID, null, naiCell.network) - verify(defaultNetworkRematchMetrics, atLeastOnce()).writeStatsAndClear() - reset(defaultNetworkRematchMetrics) + expectAddEvent(inOrder, DEFAULT_REQUEST_ID, null, naiCell.network, t1 - t0) + inOrder.verify(defaultNetworkRematchMetrics, atLeastOnce()).writeStatsAndClear() // 2. Connect a WiFi network. It has a higher score and will cause a rematch. + deps.elapsedRealtime = t2 val naiWifi = Agent(nc = nc(TRANSPORT_WIFI, NET_CAPABILITY_INTERNET)) naiWifi.connect(true) - waitForIdle() // 3. Verify: The rematch should trigger writing the stats. - eventuallyExpectAddEvent(DEFAULT_REQUEST_ID, naiCell.network, naiWifi.network) - verify(defaultNetworkRematchMetrics, atLeastOnce()).writeStatsAndClear() + expectAddEvent(inOrder, DEFAULT_REQUEST_ID, naiCell.network, naiWifi.network, t2 - t1) + inOrder.verify(defaultNetworkRematchMetrics, atLeastOnce()).writeStatsAndClear() + inOrder.verifyNoMoreInteractions() } @Test fun testDisconnectWritesStats() { - waitForIdle() - reset(defaultNetworkRematchMetrics) + val inOrder = inOrder(defaultNetworkRematchMetrics) // 1. Connect a cellular network. It becomes the default. + deps.elapsedRealtime = t1 val naiCell = Agent(nc = nc(TRANSPORT_CELLULAR, NET_CAPABILITY_INTERNET)) naiCell.connect(true) - waitForIdle() - eventuallyExpectAddEvent(DEFAULT_REQUEST_ID, null, naiCell.network) - verify(defaultNetworkRematchMetrics, atLeastOnce()).writeStatsAndClear() - reset(defaultNetworkRematchMetrics) + expectAddEvent(inOrder, DEFAULT_REQUEST_ID, null, naiCell.network, t1 - t0) + inOrder.verify(defaultNetworkRematchMetrics).writeStatsAndClear() + inOrder.verifyNoMoreInteractions() // 2. Disconnect the cellular network. + deps.elapsedRealtime = t2 naiCell.disconnect() waitForIdle() // 3. Verify: Disconnecting the default network should trigger writing the stats. - eventuallyExpectAddEvent(DEFAULT_REQUEST_ID, naiCell.network, null) - verify(defaultNetworkRematchMetrics, atLeastOnce()).writeStatsAndClear( + expectAddEvent(inOrder, DEFAULT_REQUEST_ID, naiCell.network, null, t2 - t1) + inOrder.verify(defaultNetworkRematchMetrics).writeStatsAndClear( DEFAULT_NETWORK_REMATCH__REMATCH_REASON__RMR_NETWORK_DISCONNECTED ) + // Caused by rematch. + inOrder.verify(defaultNetworkRematchMetrics).writeStatsAndClear() + inOrder.verifyNoMoreInteractions() } - private fun eventuallyExpectAddEvent( - expectedRequestId: Int, - expectedOldNetwork: Network?, - expectedNewNetwork: Network? + private fun expectAddEvent( + inOrder: InOrder, + requestId: Int, + oldNetwork: Network?, + newNetwork: Network?, + satisfiedDurationMs: Long ) { - val nriCaptor = argumentCaptor() - val oldNetworkCaptor = argumentCaptor() - val newNetworkCaptor = argumentCaptor() - verify(defaultNetworkRematchMetrics, atLeastOnce()).addEvent( - nriCaptor.capture(), - oldNetworkCaptor.capture(), - newNetworkCaptor.capture() + inOrder.verify(defaultNetworkRematchMetrics).addEvent( + argThat { it.mRequests.firstOrNull()?.requestId == requestId }, + argThat { it?.network == oldNetwork }, + argThat { it?.network == newNetwork }, + eq(satisfiedDurationMs) ) + } - // TODO: Support checking multilayer requests. - val capturedEvents = nriCaptor.allValues.indices.map { i -> - val requestId = nriCaptor.allValues[i].mRequests[0].requestId - val oldNetwork = oldNetworkCaptor.allValues[i] - val newNetwork = newNetworkCaptor.allValues[i] - if (requestId == expectedRequestId && oldNetwork?.network == expectedOldNetwork && - newNetwork?.network == expectedNewNetwork) { - return - } - Triple(requestId, oldNetwork?.network, newNetwork?.network) + private fun expectAddSatelliteEvent( + inOrder: InOrder, + expectedUid: Int, + oldNetwork: Network?, + newNetwork: Network?, + expectedSatisfiedDuration: Long + ) { + inOrder.verify(defaultNetworkRematchMetrics).addEvent( + argThat { + it.preferenceOrderForNetd == PREFERENCE_ORDER_SATELLITE_FALLBACK && + it.uids.contains(UidRange(expectedUid, expectedUid)) + }, + argThat { it?.network == oldNetwork }, + argThat { it?.network == newNetwork }, + eq(expectedSatisfiedDuration) + ) + } + + private fun updateSatelliteNetworkFallbackUids(messagingUids: Set, optinUids: Set) { + csHandler.postAndWait { + deps.satelliteNetworkFallbackUidUpdate!!.accept(messagingUids, optinUids) } + } - fail( - "Expecting ($expectedRequestId, $expectedOldNetwork, $expectedNewNetwork) " + - "but got $capturedEvents" + @Test + fun testReplaceSatelliteRequestWritesStats() { + val inOrder = inOrder(defaultNetworkRematchMetrics) + + // Trigger CS to add the SATELLITE_FALLBACK multilayer network request. + // Only an opt-in UID multilayer request is created since there is no + // role-sms UID. + // The satellite request is created and immediately satisfied by NO_SERVICE_NETWORK + // if it rematches when no network is available. + deps.elapsedRealtime = t1 + updateSatelliteNetworkFallbackUids(setOf(), setOf(TEST_UID)) + // The request created and rematched immediately so it satisfied by null with no time. + expectAddSatelliteEvent(inOrder, TEST_UID, null, NO_SERVICE_NETWORK, 0) + + // Connect a cellular network. Expect an event indicating the NO_SERVICE_NETWORK + // was satisfied for the duration t2 - t1. + deps.elapsedRealtime = t2 + val naiCell = Agent(nc = nc(TRANSPORT_CELLULAR, NET_CAPABILITY_INTERNET)) + naiCell.connect(true) + expectAddEvent(inOrder, DEFAULT_REQUEST_ID, null, naiCell.network, t2 - t0) + expectAddSatelliteEvent(inOrder, TEST_UID, NO_SERVICE_NETWORK, naiCell.network, t2 - t1) + verify(defaultNetworkRematchMetrics, atLeastOnce()).writeStatsAndClear() + + // Make a UID list update to trigger the multilayer requests replacement: + // 1. Remove opt-in UID request for TEST_UID. + // 2. Create opt-in UID request for TEST_UID + TEST_UID2. + deps.elapsedRealtime = t3 + updateSatelliteNetworkFallbackUids(setOf(), setOf(TEST_UID, TEST_UID2)) + // The removed request reports nothing. + // TODO: Consider reports another event when the request is being removed. + // Uncomment expectAddSatelliteEvent(inOrder, TEST_UID, naiCell.network, null, t3 - t2) + // Or alternatively copy the satisfiedTime from the removed requests. + // The just created request satisfied by cell immediately. + expectAddSatelliteEvent(inOrder, TEST_UID, null, naiCell.network, 0) + inOrder.verify(defaultNetworkRematchMetrics, atLeastOnce()).writeStatsAndClear() + inOrder.verifyNoMoreInteractions() + + deps.elapsedRealtime = t4 + naiCell.disconnect() + // Wait for idle is needed to keep this test stable after disconnection because + // networkCallback fired before addEvent. + waitForIdle() + // Verify: Disconnecting the default network should trigger writing the stats. + // The device reports cell satisfied for t4 - t3, with a transition null -> no service. + // This is because the original request is removed when updating the uid list and the + // duration is only calculated for the newly created request at t3. + expectAddEvent(inOrder, DEFAULT_REQUEST_ID, naiCell.network, null, t4 - t2) + expectAddSatelliteEvent(inOrder, TEST_UID, naiCell.network, null, t4 - t3) + inOrder.verify(defaultNetworkRematchMetrics).writeStatsAndClear( + DEFAULT_NETWORK_REMATCH__REMATCH_REASON__RMR_NETWORK_DISCONNECTED ) + // Caused by rematch. + expectAddSatelliteEvent(inOrder, TEST_UID, null, NO_SERVICE_NETWORK, 0) + inOrder.verify(defaultNetworkRematchMetrics).writeStatsAndClear() + inOrder.verifyNoMoreInteractions() } }