From 36ef8f079114f65d9a74299fd5d6ddf5251bb9eb Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Sat, 4 Feb 2023 01:12:00 +0000 Subject: [PATCH] Improved ExtendedMockitoTestCase to make it more error prone. - Make sure session is properly closed, as Mockito keeps static references to the sessions (and when one test fails to close a session, all subsequent tests would fail). - Clear inline mocks after each test to avoid OOM (see b/259280359 and https://github.com/mockito/mockito/issues/1614) - Convert AlarmManagerServiceTest to extend ExtendedMockitoTestCase - Also disabled CachedAppOptimizerTest, which was failing (without this change and for months - b/226641572) and cannot use ExtendedMockitoTestCase "as is" because it uses a @Rule that also starts a mockito session. Notice that to fully take advantage of these improvements, all classes should extend ExtendedMockitoTestCase; this CL just changed a few classes so the suite finishes without crashing with OOM and UserVisibilityMediatorMUMDTest doesn't fail due to other tests not closing the session. NOTE: these improvements already exist on AbstractExtendedMockitoTestCase (from android.car.test.mocks) which is what ExtendedMockitoTestCase is based on. Test: atest FrameworksMockingServicesTests Fixes: 267765119 Bug: 226641572 Change-Id: I2c0abe930521d9babda90c393003d8730fa67e96 --- .../android/server/DumpableDumperRule.java | 14 +- .../server/ExtendedMockitoTestCase.java | 179 ++++++++++++++++-- .../server/alarm/AlarmManagerServiceTest.java | 56 +++--- .../server/am/CachedAppOptimizerTest.java | 3 + 4 files changed, 207 insertions(+), 45 deletions(-) diff --git a/services/tests/mockingservicestests/src/com/android/server/DumpableDumperRule.java b/services/tests/mockingservicestests/src/com/android/server/DumpableDumperRule.java index a0687f6c1ba96..6e6d55b275ab7 100644 --- a/services/tests/mockingservicestests/src/com/android/server/DumpableDumperRule.java +++ b/services/tests/mockingservicestests/src/com/android/server/DumpableDumperRule.java @@ -15,6 +15,7 @@ */ package com.android.server; +import android.annotation.Nullable; import android.util.Dumpable; import android.util.Log; @@ -40,6 +41,8 @@ public final class DumpableDumperRule implements TestRule { private final List mDumpables = new ArrayList<>(); + private @Nullable String mTestName; + /** * Adds a {@link Dumpable} to be logged if the test case fails. */ @@ -47,15 +50,24 @@ public final class DumpableDumperRule implements TestRule { mDumpables.add(dumpable); } + /** + * Gets the name of the test being executed. + */ + public @Nullable String getTestName() { + return mTestName; + } + @Override public Statement apply(Statement base, Description description) { return new Statement() { @Override public void evaluate() throws Throwable { + mTestName = description.getDisplayName(); try { base.evaluate(); } catch (Throwable t) { - dumpOnFailure(description.getMethodName()); + dumpOnFailure(mTestName); + mTestName = null; throw t; } } diff --git a/services/tests/mockingservicestests/src/com/android/server/ExtendedMockitoTestCase.java b/services/tests/mockingservicestests/src/com/android/server/ExtendedMockitoTestCase.java index 693a96dd66186..48483a14a8a03 100644 --- a/services/tests/mockingservicestests/src/com/android/server/ExtendedMockitoTestCase.java +++ b/services/tests/mockingservicestests/src/com/android/server/ExtendedMockitoTestCase.java @@ -17,6 +17,7 @@ package com.android.server; import static com.android.dx.mockito.inline.extended.ExtendedMockito.mockitoSession; +import android.annotation.Nullable; import android.util.Log; import com.android.dx.mockito.inline.extended.StaticMockitoSessionBuilder; @@ -28,9 +29,12 @@ import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.rules.RuleChain; +import org.mockito.Mockito; import org.mockito.MockitoSession; import org.mockito.quality.Strictness; +import java.lang.reflect.Constructor; + /** * Base class to make it easier to write tests that uses {@code ExtendedMockito}. * @@ -39,7 +43,25 @@ public abstract class ExtendedMockitoTestCase { private static final String TAG = ExtendedMockitoTestCase.class.getSimpleName(); - private static final boolean DEBUG = Log.isLoggable(TAG, Log.DEBUG); + private static final boolean VERBOSE = Log.isLoggable(TAG, Log.VERBOSE); + + /** + * Number of invocations, used to force a failure on {@link #forceFailure(int, Class, String)}. + */ + private static int sInvocationsCounter; + + /** + * Sessions follow the "Highlander Rule": There can be only one! + * + *

So, we keep track of that and force-close it if needed. + */ + @Nullable + private static MockitoSession sHighlanderSession; + + /** + * Points to where the current session was created. + */ + private static Exception sSessionCreationLocation; private MockitoSession mSession; @@ -51,16 +73,43 @@ public abstract class ExtendedMockitoTestCase { .outerRule(mDumpableDumperRule) .around(mExpect); + public ExtendedMockitoTestCase() { + sInvocationsCounter++; + } + @Before - public void startSession() { - if (DEBUG) { - Log.d(TAG, "startSession()"); + public final void startSession() { + if (VERBOSE) { + Log.v(TAG, "startSession() for " + getTestName() + " on thread " + + Thread.currentThread() + "; sHighlanderSession=" + sHighlanderSession); } + createSessionLocation(); + finishHighlanderSessionIfNeeded("startSession()"); StaticMockitoSessionBuilder builder = mockitoSession() .initMocks(this) - .strictness(Strictness.LENIENT); + .strictness(getSessionStrictness()); initializeSession(builder); - mSession = builder.startMocking(); + sHighlanderSession = mSession = builder.startMocking(); + } + + private void createSessionLocation() { + try { + sSessionCreationLocation = new Exception(getTestName()); + } catch (Exception e) { + // Better safe than sorry... + Log.e(TAG, "Could not create sSessionCreationLocation with " + getTestName() + + " on thread " + Thread.currentThread(), e); + sSessionCreationLocation = e; + } + } + + /** + * Gets the session strictness. + * + * @return {@link Strictness.LENIENT} by default; subclasses can override. + */ + protected Strictness getSessionStrictness() { + return Strictness.LENIENT; } /** @@ -69,29 +118,127 @@ public abstract class ExtendedMockitoTestCase { *

Typically used to define which classes should have static methods mocked or spied. */ protected void initializeSession(StaticMockitoSessionBuilder builder) { - if (DEBUG) { - Log.d(TAG, "initializeSession()"); + if (VERBOSE) { + Log.v(TAG, "initializeSession()"); } } @After - public final void finishSession() { + public final void finishSession() throws Exception { + if (false) { // For obvious reasons, should NEVER be merged as true + forceFailure(1, RuntimeException.class, "to simulate an unfinished session"); + } + + // mSession.finishMocking() must ALWAYS be called (hence the over-protective try/finally + // statements), otherwise it would cause failures on future tests as mockito + // cannot start a session when a previous one is not finished + try { + if (VERBOSE) { + Log.v(TAG, "finishSession() for " + getTestName() + " on thread " + + Thread.currentThread() + "; sHighlanderSession=" + sHighlanderSession); + + } + } finally { + sHighlanderSession = null; + finishSessionMocking(); + afterSessionFinished(); + } + } + + /** + * Called after the mockito session was finished + * + *

This method should be used by subclasses that MUST do their cleanup after the session is + * finished (as methods marked with {@link After} in the subclasses would be called BEFORE + * that). + */ + protected void afterSessionFinished() { + if (VERBOSE) { + Log.v(TAG, "afterSessionFinished()"); + } + } + + private void finishSessionMocking() { if (mSession == null) { - Log.w(TAG, "finishSession(): no session"); + Log.w(TAG, getClass().getSimpleName() + ".finishSession(): no session"); return; } try { - if (DEBUG) { - Log.d(TAG, "finishSession()"); + mSession.finishMocking(); + } finally { + // Shouldn't need to set mSession to null as JUnit always instantiate a new object, + // but it doesn't hurt.... + mSession = null; + // When using inline mock maker, clean up inline mocks to prevent OutOfMemory + // errors. See https://github.com/mockito/mockito/issues/1614 and b/259280359. + Mockito.framework().clearInlineMocks(); + } + } + + private void finishHighlanderSessionIfNeeded(String where) { + if (sHighlanderSession == null) { + if (VERBOSE) { + Log.v(TAG, "finishHighlanderSessionIfNeeded(): sHighlanderSession already null"); + } + return; + } + + if (sSessionCreationLocation != null) { + if (VERBOSE) { + Log.e(TAG, where + ": There can be only one! Closing unfinished session, " + + "created at", sSessionCreationLocation); + } else { + Log.e(TAG, where + ": There can be only one! Closing unfinished session, " + + "created at " + sSessionCreationLocation); + } + } else { + Log.e(TAG, where + ": There can be only one! Closing unfinished session created at " + + "unknown location"); + } + try { + sHighlanderSession.finishMocking(); + } catch (Throwable t) { + if (VERBOSE) { + Log.e(TAG, "Failed to close unfinished session on " + getTestName(), t); + } else { + Log.e(TAG, "Failed to close unfinished session on " + getTestName() + ": " + t); } } finally { - // mSession.finishMocking() must ALWAYS be called (hence the over-protective try/finally - // statements), otherwise it would cause failures on future tests as mockito - // cannot start a session when a previous one is not finished - mSession.finishMocking(); + if (VERBOSE) { + Log.v(TAG, "Resetting sHighlanderSession at finishHighlanderSessionIfNeeded()"); + } + sHighlanderSession = null; } } + /** + * Forces a failure at the given invocation of a test method by throwing an exception. + */ + protected final void forceFailure(int invocationCount, + Class failureClass, String reason) throws T { + if (sInvocationsCounter != invocationCount) { + Log.d(TAG, "forceFailure(" + invocationCount + "): no-op on invocation #" + + sInvocationsCounter); + return; + } + String message = "Throwing on invocation #" + sInvocationsCounter + ": " + reason; + Log.e(TAG, message); + T throwable; + try { + Constructor constructor = failureClass.getConstructor(String.class); + throwable = constructor.newInstance("Throwing on invocation #" + sInvocationsCounter + + ": " + reason); + } catch (Exception e) { + throw new IllegalArgumentException("Could not create exception of class " + failureClass + + " using msg='" + message + "' as constructor"); + } + throw throwable; + } + + protected final @Nullable String getTestName() { + return mDumpableDumperRule.getTestName(); + } + protected final StandardSubjectBuilder expectWithMessage(String msg) { return mExpect.withMessage(msg); } diff --git a/services/tests/mockingservicestests/src/com/android/server/alarm/AlarmManagerServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/alarm/AlarmManagerServiceTest.java index 3b20735770b59..a1f4fddd23349 100644 --- a/services/tests/mockingservicestests/src/com/android/server/alarm/AlarmManagerServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/alarm/AlarmManagerServiceTest.java @@ -48,7 +48,6 @@ import static com.android.dx.mockito.inline.extended.ExtendedMockito.doCallRealM import static com.android.dx.mockito.inline.extended.ExtendedMockito.doReturn; import static com.android.dx.mockito.inline.extended.ExtendedMockito.doThrow; import static com.android.dx.mockito.inline.extended.ExtendedMockito.mock; -import static com.android.dx.mockito.inline.extended.ExtendedMockito.mockitoSession; import static com.android.dx.mockito.inline.extended.ExtendedMockito.spyOn; import static com.android.dx.mockito.inline.extended.ExtendedMockito.verify; import static com.android.dx.mockito.inline.extended.ExtendedMockito.when; @@ -159,6 +158,7 @@ import android.util.SparseArray; import androidx.test.runner.AndroidJUnit4; import com.android.dx.mockito.inline.extended.MockedVoidMethod; +import com.android.dx.mockito.inline.extended.StaticMockitoSessionBuilder; import com.android.internal.annotations.GuardedBy; import com.android.internal.app.IAppOpsCallback; import com.android.internal.app.IAppOpsService; @@ -167,6 +167,7 @@ import com.android.server.AlarmManagerInternal; import com.android.server.AppStateTracker; import com.android.server.AppStateTrackerImpl; import com.android.server.DeviceIdleInternal; +import com.android.server.ExtendedMockitoTestCase; import com.android.server.LocalServices; import com.android.server.SystemClockTime.TimeConfidence; import com.android.server.SystemService; @@ -179,7 +180,6 @@ import com.android.server.usage.AppStandbyInternal; import libcore.util.EmptyArray; -import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -187,7 +187,6 @@ import org.mockito.Answers; import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatchers; import org.mockito.Mock; -import org.mockito.MockitoSession; import org.mockito.quality.Strictness; import org.mockito.stubbing.Answer; @@ -203,7 +202,7 @@ import java.util.function.LongConsumer; @Presubmit @RunWith(AndroidJUnit4.class) -public class AlarmManagerServiceTest { +public final class AlarmManagerServiceTest extends ExtendedMockitoTestCase { private static final String TAG = AlarmManagerServiceTest.class.getSimpleName(); private static final int SYSTEM_UI_UID = 12345; private static final int TEST_CALLING_USER = UserHandle.getUserId(TEST_CALLING_UID); @@ -255,7 +254,6 @@ public class AlarmManagerServiceTest { DeviceConfig.Properties mDeviceConfigProperties; HashSet mDeviceConfigKeys = new HashSet<>(); - private MockitoSession mMockingSession; private Injector mInjector; private volatile long mNowElapsedTest; private volatile long mNowRtcTest; @@ -408,26 +406,31 @@ public class AlarmManagerServiceTest { } } + @Override + protected Strictness getSessionStrictness() { + return Strictness.WARN; + } + + @Override + protected void initializeSession(StaticMockitoSessionBuilder builder) { + builder + .spyStatic(ActivityManager.class) + .mockStatic(CompatChanges.class) + .spyStatic(DateFormat.class) + .spyStatic(DeviceConfig.class) + .mockStatic(LocalServices.class) + .spyStatic(Looper.class) + .mockStatic(MetricsHelper.class) + .mockStatic(PermissionChecker.class) + .mockStatic(PermissionManagerService.class) + .mockStatic(ServiceManager.class) + .mockStatic(Settings.Global.class) + .mockStatic(SystemProperties.class) + .spyStatic(UserHandle.class); + } + @Before public final void setUp() { - mMockingSession = mockitoSession() - .initMocks(this) - .spyStatic(ActivityManager.class) - .mockStatic(CompatChanges.class) - .spyStatic(DateFormat.class) - .spyStatic(DeviceConfig.class) - .mockStatic(LocalServices.class) - .spyStatic(Looper.class) - .mockStatic(MetricsHelper.class) - .mockStatic(PermissionChecker.class) - .mockStatic(PermissionManagerService.class) - .mockStatic(ServiceManager.class) - .mockStatic(Settings.Global.class) - .mockStatic(SystemProperties.class) - .spyStatic(UserHandle.class) - .strictness(Strictness.WARN) - .startMocking(); - doReturn(mIActivityManager).when(ActivityManager::getService); doReturn(mDeviceIdleInternal).when( () -> LocalServices.getService(DeviceIdleInternal.class)); @@ -3750,11 +3753,8 @@ public class AlarmManagerServiceTest { testTemporaryQuota_bumpedBeforeDeferral(STANDBY_BUCKET_RARE); } - @After - public void tearDown() { - if (mMockingSession != null) { - mMockingSession.finishMocking(); - } + @Override + public void afterSessionFinished() { LocalServices.removeServiceForTest(AlarmManagerInternal.class); } } diff --git a/services/tests/mockingservicestests/src/com/android/server/am/CachedAppOptimizerTest.java b/services/tests/mockingservicestests/src/com/android/server/am/CachedAppOptimizerTest.java index 64e39ef9e1f75..f0cd6cc965c36 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/CachedAppOptimizerTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/CachedAppOptimizerTest.java @@ -49,6 +49,7 @@ import java.util.concurrent.TimeUnit; import org.junit.After; import org.junit.Assume; import org.junit.Before; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -63,6 +64,8 @@ import org.mockito.junit.MockitoJUnitRunner; */ @Presubmit @RunWith(MockitoJUnitRunner.class) +@Ignore("TODO(b/226641572): this test is broken and it cannot use ExtendedMockitoTestCase as it " + + "uses TestableDeviceConfigRule, which creates its own mockito session") public final class CachedAppOptimizerTest { private ServiceThread mThread;