Repository: mockito/mockito
Issue: Regression: @Spy with @InjectMock are not injected anymore
Labels: injection

Hi,

 - [x] The mockito message in the stacktrace have useful information, but it didn't help
    java.lang.NullPointerException
	at org.test.TestedService.servivce(TestedService.java:16)
	at org.test.TestedServiceTest.mockSpyAndInjectAll(TestedServiceTest.java:33)
 - [x] The problematic code (if that's possible) is copied here;
    Fix of #1587 (Remove unnecessary loop from InjectingAnnotationEngine) has introduce a regression.
 - [x] Provide versions (mockito / jdk / os / any other relevant information)
    Issue can be reproduce starting from Mockito version 2.23.15.
 - [x] Provide a [Short, Self Contained, Correct (Compilable), Example](http://sscce.org) of the issue
    Please find attached [inject-spy.zip](https://github.com/mockito/mockito/files/2905749/inject-spy.zip).
    execute `./gradlew check`
- [x] Read the [contributing guide](https://github.com/mockito/mockito/blob/master/.github/CONTRIBUTING.md)

We are experiencing a regression behavior caused by fix of #1587 (Remove unnecessary loop from InjectingAnnotationEngine) which prevent us to update from Mockito 2.23.4 to newer versions.

```java
public class TestedServiceTest extends AbstractTest {

    @InjectMocks
    private TestedService testedService;

    @InjectMocks
    @Spy
    private SpiedCompositionService spiedCompositionService;

    @Mock
    private MockedDao mockedDao;
    ...
}
```

As you can see in this snippet and example project attached, with Mockito 2.23.14 when we try to inject a mock in a spy and this spy in the tested object, it works as expected and everything is injected. But when we switch to Mockito 2.23.15, this behavior doesn't work anymore, the spy is not injected to tested object and obviously causes a NullPointerException.

We also figured out injection of injected spies was working only because test class extends has an abstract test class.

I understand this test case is not a state of the art way to do unit test, our use case is more a component test because several classes work together to do a coherent unit of work and it makes more sense to test them together.

We are relying on the behavior provided up to Mockito 2.23.14 and the only workaround I see is to expose attributes to package visibility and do injection by hand.

The loop removed by #1587 was injecting injected spies newly instantiated by first loop. The depth of injection was relying on the depth of test hierarchy.

Issue comments:
Is there any update on this? This has broken our tests as well attempting to upgrade our version of Mockito. The usage of an @InjectMocks as well as a @Spy & @InjectMocks makes sense from the perspective of integration testing of two components that act as one cohesive unit. 
Here's a pseudo code to describe the issues around this issue.

# Before 2.23.15:	

```
while (clazz != Object.clazz) {
	val mocks = aggregateSpyOrMockFields(clazz.getMockFields());
  val fields = aggregateInjectMocksFields(clazz);
	val ret = tryConstructorInjection(clazz, fields, mock);
  if (! ret.fieldWasInitializedUsingConstructorInjection()) {
    trySetterOrPropertyInjection(clazz, fields, mock);
  }
	clazz = clazz.getSuperclass();
}
```
# After 2.23.15:
```
	val mocks = aggregateSpyOrMockFields(clazz.getMockFields());
  val fields = aggregateInjectMocksFields(clazz);
	val ret = tryConstructorInjection(clazz, fields, mock);
  if (! ret.fieldWasInitializedUsingConstructorInjection()) {
    trySetterOrPropertyInjection(clazz, fields, mock);
  }
```

# Details

Before 2.23.15, if the `@InjectMocks` field has a field that not listed in the constructor argument will fill by setter/property injection at the 2nd loop 😃
It may not the expected behaviors by Mockito authors, but some users' code depends on this behavior.

My suggestion is to revert the [Fixes #1587 : Remove unnecessary loop from InjectingAnnotationEngine by LihMeh · Pull Request #1588 · mockito/mockito · GitHub](https://github.com/mockito/mockito/pull/1588) at this time.

Feel free to open a PR with regression test and I will be happy to merge it. Thanks!
This should be fixed with #1810

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 {
+    }
+}