From 88692dc7c6758f442d89d21e22b5be6ca5140f1b Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Tue, 31 May 2022 07:20:54 -0700 Subject: [PATCH 1/4] ethernet: increase timeout to deflake tests The up / up / down / up issue causes ethernet tests to fail as createInterface() uses eventually expect to wait on the down callback. On the test infrastructure, that callback often does not arrive within 1000ms. I have seen this failure locally, although in less than 1% of all test runs. Bug: 234314411 Bug: 227008268 Test: atest EthernetManagerTest Change-Id: I26929c47cfc557047e7987a920a07af4e1bd55d6 --- tests/cts/net/src/android/net/cts/EthernetManagerTest.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt index 0a02593c87..1c95fbed6c 100644 --- a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt +++ b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt @@ -71,7 +71,9 @@ import kotlin.test.assertNull import kotlin.test.assertTrue import kotlin.test.fail -private const val TIMEOUT_MS = 1000L +// TODO: try to lower this timeout in the future. Currently, ethernet tests are still flaky because +// the interface is not ready fast enough (mostly due to the up / up / down / up issue). +private const val TIMEOUT_MS = 2000L private const val NO_CALLBACK_TIMEOUT_MS = 200L private val DEFAULT_IP_CONFIGURATION = IpConfiguration(IpConfiguration.IpAssignment.DHCP, IpConfiguration.ProxySettings.NONE, null, null) From f33f276b291d8b24b680a02f64404fd1c94186ad Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Wed, 1 Jun 2022 21:06:42 -0700 Subject: [PATCH 2/4] ethernet: broadcast state change for server interfaces Also update the test to ensure mFactory.hasInterface is mocked to match mFactory.getInterfaceState: previously it would be called by the factory in the mocked getInterfaceState, but the new code calls it directly. Bug: 171872016 Test: atest EthernetManagerTest Change-Id: I9ac959f181c88a7992991923b98a836f2833fa88 --- .../server/ethernet/EthernetTracker.java | 31 +++++++++++++++---- .../server/ethernet/EthernetTrackerTest.java | 19 ++++++++++-- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index e5f1ea534b..4da7300a8a 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -228,7 +228,7 @@ public class EthernetTracker { */ protected void broadcastInterfaceStateChange(@NonNull String iface) { ensureRunningOnEthernetServiceThread(); - final int state = mFactory.getInterfaceState(iface); + final int state = getInterfaceState(iface); final int role = getInterfaceRole(iface); final IpConfiguration config = getIpConfigurationForCallback(iface, state); final int n = mListeners.beginBroadcast(); @@ -435,15 +435,34 @@ public class EthernetTracker { if (mDefaultInterface != null) { removeInterface(mDefaultInterface); addInterface(mDefaultInterface); + // when this broadcast is sent, any calls to notifyTetheredInterfaceAvailable or + // notifyTetheredInterfaceUnavailable have already happened + broadcastInterfaceStateChange(mDefaultInterface); } } + private int getInterfaceState(final String iface) { + if (mFactory.hasInterface(iface)) { + return mFactory.getInterfaceState(iface); + } + if (getInterfaceMode(iface) == INTERFACE_MODE_SERVER) { + // server mode interfaces are not tracked by the factory. + // TODO(b/234743836): interface state for server mode interfaces is not tracked + // properly; just return link up. + return EthernetManager.STATE_LINK_UP; + } + return EthernetManager.STATE_ABSENT; + } + private int getInterfaceRole(final String iface) { - if (!mFactory.hasInterface(iface)) return EthernetManager.ROLE_NONE; - final int mode = getInterfaceMode(iface); - return (mode == INTERFACE_MODE_CLIENT) - ? EthernetManager.ROLE_CLIENT - : EthernetManager.ROLE_SERVER; + if (mFactory.hasInterface(iface)) { + // only client mode interfaces are tracked by the factory. + return EthernetManager.ROLE_CLIENT; + } + if (getInterfaceMode(iface) == INTERFACE_MODE_SERVER) { + return EthernetManager.ROLE_SERVER; + } + return EthernetManager.ROLE_NONE; } private int getInterfaceMode(final String iface) { diff --git a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java index 33b36fdeee..93789cabcb 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java @@ -28,6 +28,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; @@ -67,6 +68,7 @@ import org.mockito.MockitoAnnotations; import java.net.InetAddress; import java.util.ArrayList; +import java.util.concurrent.atomic.AtomicBoolean; @SmallTest @RunWith(AndroidJUnit4.class) @@ -432,7 +434,20 @@ public class EthernetTrackerTest { when(mNetd.interfaceGetList()).thenReturn(new String[] {testIface}); when(mNetd.interfaceGetCfg(eq(testIface))).thenReturn(ifaceParcel); doReturn(new String[] {testIface}).when(mFactory).getAvailableInterfaces(anyBoolean()); - doReturn(EthernetManager.STATE_LINK_UP).when(mFactory).getInterfaceState(eq(testIface)); + + final AtomicBoolean ifaceUp = new AtomicBoolean(true); + doAnswer(inv -> ifaceUp.get()).when(mFactory).hasInterface(testIface); + doAnswer(inv -> + ifaceUp.get() ? EthernetManager.STATE_LINK_UP : EthernetManager.STATE_ABSENT) + .when(mFactory).getInterfaceState(testIface); + doAnswer(inv -> { + ifaceUp.set(true); + return null; + }).when(mFactory).addInterface(eq(testIface), eq(testHwAddr), any(), any()); + doAnswer(inv -> { + ifaceUp.set(false); + return null; + }).when(mFactory).removeInterface(testIface); final EthernetStateListener listener = spy(new EthernetStateListener()); tracker.addListener(listener, true /* canUseRestrictedNetworks */); @@ -443,7 +458,6 @@ public class EthernetTrackerTest { verify(listener).onEthernetStateChanged(eq(EthernetManager.ETHERNET_STATE_ENABLED)); reset(listener); - doReturn(EthernetManager.STATE_ABSENT).when(mFactory).getInterfaceState(eq(testIface)); tracker.setEthernetEnabled(false); waitForIdle(); verify(mFactory).removeInterface(eq(testIface)); @@ -452,7 +466,6 @@ public class EthernetTrackerTest { anyInt(), any()); reset(listener); - doReturn(EthernetManager.STATE_LINK_UP).when(mFactory).getInterfaceState(eq(testIface)); tracker.setEthernetEnabled(true); waitForIdle(); verify(mFactory).addInterface(eq(testIface), eq(testHwAddr), any(), any()); From b4a86b9181cfd51cb5285de4575a2c8cdaee3326 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Wed, 1 Jun 2022 22:09:33 -0700 Subject: [PATCH 3/4] ethernet: stop using EthernetManager shims Also update the runner to DevSdkIgnoreRunner, as the default runner would crash trying to load referenced classes like InterfaceStateListener on S: they are only part of the updatable bootclasspath in T+. Bug: 171872016 Test: atest EthernetManagerTest Change-Id: Ia38f3b86a70ccfd06a8c17392fbbb1586ed6bb7e --- .../android/net/cts/EthernetManagerTest.kt | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt index 1c95fbed6c..ba54df78df 100644 --- a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt +++ b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt @@ -20,6 +20,14 @@ import android.Manifest.permission.MANAGE_TEST_NETWORKS import android.Manifest.permission.NETWORK_SETTINGS import android.content.Context import android.net.ConnectivityManager +import android.net.EthernetManager +import android.net.EthernetManager.InterfaceStateListener +import android.net.EthernetManager.ROLE_CLIENT +import android.net.EthernetManager.ROLE_NONE +import android.net.EthernetManager.ROLE_SERVER +import android.net.EthernetManager.STATE_ABSENT +import android.net.EthernetManager.STATE_LINK_DOWN +import android.net.EthernetManager.STATE_LINK_UP import android.net.EthernetNetworkSpecifier import android.net.InetAddresses import android.net.IpConfiguration @@ -32,35 +40,27 @@ import android.net.NetworkRequest import android.net.TestNetworkInterface import android.net.TestNetworkManager import android.net.cts.EthernetManagerTest.EthernetStateListener.CallbackEntry.InterfaceStateChanged +import android.os.Build import android.os.Handler import android.os.HandlerExecutor import android.os.Looper import android.platform.test.annotations.AppModeFull import android.util.ArraySet import androidx.test.platform.app.InstrumentationRegistry -import androidx.test.runner.AndroidJUnit4 import com.android.net.module.util.ArrayTrackRecord import com.android.net.module.util.TrackRecord -import com.android.networkstack.apishim.EthernetManagerShimImpl -import com.android.networkstack.apishim.common.EthernetManagerShim.InterfaceStateListener -import com.android.networkstack.apishim.common.EthernetManagerShim.ROLE_CLIENT -import com.android.networkstack.apishim.common.EthernetManagerShim.ROLE_NONE -import com.android.networkstack.apishim.common.EthernetManagerShim.STATE_ABSENT -import com.android.networkstack.apishim.common.EthernetManagerShim.STATE_LINK_DOWN -import com.android.networkstack.apishim.common.EthernetManagerShim.STATE_LINK_UP import com.android.testutils.anyNetwork import com.android.testutils.DevSdkIgnoreRule +import com.android.testutils.DevSdkIgnoreRunner import com.android.testutils.RecorderCallback.CallbackEntry.Available import com.android.testutils.RecorderCallback.CallbackEntry.Lost import com.android.testutils.RouterAdvertisementResponder -import com.android.testutils.SC_V2 import com.android.testutils.TapPacketReader import com.android.testutils.TestableNetworkCallback import com.android.testutils.runAsShell import com.android.testutils.waitForIdle import org.junit.After import org.junit.Before -import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import java.net.Inet6Address @@ -84,14 +84,13 @@ private val ETH_REQUEST: NetworkRequest = NetworkRequest.Builder() .build() @AppModeFull(reason = "Instant apps can't access EthernetManager") -@RunWith(AndroidJUnit4::class) +// EthernetManager is not updatable before T, so tests do not need to be backwards compatible. +@RunWith(DevSdkIgnoreRunner::class) +@DevSdkIgnoreRule.IgnoreUpTo(Build.VERSION_CODES.S_V2) class EthernetManagerTest { - // EthernetManager is not updatable before T, so tests do not need to be backwards compatible - @get:Rule - val ignoreRule = DevSdkIgnoreRule(ignoreClassUpTo = SC_V2) private val context by lazy { InstrumentationRegistry.getInstrumentation().context } - private val em by lazy { EthernetManagerShimImpl.newInstance(context) } + private val em by lazy { context.getSystemService(EthernetManager::class.java) } private val cm by lazy { context.getSystemService(ConnectivityManager::class.java) } private val ifaceListener = EthernetStateListener() From 843cd2cf32c85448d52d65672a14cf5adbd9afa4 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Wed, 1 Jun 2022 22:18:11 -0700 Subject: [PATCH 4/4] ethernet: add test for tethered interface callbacks Bug: 171872016 Test: atest EthernetManagerTest Change-Id: I0bda01265bb8441d1229e63fc957db4e52b15ccb --- .../android/net/cts/EthernetManagerTest.kt | 97 ++++++++++++++++++- 1 file changed, 92 insertions(+), 5 deletions(-) diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt index ba54df78df..db24b4493b 100644 --- a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt +++ b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt @@ -28,6 +28,8 @@ import android.net.EthernetManager.ROLE_SERVER import android.net.EthernetManager.STATE_ABSENT import android.net.EthernetManager.STATE_LINK_DOWN import android.net.EthernetManager.STATE_LINK_UP +import android.net.EthernetManager.TetheredInterfaceCallback +import android.net.EthernetManager.TetheredInterfaceRequest import android.net.EthernetNetworkSpecifier import android.net.InetAddresses import android.net.IpConfiguration @@ -44,6 +46,7 @@ import android.os.Build import android.os.Handler import android.os.HandlerExecutor import android.os.Looper +import android.os.SystemProperties import android.platform.test.annotations.AppModeFull import android.util.ArraySet import androidx.test.platform.app.InstrumentationRegistry @@ -60,11 +63,16 @@ import com.android.testutils.TestableNetworkCallback import com.android.testutils.runAsShell import com.android.testutils.waitForIdle import org.junit.After +import org.junit.Assume.assumeFalse import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import java.net.Inet6Address +import java.util.concurrent.CompletableFuture +import java.util.concurrent.ExecutionException +import java.util.concurrent.TimeUnit import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull @@ -98,6 +106,8 @@ class EthernetManagerTest { private val addedListeners = ArrayList() private val networkRequests = ArrayList() + private var tetheredInterfaceRequest: TetheredInterfaceRequest? = null + private class EthernetTestInterface( context: Context, private val handler: Handler @@ -162,11 +172,11 @@ class EthernetManagerTest { } fun expectCallback(iface: EthernetTestInterface, state: Int, role: Int) { - expectCallback(createChangeEvent(iface, state, role)) + expectCallback(createChangeEvent(iface.interfaceName, state, role)) } - fun createChangeEvent(iface: EthernetTestInterface, state: Int, role: Int) = - InterfaceStateChanged(iface.interfaceName, state, role, + fun createChangeEvent(iface: String, state: Int, role: Int) = + InterfaceStateChanged(iface, state, role, if (state != STATE_ABSENT) DEFAULT_IP_CONFIGURATION else null) fun pollForNextCallback(): CallbackEntry { @@ -175,8 +185,12 @@ class EthernetManagerTest { fun eventuallyExpect(expected: CallbackEntry) = events.poll(TIMEOUT_MS) { it == expected } + fun eventuallyExpect(interfaceName: String, state: Int, role: Int) { + assertNotNull(eventuallyExpect(createChangeEvent(interfaceName, state, role))) + } + fun eventuallyExpect(iface: EthernetTestInterface, state: Int, role: Int) { - assertNotNull(eventuallyExpect(createChangeEvent(iface, state, role))) + eventuallyExpect(iface.interfaceName, state, role) } fun assertNoCallback() { @@ -185,6 +199,34 @@ class EthernetManagerTest { } } + private class TetheredInterfaceListener : TetheredInterfaceCallback { + private val available = CompletableFuture() + + override fun onAvailable(iface: String) { + available.complete(iface) + } + + override fun onUnavailable() { + available.completeExceptionally(IllegalStateException("onUnavailable was called")) + } + + fun expectOnAvailable(): String { + return available.get(TIMEOUT_MS, TimeUnit.MILLISECONDS) + } + + fun expectOnUnavailable() { + // Assert that the future fails with the IllegalStateException from the + // completeExceptionally() call inside onUnavailable. + assertFailsWith(IllegalStateException::class) { + try { + available.get(TIMEOUT_MS, TimeUnit.MILLISECONDS) + } catch (e: ExecutionException) { + throw e.cause!! + } + } + } + } + @Before fun setUp() { setIncludeTestInterfaces(true) @@ -202,6 +244,7 @@ class EthernetManagerTest { em.removeInterfaceStateListener(listener) } networkRequests.forEach { cm.unregisterNetworkCallback(it) } + releaseTetheredInterface() } private fun addInterfaceStateListener(listener: EthernetStateListener) { @@ -248,6 +291,19 @@ class EthernetManagerTest { networkRequests.remove(cb) } + private fun requestTetheredInterface() = TetheredInterfaceListener().also { + tetheredInterfaceRequest = runAsShell(NETWORK_SETTINGS) { + em.requestTetheredInterface(HandlerExecutor(Handler(Looper.getMainLooper())), it) + } + } + + private fun releaseTetheredInterface() { + runAsShell(NETWORK_SETTINGS) { + tetheredInterfaceRequest?.release() + tetheredInterfaceRequest = null + } + } + private fun NetworkRequest.createCopyWithEthernetSpecifier(ifaceName: String) = NetworkRequest.Builder(NetworkRequest(ETH_REQUEST)) .setNetworkSpecifier(EthernetNetworkSpecifier(ifaceName)).build() @@ -300,6 +356,34 @@ class EthernetManagerTest { } } + // TODO: this function is now used in two places (EthernetManagerTest and + // EthernetTetheringTest), so it should be moved to testutils. + private fun isAdbOverNetwork(): Boolean { + // If adb TCP port opened, this test may running by adb over network. + return (SystemProperties.getInt("persist.adb.tcp.port", -1) > -1 || + SystemProperties.getInt("service.adb.tcp.port", -1) > -1) + } + + @Test + fun testCallbacks_forServerModeInterfaces() { + // do not run this test when adb might be connected over ethernet. + assumeFalse(isAdbOverNetwork()) + + val listener = EthernetStateListener() + addInterfaceStateListener(listener) + + // it is possible that a physical interface is present, so it is not guaranteed that iface + // will be put into server mode. This should not matter for the test though. Calling + // createInterface() makes sure we have at least one interface available. + val iface = createInterface() + val cb = requestTetheredInterface() + val ifaceName = cb.expectOnAvailable() + listener.eventuallyExpect(ifaceName, STATE_LINK_UP, ROLE_SERVER) + + releaseTetheredInterface() + listener.eventuallyExpect(ifaceName, STATE_LINK_UP, ROLE_CLIENT) + } + /** * Validate all interfaces are returned for an EthernetStateListener upon registration. */ @@ -315,7 +399,10 @@ class EthernetManagerTest { assertTrue(ifaces.contains(iface), "Untracked interface $iface returned") // If the event's iface was created in the test, additional criteria can be validated. createdIfaces.find { it.interfaceName.equals(iface) }?.let { - assertEquals(event, listener.createChangeEvent(it, STATE_LINK_UP, ROLE_CLIENT)) + assertEquals(event, + listener.createChangeEvent(it.interfaceName, + STATE_LINK_UP, + ROLE_CLIENT)) } } // Assert all callbacks are accounted for.