Replace connectToCell -> NetworkCallbackRule

Developers often forget to unregister requests in tests, and cleanup
code also makes tests more verbose.

Add a rule that unregisters callbacks automatically when after each test
method.

Replace current usage of connectToCell with this rule, as well as
network callback usage in CaptivePortalTest (which is one example that
did not unregister its callback).

Test: atest
Change-Id: Ie3b9c5cf529004b7b3af04bc13a7e69acfbbaa18
This commit is contained in:
Remi NGUYEN VAN 2024-03-26 18:12:46 +09:00
commit e629bb7c84
8 changed files with 207 additions and 75 deletions

View file

@ -0,0 +1,154 @@
/*
* Copyright (C) 2024 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.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* 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.
*/
package com.android.testutils
import android.net.ConnectivityManager
import android.net.ConnectivityManager.NetworkCallback
import android.net.Network
import android.net.NetworkCapabilities
import android.net.NetworkRequest
import androidx.test.platform.app.InstrumentationRegistry
import com.android.testutils.RecorderCallback.CallbackEntry
import java.util.Collections
import kotlin.test.fail
import org.junit.rules.TestRule
import org.junit.runner.Description
import org.junit.runners.model.Statement
/**
* A rule to file [NetworkCallback]s to request or watch networks.
*
* The callbacks filed in test methods are automatically unregistered when the method completes.
*/
class AutoReleaseNetworkCallbackRule : NetworkCallbackHelper(), TestRule {
override fun apply(base: Statement, description: Description): Statement {
return RequestCellNetworkStatement(base, description)
}
private inner class RequestCellNetworkStatement(
private val base: Statement,
private val description: Description
) : Statement() {
override fun evaluate() {
tryTest {
base.evaluate()
} cleanup {
unregisterAll()
}
}
}
}
/**
* Helps file [NetworkCallback]s to request or watch networks, keeping track of them for cleanup.
*/
open class NetworkCallbackHelper {
private val cm by lazy {
InstrumentationRegistry.getInstrumentation().context
.getSystemService(ConnectivityManager::class.java)
?: fail("ConnectivityManager not found")
}
private val cbToCleanup = Collections.synchronizedSet(mutableSetOf<NetworkCallback>())
private var cellRequestCb: TestableNetworkCallback? = null
/**
* Convenience method to request a cell network, similarly to [requestNetwork].
*
* The rule will keep tract of a single cell network request, which can be unrequested manually
* using [unrequestCell].
*/
fun requestCell(): Network {
if (cellRequestCb != null) {
fail("Cell network was already requested")
}
val cb = requestNetwork(
NetworkRequest.Builder()
.addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR)
.addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)
.build()
)
cellRequestCb = cb
return cb.expect<CallbackEntry.Available>(
errorMsg = "Cell network not available. " +
"Please ensure the device has working mobile data."
).network
}
/**
* Unrequest a cell network requested through [requestCell].
*/
fun unrequestCell() {
val cb = cellRequestCb ?: fail("Cell network was not requested")
unregisterNetworkCallback(cb)
cellRequestCb = null
}
/**
* File a request for a Network.
*
* This will fail tests (throw) if the cell network cannot be obtained, or if it was already
* requested.
*
* Tests may call [unregisterNetworkCallback] once they are done using the returned [Network],
* otherwise it will be automatically unrequested after the test.
*/
@JvmOverloads
fun requestNetwork(
request: NetworkRequest,
cb: TestableNetworkCallback = TestableNetworkCallback()
): TestableNetworkCallback {
cm.requestNetwork(request, cb)
cbToCleanup.add(cb)
return cb
}
/**
* File a callback for a NetworkRequest.
*
* This will fail tests (throw) if the cell network cannot be obtained, or if it was already
* requested.
*
* Tests may call [unregisterNetworkCallback] once they are done using the returned [Network],
* otherwise it will be automatically unrequested after the test.
*/
@JvmOverloads
fun registerNetworkCallback(
request: NetworkRequest,
cb: TestableNetworkCallback = TestableNetworkCallback()
): TestableNetworkCallback {
cm.registerNetworkCallback(request, cb)
cbToCleanup.add(cb)
return cb
}
/**
* Unregister a callback filed using registration methods in this class.
*/
fun unregisterNetworkCallback(cb: NetworkCallback) {
cm.unregisterNetworkCallback(cb)
cbToCleanup.remove(cb)
}
/**
* Unregister all callbacks that were filed using registration methods in this class.
*/
fun unregisterAll() {
cbToCleanup.forEach { cm.unregisterNetworkCallback(it) }
cbToCleanup.clear()
cellRequestCb = null
}
}

View file

@ -23,8 +23,8 @@ import static android.Manifest.permission.WRITE_DEVICE_CONFIG;
import static android.content.pm.PackageManager.FEATURE_TELEPHONY;
import static android.content.pm.PackageManager.FEATURE_WIFI;
import static android.net.ConnectivityManager.TYPE_VPN;
import static android.net.NetworkCapabilities.TRANSPORT_VPN;
import static android.net.NetworkCapabilities.TRANSPORT_TEST;
import static android.net.NetworkCapabilities.TRANSPORT_VPN;
import static android.os.Process.INVALID_UID;
import static android.system.OsConstants.AF_INET;
import static android.system.OsConstants.AF_INET6;
@ -123,6 +123,7 @@ import com.android.modules.utils.build.SdkLevel;
import com.android.net.module.util.ArrayTrackRecord;
import com.android.net.module.util.CollectionUtils;
import com.android.net.module.util.PacketBuilder;
import com.android.testutils.AutoReleaseNetworkCallbackRule;
import com.android.testutils.DevSdkIgnoreRule;
import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo;
import com.android.testutils.RecorderCallback;
@ -233,9 +234,13 @@ public class VpnTest {
// The registered callbacks.
private List<NetworkCallback> mRegisteredCallbacks = new ArrayList<>();
@Rule
@Rule(order = 1)
public final DevSdkIgnoreRule mDevSdkIgnoreRule = new DevSdkIgnoreRule();
@Rule(order = 2)
public final AutoReleaseNetworkCallbackRule
mNetworkCallbackRule = new AutoReleaseNetworkCallbackRule();
private boolean supportedHardware() {
final PackageManager pm = getInstrumentation().getContext().getPackageManager();
return !pm.hasSystemFeature("android.hardware.type.watch");
@ -273,7 +278,6 @@ public class VpnTest {
public void tearDown() throws Exception {
restorePrivateDnsSetting();
mRemoteSocketFactoryClient.unbind();
mCtsNetUtils.tearDown();
Log.i(TAG, "Stopping VPN");
stopVpn();
unregisterRegisteredCallbacks();
@ -890,9 +894,7 @@ public class VpnTest {
testAndCleanup(() -> {
// Ensure both of wifi and mobile data are connected.
final Network wifiNetwork = mCtsNetUtils.ensureWifiConnected();
assertTrue("Wifi is not connected", (wifiNetwork != null));
final Network cellNetwork = mCtsNetUtils.connectToCell();
assertTrue("Mobile data is not connected", (cellNetwork != null));
final Network cellNetwork = mNetworkCallbackRule.requestCell();
// Store current default network.
final Network defaultNetwork = mCM.getActiveNetwork();
// Start VPN and set empty array as its underlying networks.

View file

@ -35,6 +35,7 @@ import android.net.wifi.WifiNetworkSpecifier
import android.net.wifi.WifiSsid
import androidx.test.platform.app.InstrumentationRegistry
import com.android.testutils.ConnectUtil
import com.android.testutils.NetworkCallbackHelper
import com.android.testutils.RecorderCallback.CallbackEntry.Available
import com.android.testutils.RecorderCallback.CallbackEntry.CapabilitiesChanged
import com.android.testutils.TestableNetworkCallback
@ -48,9 +49,14 @@ class ConnectivityMultiDevicesSnippet : Snippet {
private val cm = context.getSystemService(ConnectivityManager::class.java)!!
private val pm = context.packageManager
private val ctsNetUtils = CtsNetUtils(context)
private val cbHelper = NetworkCallbackHelper()
private val ctsTetheringUtils = CtsTetheringUtils(context)
private var oldSoftApConfig: SoftApConfiguration? = null
override fun shutdown() {
cbHelper.unregisterAll()
}
@Rpc(description = "Check whether the device has wifi feature.")
fun hasWifiFeature() = pm.hasSystemFeature(FEATURE_WIFI)
@ -65,13 +71,13 @@ class ConnectivityMultiDevicesSnippet : Snippet {
@Rpc(description = "Request cellular connection and ensure it is the default network.")
fun requestCellularAndEnsureDefault() {
ctsNetUtils.disableWifi()
val network = ctsNetUtils.connectToCell()
val network = cbHelper.requestCell()
ctsNetUtils.expectNetworkIsSystemDefault(network)
}
@Rpc(description = "Unrequest cellular connection.")
fun unrequestCellular() {
ctsNetUtils.disconnectFromCell()
cbHelper.unrequestCell()
}
@Rpc(description = "Ensure any wifi is connected and is the default network.")

View file

@ -48,6 +48,7 @@ import androidx.test.filters.RequiresDevice;
import androidx.test.filters.SdkSuppress;
import androidx.test.runner.AndroidJUnit4;
import com.android.testutils.AutoReleaseNetworkCallbackRule;
import com.android.testutils.DevSdkIgnoreRule;
import org.junit.Before;
@ -67,7 +68,10 @@ import java.util.function.Supplier;
@RunWith(AndroidJUnit4.class)
@SdkSuppress(minSdkVersion = Build.VERSION_CODES.R) // BatteryStatsManager did not exist on Q
public class BatteryStatsManagerTest{
@Rule
@Rule(order = 1)
public final AutoReleaseNetworkCallbackRule
networkCallbackRule = new AutoReleaseNetworkCallbackRule();
@Rule(order = 2)
public final DevSdkIgnoreRule ignoreRule = new DevSdkIgnoreRule();
private static final String TAG = BatteryStatsManagerTest.class.getSimpleName();
private static final String TEST_URL = "https://connectivitycheck.gstatic.com/generate_204";
@ -145,7 +149,7 @@ public class BatteryStatsManagerTest{
return;
}
final Network cellNetwork = mCtsNetUtils.connectToCell();
final Network cellNetwork = networkCallbackRule.requestCell();
final URL url = new URL(TEST_URL);
// Get cellular battery stats

View file

@ -47,6 +47,7 @@ import androidx.test.runner.AndroidJUnit4
import com.android.modules.utils.build.SdkLevel.isAtLeastR
import com.android.net.module.util.NetworkStackConstants.TEST_CAPTIVE_PORTAL_HTTPS_URL
import com.android.net.module.util.NetworkStackConstants.TEST_CAPTIVE_PORTAL_HTTP_URL
import com.android.testutils.AutoReleaseNetworkCallbackRule
import com.android.testutils.DeviceConfigRule
import com.android.testutils.RecorderCallback.CallbackEntry.CapabilitiesChanged
import com.android.testutils.SkipMainlinePresubmit
@ -101,9 +102,12 @@ class CaptivePortalTest {
private val server = TestHttpServer("localhost")
@get:Rule
@get:Rule(order = 1)
val deviceConfigRule = DeviceConfigRule(retryCountBeforeSIfConfigChanged = 5)
@get:Rule(order = 2)
val networkCallbackRule = AutoReleaseNetworkCallbackRule()
companion object {
@JvmStatic @BeforeClass
fun setUpClass() {
@ -144,15 +148,15 @@ class CaptivePortalTest {
assumeTrue(pm.hasSystemFeature(FEATURE_WIFI))
assumeFalse(pm.hasSystemFeature(FEATURE_WATCH))
utils.ensureWifiConnected()
val cellNetwork = utils.connectToCell()
val cellNetwork = networkCallbackRule.requestCell()
// Verify cell network is validated
val cellReq = NetworkRequest.Builder()
.addTransportType(TRANSPORT_CELLULAR)
.addCapability(NET_CAPABILITY_INTERNET)
.build()
val cellCb = TestableNetworkCallback(timeoutMs = TEST_TIMEOUT_MS)
cm.registerNetworkCallback(cellReq, cellCb)
val cellCb = networkCallbackRule.registerNetworkCallback(cellReq,
TestableNetworkCallback(timeoutMs = TEST_TIMEOUT_MS))
val cb = cellCb.poll { it.network == cellNetwork &&
it is CapabilitiesChanged && it.caps.hasCapability(NET_CAPABILITY_VALIDATED)
}
@ -213,8 +217,6 @@ class CaptivePortalTest {
} finally {
cm.unregisterNetworkCallback(wifiCb)
server.stop()
// disconnectFromCell should be called after connectToCell
utils.disconnectFromCell()
}
}

View file

@ -191,6 +191,7 @@ import com.android.networkstack.apishim.ConnectivityManagerShimImpl;
import com.android.networkstack.apishim.ConstantsShim;
import com.android.networkstack.apishim.NetworkInformationShimImpl;
import com.android.networkstack.apishim.common.ConnectivityManagerShim;
import com.android.testutils.AutoReleaseNetworkCallbackRule;
import com.android.testutils.CompatUtil;
import com.android.testutils.ConnectivityModuleTest;
import com.android.testutils.DevSdkIgnoreRule;
@ -259,10 +260,14 @@ import fi.iki.elonen.NanoHTTPD.Response.Status;
@RunWith(AndroidJUnit4.class)
public class ConnectivityManagerTest {
@Rule
@Rule(order = 1)
public final DevSdkIgnoreRule ignoreRule = new DevSdkIgnoreRule();
@Rule
@Rule(order = 2)
public final AutoReleaseNetworkCallbackRule
networkCallbackRule = new AutoReleaseNetworkCallbackRule();
@Rule(order = 3)
public final DeviceConfigRule mTestValidationConfigRule = new DeviceConfigRule(
5 /* retryCountBeforeSIfConfigChanged */);
@ -411,11 +416,6 @@ public class ConnectivityManagerTest {
@After
public void tearDown() throws Exception {
// Release any NetworkRequests filed to connect mobile data.
if (mCtsNetUtils.cellConnectAttempted()) {
mCtsNetUtils.disconnectFromCell();
}
if (TestUtils.shouldTestSApis()) {
runWithShellPermissionIdentity(
() -> mCmShim.setRequireVpnForUids(false, mVpnRequiredUidRanges),
@ -555,7 +555,7 @@ public class ConnectivityManagerTest {
throws InterruptedException {
assumeTrue(mPackageManager.hasSystemFeature(FEATURE_TELEPHONY));
// Make sure cell is active to retrieve IMSI for verification in later step.
final Network cellNetwork = mCtsNetUtils.connectToCell();
final Network cellNetwork = networkCallbackRule.requestCell();
final String subscriberId = getSubscriberIdForCellNetwork(cellNetwork);
assertFalse(TextUtils.isEmpty(subscriberId));
@ -853,7 +853,7 @@ public class ConnectivityManagerTest {
assumeTrue(mPackageManager.hasSystemFeature(FEATURE_TELEPHONY));
Network wifiNetwork = mCtsNetUtils.ensureWifiConnected();
Network cellNetwork = mCtsNetUtils.connectToCell();
Network cellNetwork = networkCallbackRule.requestCell();
// This server returns the requestor's IP address as the response body.
URL url = new URL("http://google-ipv6test.appspot.com/ip.js?fmt=text");
String wifiAddressString = httpGet(wifiNetwork, url);
@ -2025,7 +2025,7 @@ public class ConnectivityManagerTest {
return;
}
final Network network = mCtsNetUtils.connectToCell();
final Network network = networkCallbackRule.requestCell();
final int supported = getSupportedKeepalivesForNet(network);
final InetAddress srcAddr = getFirstV4Address(network);
assumeTrue("This test requires native IPv4", srcAddr != null);
@ -2200,8 +2200,7 @@ public class ConnectivityManagerTest {
registerCallbackAndWaitForAvailable(makeWifiNetworkRequest(), wifiCb);
}
if (supportTelephony) {
// connectToCell needs to be followed by disconnectFromCell, which is called in tearDown
mCtsNetUtils.connectToCell();
networkCallbackRule.requestCell();
registerCallbackAndWaitForAvailable(makeCellNetworkRequest(), telephonyCb);
}
@ -2992,7 +2991,7 @@ public class ConnectivityManagerTest {
final TestableNetworkCallback wifiCb = new TestableNetworkCallback();
try {
// Ensure at least one default network candidate connected.
mCtsNetUtils.connectToCell();
networkCallbackRule.requestCell();
final Network wifiNetwork = prepareUnvalidatedNetwork();
// Default network should not be wifi ,but checking that wifi is not the default doesn't
@ -3034,7 +3033,7 @@ public class ConnectivityManagerTest {
allowBadWifi();
try {
final Network cellNetwork = mCtsNetUtils.connectToCell();
final Network cellNetwork = networkCallbackRule.requestCell();
final Network wifiNetwork = prepareValidatedNetwork();
registerDefaultNetworkCallback(defaultCb);
@ -3214,8 +3213,6 @@ public class ConnectivityManagerTest {
if (supportWifi) {
mCtsNetUtils.ensureWifiDisconnected(null /* wifiNetworkToCheck */);
} else {
mCtsNetUtils.disconnectFromCell();
}
final CompletableFuture<Boolean> future = new CompletableFuture<>();
@ -3226,7 +3223,7 @@ public class ConnectivityManagerTest {
if (supportWifi) {
mCtsNetUtils.ensureWifiConnected();
} else {
mCtsNetUtils.connectToCell();
networkCallbackRule.requestCell();
}
assertTrue(future.get(LISTEN_ACTIVITY_TIMEOUT_MS, TimeUnit.MILLISECONDS));
}, () -> {
@ -3267,7 +3264,7 @@ public class ConnectivityManagerTest {
// For testing mobile data preferred uids feature, it needs both wifi and cell network.
final Network wifiNetwork = mCtsNetUtils.ensureWifiConnected();
final Network cellNetwork = mCtsNetUtils.connectToCell();
final Network cellNetwork = networkCallbackRule.requestCell();
final TestableNetworkCallback defaultTrackingCb = new TestableNetworkCallback();
final TestableNetworkCallback systemDefaultCb = new TestableNetworkCallback();
final Handler h = new Handler(Looper.getMainLooper());

View file

@ -43,9 +43,9 @@ import android.util.ArraySet;
import androidx.test.ext.junit.runners.AndroidJUnit4;
import androidx.test.platform.app.InstrumentationRegistry;
import com.android.testutils.AutoReleaseNetworkCallbackRule;
import com.android.testutils.DeviceConfigRule;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
@ -55,9 +55,13 @@ import java.util.Set;
@RunWith(AndroidJUnit4.class)
public class MultinetworkApiTest {
@Rule
@Rule(order = 1)
public final DeviceConfigRule mDeviceConfigRule = new DeviceConfigRule();
@Rule(order = 2)
public final AutoReleaseNetworkCallbackRule
mNetworkCallbackRule = new AutoReleaseNetworkCallbackRule();
static {
System.loadLibrary("nativemultinetwork_jni");
}
@ -93,13 +97,6 @@ public class MultinetworkApiTest {
mCtsNetUtils = new CtsNetUtils(mContext);
}
@After
public void tearDown() {
if (mCtsNetUtils.cellConnectAttempted()) {
mCtsNetUtils.disconnectFromCell();
}
}
@Test
public void testGetaddrinfo() throws Exception {
for (Network network : getTestableNetworks()) {
@ -274,8 +271,8 @@ public class MultinetworkApiTest {
// Network).
final Set<Network> testableNetworks = new ArraySet<>();
if (mContext.getPackageManager().hasSystemFeature(FEATURE_TELEPHONY)) {
if (!mCtsNetUtils.cellConnectAttempted()) {
mRequestedCellNetwork = mCtsNetUtils.connectToCell();
if (mRequestedCellNetwork == null) {
mRequestedCellNetwork = mNetworkCallbackRule.requestCell();
}
assertNotNull("Cell network requested but not obtained", mRequestedCellNetwork);
testableNetworks.add(mRequestedCellNetwork);

View file

@ -408,40 +408,10 @@ public final class CtsNetUtils {
return network;
}
public Network connectToCell() throws InterruptedException {
if (cellConnectAttempted()) {
mCm.unregisterNetworkCallback(mCellNetworkCallback);
}
NetworkRequest cellRequest = new NetworkRequest.Builder()
.addTransportType(TRANSPORT_CELLULAR)
.addCapability(NET_CAPABILITY_INTERNET)
.build();
mCellNetworkCallback = new TestNetworkCallback();
mCm.requestNetwork(cellRequest, mCellNetworkCallback);
final Network cellNetwork = mCellNetworkCallback.waitForAvailable();
assertNotNull("Cell network not available. " +
"Please ensure the device has working mobile data.", cellNetwork);
return cellNetwork;
}
public void disconnectFromCell() {
if (!cellConnectAttempted()) {
throw new IllegalStateException("Cell connection not attempted");
}
mCm.unregisterNetworkCallback(mCellNetworkCallback);
mCellNetworkCallback = null;
}
public boolean cellConnectAttempted() {
return mCellNetworkCallback != null;
}
public void tearDown() {
if (cellConnectAttempted()) {
disconnectFromCell();
}
}
private NetworkRequest makeWifiNetworkRequest() {
return new NetworkRequest.Builder()
.addTransportType(NetworkCapabilities.TRANSPORT_WIFI)