From 9828d104b55930d642512fa0b0207bd10d711666 Mon Sep 17 00:00:00 2001 From: ibaker Date: Tue, 3 Dec 2024 10:05:51 -0800 Subject: [PATCH] Improve reflective instantiation in `ForwardingFoo` test util This code previously passed null or 'default' for every parameter. Now it tries to mock non-final types, and recursively constructor final types if possible, eventually giving up and passing null instead. The previous null-passing behaviour led to a quite confusing failure in `ForwardingPlayer.addListener` when writing https://github.com/androidx/media/commit/25c927e9f3dde097b99c710401667f575209a1ec due to an NPE when trying to compare parameter equality in `Mockito.verify` [1]. With this change, the failure is much clearer [2]. There's a relatively simple case this code still doesn't handle: A final type like `PlaybackParameters` where the constructor parameters **have** to be non-default primitives (greater than zero in that case). ------- [1] ``` java.lang.reflect.InvocationTargetException at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:569) at androidx.media3.test.utils.TestUtil.assertForwardingClassForwardsAllMethodsExcept(TestUtil.java:687) at androidx.media3.common.ForwardingPlayerTest.forwardingPlayer_forwardsAllPlayerMethods(ForwardingPlayerTest.java:110) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:569) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:588) at org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$2(SandboxTestRunner.java:290) at org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:101) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) at java.base/java.lang.Thread.run(Thread.java:840) Caused by: java.lang.NullPointerException: Cannot invoke "Object.hashCode()" because "this.listener" is null at androidx.media3.common.ForwardingPlayer$ForwardingListener.hashCode(ForwardingPlayer.java:1140) at java.base/java.lang.Object.toString(Object.java:256) at java.base/java.lang.String.valueOf(String.java:4220) at org.mockito.internal.verification.argumentmatching.ArgumentMatchingTool.toStringEquals(ArgumentMatchingTool.java:54) at org.mockito.internal.verification.argumentmatching.ArgumentMatchingTool.getSuspiciouslyNotMatchingArgsIndexes(ArgumentMatchingTool.java:36) at org.mockito.internal.verification.checkers.MissingInvocationChecker.checkMissingInvocation(MissingInvocationChecker.java:45) at org.mockito.internal.verification.Times.verify(Times.java:37) at org.mockito.internal.verification.MockAwareVerificationMode.verify(MockAwareVerificationMode.java:30) at org.mockito.internal.handler.MockHandlerImpl.handle(MockHandlerImpl.java:75) at org.mockito.internal.handler.NullResultGuardian.handle(NullResultGuardian.java:29) at org.mockito.internal.handler.InvocationNotifierHandler.handle(InvocationNotifierHandler.java:34) at org.mockito.internal.creation.bytebuddy.MockMethodInterceptor.doIntercept(MockMethodInterceptor.java:82) at org.mockito.internal.creation.bytebuddy.MockMethodInterceptor.doIntercept(MockMethodInterceptor.java:56) at org.mockito.internal.creation.bytebuddy.MockMethodInterceptor$DispatcherDefaultingToRealMethod.interceptAbstract(MockMethodInterceptor.java:161) at androidx.media3.common.Player$MockitoMock$1276619531.addListener(Unknown Source) ... 22 more ``` ---- [2] ``` Argument(s) are different! Wanted: player.addListener( Mock for Listener, hashCode: 107929032 ); -> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) Actual invocations have different arguments: player.addListener( androidx.media3.common.ForwardingPlayer$ForwardingListener@af47cec0 ); -> at androidx.media3.common.ForwardingPlayer.addListener(ForwardingPlayer.java:81) ``` PiperOrigin-RevId: 702378861 --- .../media3/common/ForwardingPlayerTest.java | 14 +-- .../androidx/media3/test/utils/TestUtil.java | 118 +++++++++++++++++- 2 files changed, 117 insertions(+), 15 deletions(-) diff --git a/libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java b/libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java index 174955e106..1e6e45b73d 100644 --- a/libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java @@ -18,8 +18,10 @@ package androidx.media3.common; import static androidx.media3.common.Player.EVENT_IS_PLAYING_CHANGED; import static androidx.media3.common.Player.EVENT_MEDIA_ITEM_TRANSITION; import static androidx.media3.common.Player.EVENT_TIMELINE_CHANGED; +import static androidx.media3.common.util.Assertions.checkNotNull; import static androidx.media3.test.utils.TestUtil.assertForwardingClassForwardsAllMethodsExcept; import static androidx.media3.test.utils.TestUtil.assertForwardingClassOverridesAllMethods; +import static androidx.media3.test.utils.TestUtil.getInnerClass; import static com.google.common.truth.Truth.assertThat; import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.mock; @@ -118,19 +120,11 @@ public class ForwardingPlayerTest { public void forwardingListener_overridesAllListenerMethods() throws Exception { // Check with reflection that ForwardingListener overrides all Listener methods. Class forwardingListenerClass = - (Class) getInnerClass("ForwardingListener"); + (Class) + checkNotNull(getInnerClass(ForwardingPlayer.class, "ForwardingListener")); assertForwardingClassOverridesAllMethods(Player.Listener.class, forwardingListenerClass); } - private static Class getInnerClass(String className) { - for (Class innerClass : ForwardingPlayer.class.getDeclaredClasses()) { - if (innerClass.getSimpleName().equals(className)) { - return innerClass; - } - } - throw new IllegalStateException(); - } - private static class FakePlayer extends StubPlayer { private final Set listeners = new HashSet<>(); diff --git a/libraries/test_utils/src/main/java/androidx/media3/test/utils/TestUtil.java b/libraries/test_utils/src/main/java/androidx/media3/test/utils/TestUtil.java index a71a4a953a..90fd63eb54 100644 --- a/libraries/test_utils/src/main/java/androidx/media3/test/utils/TestUtil.java +++ b/libraries/test_utils/src/main/java/androidx/media3/test/utils/TestUtil.java @@ -58,6 +58,7 @@ import com.google.common.base.Function; import com.google.common.collect.BoundType; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Ordering; import com.google.common.collect.Range; import com.google.common.io.ByteStreams; import com.google.common.primitives.Bytes; @@ -69,6 +70,7 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.lang.reflect.Array; +import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; @@ -77,6 +79,7 @@ import java.nio.ByteOrder; import java.nio.FloatBuffer; import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -86,6 +89,7 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutionException; import org.checkerframework.checker.nullness.qual.NonNull; +import org.mockito.Mockito; /** Utility methods for tests. */ @UnstableApi @@ -648,7 +652,10 @@ public class TestUtil { public static void assertForwardingClassForwardsAllMethods( Class superType, Function forwardingInstanceFactory) - throws InvocationTargetException, IllegalAccessException { + throws InvocationTargetException, + IllegalAccessException, + NoSuchMethodException, + InstantiationException { assertForwardingClassForwardsAllMethodsExcept( superType, forwardingInstanceFactory, /* excludedMethods= */ ImmutableSet.of()); } @@ -667,7 +674,10 @@ public class TestUtil { public static void assertForwardingClassForwardsAllMethodsExcept( Class superType, Function forwardingInstanceFactory, Set excludedMethods) - throws InvocationTargetException, IllegalAccessException { + throws InvocationTargetException, + IllegalAccessException, + NoSuchMethodException, + InstantiationException { for (Method method : getPublicMethods(superType)) { if (excludedMethods.contains(method.getName())) { continue; @@ -676,9 +686,7 @@ public class TestUtil { F forwardingInstance = forwardingInstanceFactory.apply(delegate); @NullableType Object[] parameters = new Object[method.getParameterCount()]; for (int i = 0; i < method.getParameterCount(); i++) { - // Get a default value of the right type by creating a single-element array. This gives us - // null for object types, and the 'default' value for primitives. - parameters[i] = Array.get(Array.newInstance(method.getParameterTypes()[i], 1), 0); + parameters[i] = tryCreateMockInstance(method.getParameterTypes()[i]); } method.invoke(forwardingInstance, parameters); @@ -688,6 +696,106 @@ public class TestUtil { } } + /** + * Create an instance of {@code clazz}, using {@link Mockito#mock} if possible. + * + *

For final types, where mocking is not possible, {@link #tryCreateInstance(Class)} is used + * instead. + */ + @SuppressWarnings("unchecked") + @Nullable + private static T tryCreateMockInstance(Class clazz) { + if (clazz.isPrimitive()) { + // Get a default value of the right primitive type by creating a single-element array. + return (T) Array.get(Array.newInstance(clazz, 1), 0); + } else if (clazz.isArray()) { + return (T) Array.newInstance(clazz.getComponentType(), 0); + } else if (!Modifier.isFinal(clazz.getModifiers())) { + try { + return mock(clazz); + } catch (RuntimeException e) { + // continue + } + } + // clazz is a final type, or couldn't otherwise be mocked, so we try and instantiate it via a + // public constructor instead. + return tryCreateInstance(clazz); + } + + /** + * Creates an instance of {@code clazz} by passing mock parameter values to its public + * constructor. If the type has no public constructor, we look for a nested {@code Builder} type + * and try and use that instead. If all the constructors and builders fail, return null. + */ + // The nullness checker is deliberately over-conservative and doesn't permit passing a null + // parameter to Constructor.newInstance(), even if the real constructor does accept null. + @SuppressWarnings({"unchecked", "nullness:argument.type.incompatible"}) + @Nullable + private static T tryCreateInstance(Class clazz) { + Constructor[] constructors = (Constructor[]) clazz.getConstructors(); + // Start with the constructor with fewest parameters. + Arrays.sort(constructors, Ordering.natural().onResultOf(c -> c.getParameterTypes().length)); + for (Constructor constructor : constructors) { + try { + return constructor.newInstance(createParameters(constructor.getParameterTypes())); + } catch (ReflectiveOperationException e) { + // continue + } + } + + // We didn't find a usable constructor, so look for a static factory method instead. + Method[] methods = clazz.getMethods(); + // Start with the method with fewest parameters. + Arrays.sort(methods, Ordering.natural().onResultOf(m -> m.getParameterTypes().length)); + for (Method method : methods) { + if (!Modifier.isStatic(method.getModifiers()) + || !clazz.isAssignableFrom(method.getReturnType())) { + // Skip non-static methods or those that don't return an instance of clazz + continue; + } + try { + return (T) method.invoke(null, createParameters(method.getParameterTypes())); + } catch (ReflectiveOperationException e) { + // continue + } + } + + // Try and instantiate via a builder instead + @Nullable Class builderClazz = getInnerClass(clazz, "Builder"); + if (builderClazz != null) { + Object builder = tryCreateInstance(builderClazz); + if (builder != null) { + try { + return (T) builderClazz.getMethod("build").invoke(builder); + } catch (ReflectiveOperationException e) { + // continue + } + } + } + return null; + } + + private static @NullableType Object[] createParameters(Class[] parameterTypes) { + @NullableType Object[] parameters = new Object[parameterTypes.length]; + for (int i = 0; i < parameters.length; i++) { + parameters[i] = tryCreateMockInstance(parameterTypes[i]); + } + return parameters; + } + + /** + * Returns an inner class of {@code clazz} called {@code className} if it exists, otherwise null. + */ + @Nullable + public static Class getInnerClass(Class clazz, String className) { + for (Class innerClass : clazz.getDeclaredClasses()) { + if (innerClass.getSimpleName().equals(className)) { + return innerClass; + } + } + return null; + } + /** Returns a {@link MediaItem} that has all fields set to non-default values. */ public static MediaItem buildFullyCustomizedMediaItem() { return new MediaItem.Builder()