From a5f724dfdd751cdc9ee1e60a2d77cc30b81a01bc Mon Sep 17 00:00:00 2001 From: James Mattis Date: Tue, 28 Dec 2021 15:04:44 -0800 Subject: [PATCH] Changes to make eth service methods more testable Updates to make methods in EthernetServiceImpl that rely on EthernetTracker unit testable. This CL also includes added tests for such methods in EthernetServiceImplTest. Bug: 210485380 Test: atest EthernetServiceTests Change-Id: I63969b60cc4cf9d391e2cd21d02e1bdc8988aba8 --- .../server/ethernet/EthernetService.java | 11 +++- .../server/ethernet/EthernetServiceImpl.java | 20 +++----- .../server/ethernet/EthernetTracker.java | 10 ++-- .../ethernet/EthernetServiceImplTest.java | 51 ++++++++++++++++--- 4 files changed, 68 insertions(+), 24 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetService.java b/service-t/src/com/android/server/ethernet/EthernetService.java index 2448146ed5..467ab677d8 100644 --- a/service-t/src/com/android/server/ethernet/EthernetService.java +++ b/service-t/src/com/android/server/ethernet/EthernetService.java @@ -17,17 +17,24 @@ package com.android.server.ethernet; import android.content.Context; +import android.os.Handler; +import android.os.HandlerThread; import android.util.Log; import com.android.server.SystemService; public final class EthernetService extends SystemService { private static final String TAG = "EthernetService"; - final EthernetServiceImpl mImpl; + private static final String THREAD_NAME = "EthernetServiceThread"; + private final EthernetServiceImpl mImpl; public EthernetService(Context context) { super(context); - mImpl = new EthernetServiceImpl(context); + final HandlerThread handlerThread = new HandlerThread(THREAD_NAME); + handlerThread.start(); + mImpl = new EthernetServiceImpl( + context, handlerThread.getThreadHandler(), + new EthernetTracker(context, handlerThread.getThreadHandler())); } @Override diff --git a/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java b/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java index fd690b5547..3479ea4722 100644 --- a/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java +++ b/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java @@ -28,7 +28,6 @@ import android.net.InternalNetworkUpdateRequest; import android.net.IpConfiguration; import android.os.Binder; import android.os.Handler; -import android.os.HandlerThread; import android.os.RemoteException; import android.util.Log; import android.util.PrintWriterPrinter; @@ -49,15 +48,17 @@ import java.util.concurrent.atomic.AtomicBoolean; public class EthernetServiceImpl extends IEthernetManager.Stub { private static final String TAG = "EthernetServiceImpl"; - private final Context mContext; @VisibleForTesting final AtomicBoolean mStarted = new AtomicBoolean(false); + private final Context mContext; + private final Handler mHandler; + private final EthernetTracker mTracker; - private Handler mHandler; - private EthernetTracker mTracker; - - public EthernetServiceImpl(Context context) { + EthernetServiceImpl(@NonNull final Context context, @NonNull final Handler handler, + @NonNull final EthernetTracker tracker) { mContext = context; + mHandler = handler; + mTracker = tracker; } private void enforceAccessPermission() { @@ -91,14 +92,7 @@ public class EthernetServiceImpl extends IEthernetManager.Stub { public void start() { Log.i(TAG, "Starting Ethernet service"); - - HandlerThread handlerThread = new HandlerThread("EthernetServiceThread"); - handlerThread.start(); - mHandler = new Handler(handlerThread.getLooper()); - - mTracker = new EthernetTracker(mContext, mHandler); mTracker.start(); - mStarted.set(true); } diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index 65c3cefe0d..9660194508 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -18,6 +18,8 @@ package com.android.server.ethernet; import static android.net.TestNetworkManager.TEST_TAP_PREFIX; +import static com.android.internal.annotations.VisibleForTesting.Visibility.PACKAGE; + import android.annotation.NonNull; import android.annotation.Nullable; import android.content.Context; @@ -67,7 +69,8 @@ import java.util.concurrent.ConcurrentHashMap; * *

All public or package private methods must be thread-safe unless stated otherwise. */ -final class EthernetTracker { +@VisibleForTesting(visibility = PACKAGE) +public class EthernetTracker { private static final int INTERFACE_MODE_CLIENT = 1; private static final int INTERFACE_MODE_SERVER = 2; @@ -138,10 +141,10 @@ final class EthernetTracker { NetworkCapabilities nc = createNetworkCapabilities(true /* clear default capabilities */); mFactory = new EthernetNetworkFactory(handler, context, nc); - mFactory.register(); } void start() { + mFactory.register(); mConfigStore.read(); // Default interface is just the first one we want to track. @@ -176,7 +179,8 @@ final class EthernetTracker { return mIpConfigurations.get(iface); } - boolean isTrackingInterface(String iface) { + @VisibleForTesting(visibility = PACKAGE) + protected boolean isTrackingInterface(String iface) { return mFactory.hasInterface(iface); } diff --git a/tests/ethernet/java/com/android/server/ethernet/EthernetServiceImplTest.java b/tests/ethernet/java/com/android/server/ethernet/EthernetServiceImplTest.java index 9869b82cb2..516e574522 100644 --- a/tests/ethernet/java/com/android/server/ethernet/EthernetServiceImplTest.java +++ b/tests/ethernet/java/com/android/server/ethernet/EthernetServiceImplTest.java @@ -20,11 +20,13 @@ import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.doReturn; +import android.annotation.NonNull; import android.content.Context; import android.content.pm.PackageManager; import android.net.InternalNetworkUpdateRequest; import android.net.IpConfiguration; import android.net.StaticIpConfiguration; +import android.os.Handler; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; @@ -38,16 +40,21 @@ import org.mockito.MockitoAnnotations; @RunWith(AndroidJUnit4.class) @SmallTest public class EthernetServiceImplTest { + private static final String TEST_IFACE = "test123"; private EthernetServiceImpl mEthernetServiceImpl; @Mock private Context mContext; + @Mock private Handler mHandler; + @Mock private EthernetTracker mEthernetTracker; @Mock private PackageManager mPackageManager; @Before public void setup() { MockitoAnnotations.initMocks(this); doReturn(mPackageManager).when(mContext).getPackageManager(); - mEthernetServiceImpl = new EthernetServiceImpl(mContext); + mEthernetServiceImpl = new EthernetServiceImpl(mContext, mHandler, mEthernetTracker); mEthernetServiceImpl.mStarted.set(true); + toggleAutomotiveFeature(true); + shouldTrackIface(TEST_IFACE, true); } @Test @@ -111,7 +118,7 @@ public class EthernetServiceImplTest { @Test public void testUpdateConfigurationRejectsWithoutAutomotiveFeature() { - disableAutomotiveFeature(); + toggleAutomotiveFeature(false); assertThrows(UnsupportedOperationException.class, () -> { final InternalNetworkUpdateRequest r = new InternalNetworkUpdateRequest(new StaticIpConfiguration(), null); @@ -122,7 +129,7 @@ public class EthernetServiceImplTest { @Test public void testConnectNetworkRejectsWithoutAutomotiveFeature() { - disableAutomotiveFeature(); + toggleAutomotiveFeature(false); assertThrows(UnsupportedOperationException.class, () -> { mEthernetServiceImpl.connectNetwork("" /* iface */, null /* listener */); }); @@ -130,13 +137,45 @@ public class EthernetServiceImplTest { @Test public void testDisconnectNetworkRejectsWithoutAutomotiveFeature() { - disableAutomotiveFeature(); + toggleAutomotiveFeature(false); assertThrows(UnsupportedOperationException.class, () -> { mEthernetServiceImpl.disconnectNetwork("" /* iface */, null /* listener */); }); } - private void disableAutomotiveFeature() { - doReturn(false).when(mPackageManager).hasSystemFeature(PackageManager.FEATURE_AUTOMOTIVE); + private void toggleAutomotiveFeature(final boolean isEnabled) { + doReturn(isEnabled) + .when(mPackageManager).hasSystemFeature(PackageManager.FEATURE_AUTOMOTIVE); + } + + @Test + public void testUpdateConfigurationRejectsWithUntrackedIface() { + shouldTrackIface(TEST_IFACE, false); + assertThrows(UnsupportedOperationException.class, () -> { + final InternalNetworkUpdateRequest r = + new InternalNetworkUpdateRequest(new StaticIpConfiguration(), null); + + mEthernetServiceImpl.updateConfiguration(TEST_IFACE, r, null /* listener */); + }); + } + + @Test + public void testConnectNetworkRejectsWithUntrackedIface() { + shouldTrackIface(TEST_IFACE, false); + assertThrows(UnsupportedOperationException.class, () -> { + mEthernetServiceImpl.connectNetwork(TEST_IFACE, null /* listener */); + }); + } + + @Test + public void testDisconnectNetworkRejectsWithUntrackedIface() { + shouldTrackIface(TEST_IFACE, false); + assertThrows(UnsupportedOperationException.class, () -> { + mEthernetServiceImpl.disconnectNetwork(TEST_IFACE, null /* listener */); + }); + } + + private void shouldTrackIface(@NonNull final String iface, final boolean shouldTrack) { + doReturn(shouldTrack).when(mEthernetTracker).isTrackingInterface(iface); } }