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 25c927e9f3 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
This commit is contained in:
ibaker 2024-12-03 10:05:51 -08:00 committed by Copybara-Service
parent 593899c2b2
commit 9828d104b5
2 changed files with 117 additions and 15 deletions

View File

@ -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<? extends Player.Listener> forwardingListenerClass =
(Class<? extends Player.Listener>) getInnerClass("ForwardingListener");
(Class<? extends Player.Listener>)
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<Listener> listeners = new HashSet<>();

View File

@ -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 <T extends @NonNull Object, F extends T>
void assertForwardingClassForwardsAllMethods(
Class<T> superType, Function<T, F> forwardingInstanceFactory)
throws InvocationTargetException, IllegalAccessException {
throws InvocationTargetException,
IllegalAccessException,
NoSuchMethodException,
InstantiationException {
assertForwardingClassForwardsAllMethodsExcept(
superType, forwardingInstanceFactory, /* excludedMethods= */ ImmutableSet.of());
}
@ -667,7 +674,10 @@ public class TestUtil {
public static <T extends @NonNull Object, F extends T>
void assertForwardingClassForwardsAllMethodsExcept(
Class<T> superType, Function<T, F> forwardingInstanceFactory, Set<String> 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.
*
* <p>For final types, where mocking is not possible, {@link #tryCreateInstance(Class)} is used
* instead.
*/
@SuppressWarnings("unchecked")
@Nullable
private static <T> T tryCreateMockInstance(Class<T> 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> T tryCreateInstance(Class<T> clazz) {
Constructor<T>[] constructors = (Constructor<T>[]) clazz.getConstructors();
// Start with the constructor with fewest parameters.
Arrays.sort(constructors, Ordering.natural().onResultOf(c -> c.getParameterTypes().length));
for (Constructor<T> 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()