Repository: mockito/mockito
Issue: InjectingAnnotationEngine does an unnecessary loop.
Labels: injection

processInjectMocks method of InjectingAnnotationEngine does unnecessary work. It might call injectMocks several times while it is sufficient to call injectMocks once.

    private void processInjectMocks(final Class<?> clazz, final Object testInstance) {
        Class<?> classContext = clazz;
        while (classContext != Object.class) {
            injectMocks(testInstance);
            classContext = classContext.getSuperclass();
        }
    }

Inside, injectMocks loops classes by himself.

    public void injectMocks(final Object testClassInstance) {
        Class<?> clazz = testClassInstance.getClass();
        Set<Field> mockDependentFields = new HashSet<Field>();
        Set<Object> mocks = newMockSafeHashSet();

        while (clazz != Object.class) {
            new InjectMocksScanner(clazz).addTo(mockDependentFields);
            new MockScanner(testClassInstance, clazz).addPreparedMocks(mocks);
            onInjection(testClassInstance, clazz, mockDependentFields, mocks);
            clazz = clazz.getSuperclass();
        }

        new DefaultInjectionEngine().injectMocksOnFields(mockDependentFields, mocks, testClassInstance);
    }

Why not delete the unnecessary loop?

Issue comments:

Pull request review comments:
Can we assert here on `target.getBar()`. Also please add another assertion with `assertEquals(target.foo, foo)`
Thanks. applied your feedback.
Why would you iterate over the super classes of the Test class ???
This behaviour does not reflect the docs of the annotation
This test only works if I is extended... seems to make little sense

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java b/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java
index 20d967d07e..6cb5fb123b 100644
--- a/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java
+++ b/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java
@@ -39,7 +39,15 @@ public class InjectingAnnotationEngine implements AnnotationEngine, org.mockito.
      */
     public void process(Class<?> clazz, Object testInstance) {
         processIndependentAnnotations(testInstance.getClass(), testInstance);
-        injectMocks(testInstance);
+        processInjectMocks(testInstance.getClass(), testInstance);
+    }
+
+    private void processInjectMocks(final Class<?> clazz, final Object testInstance) {
+        Class<?> classContext = clazz;
+        while (classContext != Object.class) {
+            injectMocks(testInstance);
+            classContext = classContext.getSuperclass();
+        }
     }
 
     private void processIndependentAnnotations(final Class<?> clazz, final Object testInstance) {


Test patch:
diff --git a/src/test/java/org/mockito/internal/configuration/InjectingAnnotationEngineTest.java b/src/test/java/org/mockito/internal/configuration/InjectingAnnotationEngineTest.java
new file mode 100644
index 0000000000..7a8b064f68
--- /dev/null
+++ b/src/test/java/org/mockito/internal/configuration/InjectingAnnotationEngineTest.java
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2019 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockito.internal.configuration;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+class I { }
+
+@RunWith(MockitoJUnitRunner.class)
+public class InjectingAnnotationEngineTest extends I {
+    @InjectMocks
+    Target target;
+    @Mock
+    Foo foo;
+    @Spy
+    Bar bar = new Bar();
+
+    /*
+     If the test case has super classes, the @InjectMocks field has a field that not listed in the constructor argument
+     will fill by setter/property injection .
+
+     https://github.com/mockito/mockito/issues/1631
+    */
+    @Test
+    public void injectMocks() {
+        Assert.assertEquals(foo, target.getFoo());
+        Assert.assertNotNull(target.getBar());
+    }
+
+    public static class Target {
+        private final Foo foo;
+        private Bar bar;
+
+        public Target(Foo foo) {
+            this.foo = foo;
+        }
+
+        public Foo getFoo() {
+            return foo;
+        }
+
+        public Bar getBar() {
+            return bar;
+        }
+    }
+
+    public static class Foo {
+    }
+
+    public static class Bar {
+    }
+}