Fixes mockito#1516 and mockito#1631

Allows @SPY with @Injectmocks to be injected into other @Injectmocks
arnor2000 · May 9, 2019 · 818929f · 818929f
1 parent ac01235
commit 818929f
Show file tree

Hide file tree

Showing 29 changed files with 1,883 additions and 540 deletions.
diff --git a/src/main/java/org/mockito/internal/configuration/DefaultInjectionEngine.java b/src/main/java/org/mockito/internal/configuration/DefaultInjectionEngine.java
@@ -6,7 +6,6 @@
 
 import java.lang.reflect.Field;
 import java.util.Set;
-
 import org.mockito.internal.configuration.injection.MockInjection;
 
 /**
@@ -16,6 +15,37 @@
  */
 public class DefaultInjectionEngine {
 
+    /**
+     * Proceeds to ongoing mocks injection on fields with:
+     * <ul>
+     * <li>strict constructor injection strategy to not allow semi-initialized fields at this step
+     * <li>lenient field/property injection strategy to skip fields without no-args constructor
+     * </ul>
+     *
+     * @param needingInjection  fields needing injection
+     * @param mocks             mocks available for injection
+     * @param testClassInstance instance of the test
+     */
+    public void injectOngoingMocksOnFields(Set<Field> needingInjection, Set<Object> mocks, Object testClassInstance) {
+        MockInjection.onFields(needingInjection, testClassInstance)
+                .withMocks(mocks)
+                .tryStrictConstructorInjection()
+                .tryLenientPropertyOrFieldInjection()
+                .handleSpyAnnotation()
+                .apply();
+    }
+
+    /**
+     * Proceeds to terminal mocks injection on fields with:
+     * <ul>
+     * <li>lenient constructor injection strategy to initialize fields even with null arguments
+     * <li>strict field/property injection strategy to fail on fields without no-args constructor
+     * </ul>
+     *
+     * @param needingInjection fields needing injection
+     * @param mocks mocks available for injection
+     * @param testClassInstance instance of the test
+     */
     public void injectMocksOnFields(Set<Field> needingInjection, Set<Object> mocks, Object testClassInstance) {
         MockInjection.onFields(needingInjection, testClassInstance)
                 .withMocks(mocks)

diff --git a/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java b/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java
@@ -76,6 +76,12 @@ public void injectMocks(final Object testClassInstance) {
             clazz = clazz.getSuperclass();
         }
 
+        Set<Object> previousMocks;
+        do {
+            previousMocks = mocks;
+            new DefaultInjectionEngine().injectOngoingMocksOnFields(mockDependentFields, mocks, testClassInstance);
+            mocks = new MockScanner(testClassInstance, testClassInstance.getClass()).scanHierarchy();
+        } while (!previousMocks.equals(mocks));
         new DefaultInjectionEngine().injectMocksOnFields(mockDependentFields, mocks, testClassInstance);
     }
 

diff --git a/src/main/java/org/mockito/internal/configuration/SpyAnnotationEngine.java b/src/main/java/org/mockito/internal/configuration/SpyAnnotationEngine.java
@@ -36,8 +36,9 @@
  * </p>
  * <p/>
  * <p>
- * If the field is also annotated with the <strong>compatible</strong> &#64;InjectMocks then the field will be ignored,
- * The injection engine will handle this specific case.
+ * If the field is also annotated with the <strong>compatible</strong> &#64;InjectMocks and has
+ * parameterized constructor then the field will be ignored, the injection engine will handle this
+ * specific case.
  * </p>
  * <p/>
  * <p>This engine will fail, if the field is also annotated with incompatible Mockito annotations.
@@ -49,7 +50,7 @@ public class SpyAnnotationEngine implements AnnotationEngine, org.mockito.config
     public void process(Class<?> context, Object testInstance) {
         Field[] fields = context.getDeclaredFields();
         for (Field field : fields) {
-            if (field.isAnnotationPresent(Spy.class) && !field.isAnnotationPresent(InjectMocks.class)) {
+            if (shouldProcess(field)) {
                 assertNoIncompatibleAnnotations(Spy.class, field, Mock.class, Captor.class);
                 field.setAccessible(true);
                 Object instance;
@@ -71,6 +72,19 @@ public void process(Class<?> context, Object testInstance) {
         }
     }
 
+    private boolean shouldProcess(Field field) {
+        if (!field.isAnnotationPresent(Spy.class)) {
+            return false;
+        }
+        if (!field.isAnnotationPresent(InjectMocks.class)) {
+            return true;
+        }
+        if (field.getType().isInterface()) {
+            return false;
+        }
+        return !hasParameterizedConstructor(field.getType());
+    }
+
     private static Object spyInstance(Field field, Object instance) {
         return Mockito.mock(instance.getClass(),
                             withSettings().spiedInstance(instance)
@@ -115,6 +129,15 @@ private static Object spyNewInstance(Object testInstance, Field field)
         }
     }
 
+    private static boolean hasParameterizedConstructor(Class<?> type) {
+        for (Constructor<?> constructor : type.getDeclaredConstructors()) {
+            if (constructor.getParameterTypes().length > 0) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     private static Constructor<?> noArgConstructorOf(Class<?> type) {
         Constructor<?> constructor;
         try {

diff --git a/src/main/java/org/mockito/internal/configuration/injection/ConstructorInjection.java b/src/main/java/org/mockito/internal/configuration/injection/ConstructorInjection.java
@@ -5,49 +5,35 @@
 
 package org.mockito.internal.configuration.injection;
 
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.util.Set;
 import org.mockito.exceptions.base.MockitoException;
+import org.mockito.internal.util.reflection.ConstructorResolver;
+import org.mockito.internal.util.reflection.ConstructorResolver.BiggestConstructorResolver;
 import org.mockito.internal.util.reflection.FieldInitializationReport;
 import org.mockito.internal.util.reflection.FieldInitializer;
-import org.mockito.internal.util.reflection.FieldInitializer.ConstructorArgumentResolver;
 
 import static org.mockito.internal.exceptions.Reporter.fieldInitialisationThrewException;
 
-import java.lang.reflect.Field;
-import java.lang.reflect.InvocationTargetException;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Set;
-
 /**
  * Injection strategy based on constructor.
  *
  * <p>
  * The strategy will search for the constructor with most parameters
- * and try to resolve mocks by type.
- * </p>
- *
- * <blockquote>
- * TODO on missing mock type, shall it abandon or create "noname" mocks.
- * TODO and what if the arg type is not mockable.
- * </blockquote>
- *
- * <p>
- * For now the algorithm tries to create anonymous mocks if an argument type is missing.
- * If not possible the algorithm abandon resolution.
+ * and try to resolve mocks by type, or null if there is no mocks matching a parameter.
  * </p>
  */
 public class ConstructorInjection extends MockInjectionStrategy {
 
-    public ConstructorInjection() { }
-
     public boolean processInjection(Field field, Object fieldOwner, Set<Object> mockCandidates) {
         try {
-            SimpleArgumentResolver simpleArgumentResolver = new SimpleArgumentResolver(mockCandidates);
-            FieldInitializationReport report = new FieldInitializer(fieldOwner, field, simpleArgumentResolver).initialize();
+            ConstructorResolver constructorResolver = createConstructorResolver(field.getType(), mockCandidates);
+            FieldInitializationReport report = new FieldInitializer(fieldOwner, field, constructorResolver).initialize();
 
-            return report.fieldWasInitializedUsingContructorArgs();
+            return report.fieldWasInitialized();
         } catch (MockitoException e) {
-            if(e.getCause() instanceof InvocationTargetException) {
+            if (e.getCause() instanceof InvocationTargetException) {
                 Throwable realCause = e.getCause().getCause();
                 throw fieldInitialisationThrewException(field, realCause);
             }
@@ -57,30 +43,8 @@ public boolean processInjection(Field field, Object fieldOwner, Set<Object> mock
 
     }
 
-    /**
-     * Returns mocks that match the argument type, if not possible assigns null.
-     */
-    static class SimpleArgumentResolver implements ConstructorArgumentResolver {
-        final Set<Object> objects;
-
-        public SimpleArgumentResolver(Set<Object> objects) {
-            this.objects = objects;
-        }
-
-        public Object[] resolveTypeInstances(Class<?>... argTypes) {
-            List<Object> argumentInstances = new ArrayList<Object>(argTypes.length);
-            for (Class<?> argType : argTypes) {
-                argumentInstances.add(objectThatIsAssignableFrom(argType));
-            }
-            return argumentInstances.toArray();
-        }
-
-        private Object objectThatIsAssignableFrom(Class<?> argType) {
-            for (Object object : objects) {
-                if(argType.isAssignableFrom(object.getClass())) return object;
-            }
-            return null;
-        }
+    protected ConstructorResolver createConstructorResolver(Class<?> fieldType, Set<Object> mockCandidates) {
+        return new BiggestConstructorResolver(fieldType, mockCandidates);
     }
 
 }
diff --git a/.../java/org/mockito/internal/configuration/injection/LenientPropertyAndSetterInjection.java b/.../java/org/mockito/internal/configuration/injection/LenientPropertyAndSetterInjection.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2019 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockito.internal.configuration.injection;
+
+import org.mockito.internal.util.reflection.ConstructorResolver;
+import org.mockito.internal.util.reflection.ConstructorResolver.LenientNoArgsConstructorResolver;
+
+/**
+ * Inject mocks using setters then fields, if no setters available, see
+ * {@link PropertyAndSetterInjection parent class} for more information on algorithm.
+ * <p>
+ * The strategy to instantiate field (if needed) is to try to find no-args constructor on field type
+ * and skip the field otherwise.
+ * </p>
+ *
+ * @see org.mockito.internal.configuration.injection.PropertyAndSetterInjection
+ */
+public class LenientPropertyAndSetterInjection extends PropertyAndSetterInjection {
+
+    @Override
+    protected ConstructorResolver createConstructorResolver(Class<?> fieldType) {
+        return new LenientNoArgsConstructorResolver(fieldType);
+    }
+
+}
diff --git a/src/main/java/org/mockito/internal/configuration/injection/MockInjection.java b/src/main/java/org/mockito/internal/configuration/injection/MockInjection.java
@@ -77,11 +77,21 @@ public OngoingMockInjection tryConstructorInjection() {
             return this;
         }
 
+        public OngoingMockInjection tryStrictConstructorInjection() {
+            injectionStrategies.thenTry(new StrictConstructorInjection());
+            return this;
+        }
+
         public OngoingMockInjection tryPropertyOrFieldInjection() {
             injectionStrategies.thenTry(new PropertyAndSetterInjection());
             return this;
         }
 
+        public OngoingMockInjection tryLenientPropertyOrFieldInjection() {
+            injectionStrategies.thenTry(new LenientPropertyAndSetterInjection());
+            return this;
+        }
+
         public OngoingMockInjection handleSpyAnnotation() {
             postInjectionStrategies.thenTry(new SpyOnInjectedFieldsHandler());
             return this;

diff --git a/src/main/java/org/mockito/internal/configuration/injection/PropertyAndSetterInjection.java b/src/main/java/org/mockito/internal/configuration/injection/PropertyAndSetterInjection.java
@@ -5,28 +5,29 @@
 
 package org.mockito.internal.configuration.injection;
 
-import static org.mockito.internal.exceptions.Reporter.cannotInitializeForInjectMocksAnnotation;
-import static org.mockito.internal.exceptions.Reporter.fieldInitialisationThrewException;
-import static org.mockito.internal.util.collections.Sets.newMockSafeHashSet;
-import static org.mockito.internal.util.reflection.SuperTypesLastSorter.sortSuperTypesLast;
-
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Modifier;
 import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
-
 import org.mockito.exceptions.base.MockitoException;
 import org.mockito.internal.configuration.injection.filter.MockCandidateFilter;
 import org.mockito.internal.configuration.injection.filter.NameBasedCandidateFilter;
 import org.mockito.internal.configuration.injection.filter.TerminalMockCandidateFilter;
 import org.mockito.internal.configuration.injection.filter.TypeBasedCandidateFilter;
 import org.mockito.internal.util.collections.ListUtil;
+import org.mockito.internal.util.reflection.ConstructorResolver;
+import org.mockito.internal.util.reflection.ConstructorResolver.NoArgsConstructorResolver;
 import org.mockito.internal.util.reflection.FieldInitializationReport;
 import org.mockito.internal.util.reflection.FieldInitializer;
 
+import static org.mockito.internal.exceptions.Reporter.cannotInitializeForInjectMocksAnnotation;
+import static org.mockito.internal.exceptions.Reporter.fieldInitialisationThrewException;
+import static org.mockito.internal.util.collections.Sets.newMockSafeHashSet;
+import static org.mockito.internal.util.reflection.SuperTypesLastSorter.sortSuperTypesLast;
+
 /**
  * Inject mocks using first setters then fields, if no setters available.
  *
@@ -58,7 +59,7 @@
  *
  * <p>
  * <u>Note:</u> If the field needing injection is not initialized, the strategy tries
- * to create one using a no-arg constructor of the field type.
+ * to create one using a no-arg constructor of the field type or fails with an explicit message.
  * </p>
  */
 public class PropertyAndSetterInjection extends MockInjectionStrategy {
@@ -78,6 +79,10 @@ public boolean isOut(Field object) {
     public boolean processInjection(Field injectMocksField, Object injectMocksFieldOwner, Set<Object> mockCandidates) {
         FieldInitializationReport report = initializeInjectMocksField(injectMocksField, injectMocksFieldOwner);
 
+        if (!report.fieldIsInitialized()) {
+            return false;
+        }
+
         // for each field in the class hierarchy
         boolean injectionOccurred = false;
         Class<?> fieldClass = report.fieldClass();
@@ -91,7 +96,8 @@ public boolean processInjection(Field injectMocksField, Object injectMocksFieldO
 
     private FieldInitializationReport initializeInjectMocksField(Field field, Object fieldOwner) {
         try {
-            return new FieldInitializer(fieldOwner, field).initialize();
+            final ConstructorResolver constructorResolver = createConstructorResolver(field.getType());
+            return new FieldInitializer(fieldOwner, field, constructorResolver).initialize();
         } catch (MockitoException e) {
             if(e.getCause() instanceof InvocationTargetException) {
                 Throwable realCause = e.getCause().getCause();
@@ -101,6 +107,9 @@ private FieldInitializationReport initializeInjectMocksField(Field field, Object
         }
     }
 
+    protected ConstructorResolver createConstructorResolver(Class<?> fieldType) {
+        return new NoArgsConstructorResolver(fieldType);
+    }
 
     private boolean injectMockCandidates(Class<?> awaitingInjectionClazz, Object injectee, Set<Object> mocks) {
         boolean injectionOccurred;

diff --git a/src/main/java/org/mockito/internal/configuration/injection/StrictConstructorInjection.java b/src/main/java/org/mockito/internal/configuration/injection/StrictConstructorInjection.java
@@ -0,0 +1,26 @@
+/*
+ * Copyright (c) 2019 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+
+package org.mockito.internal.configuration.injection;
+
+import java.util.Set;
+import org.mockito.internal.util.reflection.ConstructorResolver;
+import org.mockito.internal.util.reflection.ConstructorResolver.StrictBiggestConstructorResolver;
+
+/**
+ * Injection strategy based on constructor.
+ * <p>
+ * The strategy will search for the constructor with most parameters and try to resolve mocks by
+ * type or skip the field if there is no mocks matching a parameter.
+ * </p>
+ */
+public class StrictConstructorInjection extends ConstructorInjection {
+
+    @Override
+    protected ConstructorResolver createConstructorResolver(Class<?> fieldType, Set<Object> mockCandidates) {
+        return new StrictBiggestConstructorResolver(fieldType, mockCandidates);
+    }
+
+}
diff --git a/...java/org/mockito/internal/configuration/injection/filter/TerminalMockCandidateFilter.java b/...java/org/mockito/internal/configuration/injection/filter/TerminalMockCandidateFilter.java
@@ -4,11 +4,10 @@
  */
 package org.mockito.internal.configuration.injection.filter;
 
-import org.mockito.internal.util.reflection.BeanPropertySetter;
-
 import java.lang.reflect.Field;
 import java.util.Collection;
 import java.util.List;
+import org.mockito.internal.util.reflection.BeanPropertySetter;
 
 import static org.mockito.internal.exceptions.Reporter.cannotInjectDependency;
 import static org.mockito.internal.util.reflection.FieldSetter.setField;
@@ -36,7 +35,8 @@ public Object thenInject() {
                             setField(injectee, candidateFieldToBeInjected,matchingMock);
                         }
                     } catch (RuntimeException e) {
-                        throw cannotInjectDependency(candidateFieldToBeInjected, matchingMock, e);
+                        final Throwable details = e.getCause() == null ? e : e.getCause();
+                        throw cannotInjectDependency(candidateFieldToBeInjected, matchingMock, details);
                     }
                     return matchingMock;
                 }