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
2023-02-04 01:12:00 +00:00
parent c7f6a80cbe
commit 36ef8f0791
4 changed files with 207 additions and 45 deletions
--- 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<Dumpable> 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;
                }
            }
--- 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!
+     *
+     * <p>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 {
     * <p>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
+     *
+     * <p>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 <T extends Throwable> void forceFailure(int invocationCount,
+            Class<T> 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<T> 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);
    }
--- 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<String> 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);
    }
 }
--- 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;