Repository: mockito/mockito
Issue: mockito-core MockitoAnnotations::openMocks initializes class twice if said class has super class

## Short, Self Contained, Correct (Compilable), Example:
Have a look [here ](https://github.com/1-alex98/mre-mockito/tree/master/src/test/java). 
You will find a `Works` class and a `DoesNotWork` class. Both are essentially the same with a small difference being the `DoesNotWork` class does fail and extends and empty class.

### DoesNotWork.java:
```
@ExtendWith({MockitoExtension.class})
public class DoesNotWork extends ToBeExtended{
    @Mock
    ToBeMocked toBeMocked;
    @InjectMocks
    ToBeInitialized toBeInitialized;

    @Test
    public void foo(){
        assert toBeInitialized.toBeMocked == null;
    }
}
```
### ToBeInitialized.java:
```
public class ToBeInitialized {
    public ToBeMocked toBeMocked;

    public ToBeInitialized(ToBeMocked toBeMocked) {
        this.toBeMocked = null;
    }
}
```
Reading the docs that should work flawlessly. But it does not. As can be seen in `Works.java` the reason is the ` extends ToBeExtended` if you remove it everything works.

### Why does it not work

In the  `DoesNotWork` the `@InjectMocks` annotated instances are initialized twice. It does it twice, one time more for every super class there is. First the `@InjectMocks` annotated instances are initialized via constructor injection. In a second round Mockito realizes that DoesNotWork.toBeInitialized is already initialized and therefore chooses Field injection over constructor injection. That makes sense if somebody does something like:

```
...
    @InjectMocks
    Foo foo= new Foo();
...
```

### The code responsible
InjectingAnnotationEngine::processInjectMocks https://github.com/mockito/mockito/blob/caf35b24e2764df0498469526ecb3e7ec68a0430/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java#L56

The method is called by the MockitoExtension:
```
processInjectMocks:61, InjectingAnnotationEngine (org.mockito.internal.configuration)
process:48, InjectingAnnotationEngine (org.mockito.internal.configuration)
openMocks:82, MockitoAnnotations (org.mockito)
<init>:43, DefaultMockitoSession (org.mockito.internal.framework)
startMocking:83, DefaultMockitoSessionBuilder (org.mockito.internal.session)
beforeEach:153, MockitoExtension (org.mockito.junit.jupiter)
```

```
    private List<AutoCloseable> processInjectMocks(
            final Class<?> clazz, final Object testInstance) {
        List<AutoCloseable> closeables = new ArrayList<>();
        Class<?> classContext = clazz;
        while (classContext != Object.class) {
            closeables.add(injectCloseableMocks(testInstance));
            classContext = classContext.getSuperclass();
        }
        return closeables;
    }
```
In this method for every super class the Test class has(`classContext.getSuperclass()`) it triggers the injection(` injectCloseableMocks`) yet another time(`while (classContext != Object.class)`). That might be clever if the super class has  `@InjectMocks` annotated instances its-self. But it is odd to call `closeables.add(injectCloseableMocks(testInstance));` instead of `closeables.add(injectCloseableMocks(classContext));` then. For me it would make more sense to call it on the super class then instead of repeating the initialization yet another time. But I have not tested this change. I can not foresee what implications this would have. Fixing this concrete bug is probably better of in the hands of somebody that is more familiar with Mockito's internal workings. 

## Why even care about this bug
Seems like it still initializes the instance right? Who does 
```
public ToBeInitialized(ToBeMocked toBeMocked) {
    this.toBeMocked = null;
 }
``` 
anyways?

Since newer Java versions get stricter in enforcing reflections over module borders this will increasingly become an issue. In my case Mockito does access a field via reflections it is not allowed to making it fail. But it should never have tried field injection since the class had a working constructor. Due to the bug that is descripted here it did nevertheless.  


## Versions:
JDK 17, Windows 11, Mockito Version 4.4.0 (Even though this all should be reproducible elsewhere and with different versions)

Thank you for working on Mockito. Great Framework ❤️ 
Any more questions? I am happy to help.

Issue comments:
> But I have not tested this change. I can not foresee what implications this would have

If you add your use case as a regression test to https://github.com/mockito/mockito/tree/main/subprojects/junit-jupiter/src/test/java/org/mockitousage and all tests pass with your fix, it is safe to merge 😄 Good catch! It's definitely worth fixing.

Looking forward to your PR!
The issues seem to come from this PR long ago https://github.com/mockito/mockito/pull/1810

The author does in **contradiction** to the docs have the wish to inject fields that are not initialized by the constructor via field injection. I think that this "feature" is problematic. Not only does this feature seem to be completely wrong implemented to me, it might causes issues if it was correctly implemented. To me this feature is highly undesired.

Here some examples

```
class Foo extends Thread {

public Foo(ToBeMocked toBeMocked){
  ...
}

}
```
Mockito would try to inject Mocks into instance variables of the Thread class(which will make it crash).
```
class Foo{

private Object fooBar;

public Foo(ToBeMocked toBeMocked){
  ...
}

}
```
Mockito would try to inject random Mocks into Object field.


What happened here if I should guess is the following.

1. There was once some totally misplaced loop that did initialization multiple times.
2. Somebody saw that and fixed it https://github.com/mockito/mockito/pull/1588 @LihMeh
3. Another guy(@tokuhirom) comes that relied on the bug and declared the bug a feature. Namely that even if there is a valid constructor other left over fields should be field injected(by accident). https://github.com/mockito/mockito/pull/1810 . He also never updated the docs to declare his behaviour. Probably more ppl would have complained about this if it was not only working under rare condition (the test class extends another class). Even more would need to be done for that feature to work anyways I think. 
4. Now I am here and I am planning on breaking the bug or feature that the guy from 3. built XD

If you ask me:
The feature @tokuhirom wanted is not a good idea if you ask me. I would kind of just revert his PR and things would work as described in docs. 
@TimvdLippe what do you think?
I wanted to chime in on this as it seems this issue and the combination of #2942 may have made @InjectMocks unusable in a larger than intended number of cases.

The root of the issue is that when the field injection occurs on a super class and the super class has a loosely typed field such as object. The mocking will completely fail because all mocks will match this field in the super class. So this forces people to mock all fields in the entire hierarchy of the class even if constructor injection succeeds and is preferred which is counter to the documentation as mentioned above.

Pull request review comments:
As a user, I think the behavior as described in this test makes sense. Therefore, I would like to keep it. That said, it would be ideal if we can keep this behavior as-is, while also fixing your issue. Do you think that is feasible or are they mutually exclusive?
1. This test tests a behaviour that was never there. It tests a bug. The proper behaviour for this would need to be implemented. Only a weird bug makes this test pass.
2. While this behaviour might make sense in some cases, it might be highly problematic in other cases. So I strongly advise against this feature. I could very well imagine this breaking quite some use cases of other's. The only reason nobody complained yet about this "feature"/tested behaviour that is does not exist yet(only in edge cases and even there does the wrong thing). Proper way of doing this would probably be to annotate setters used for DI explicitly. But I think that is a little to much to code into Mockito. 
3. The docs explicitly state the opposite of what this test is validating.

For more details/examples please read the comments in the issue.
Maybe there is a clever way to combine all use cases here but it would need to be implemented. There is nothing there yet as one might assume by this test
Commonly Beans contain fields that are not supposed to be injected, maybe an object field even. How would one know if a field is supposed to be injected? Well only way to let the user make annotations right?

A lot of strange cases can arise here, I would not go down that road. That's why DI Frameworks have annotations to clarify all this. And commonly constructor injection is recommended.
Injection in Mockito has always been a difficult topic. What one considers a bug, others consider a feature. In this case, the current behavior is what some of our users are relying on, as you saw with the reverts that happened. Therefore, if we were to make changes again, we are breaking those users.

This means that the Mockito maintainers now have to choose between two scenarios, without being able to satisfy all users. In this case, I lean towards keeping the current behavior to be conservative, rather than breaking these users and introduce regressions in their test suites.

If you want fully deterministic dependency injection, I advise you to use a DI framework as you mentioned. Mockito injection is a best-effort attempt.
Yes but current state is just a mess
Whatever you do leaving it like it is now is super broken
If I were you I would not support cases of mixed injection, where both constructor and setter injection is being used
I am still not sure whether this behavior makes sense or not. @tokuhirom since you contributed the original regression test, can you please review this PR?
These asserts need to be changed to `AssertJ`, as it fails the build.
@1-alex98 Did you see this comment? We need to change the code, not just a rebase.
yes I did that locally. I am not sure why that did not make it here. I will check if I am home. I probably forgot to push or something. @TimvdLippe 

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 ef9933d9d6..eeb019ade5 100644
--- a/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java
+++ b/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java
@@ -45,7 +45,7 @@ public class InjectingAnnotationEngine implements AnnotationEngine {
     public AutoCloseable process(Class<?> clazz, Object testInstance) {
         List<AutoCloseable> closeables = new ArrayList<>();
         closeables.addAll(processIndependentAnnotations(testInstance.getClass(), testInstance));
-        closeables.addAll(processInjectMocks(testInstance.getClass(), testInstance));
+        closeables.add(injectCloseableMocks(testInstance));
         return () -> {
             for (AutoCloseable closeable : closeables) {
                 closeable.close();
@@ -53,17 +53,6 @@ public AutoCloseable process(Class<?> clazz, Object testInstance) {
         };
     }
 
-    private List<AutoCloseable> processInjectMocks(
-            final Class<?> clazz, final Object testInstance) {
-        List<AutoCloseable> closeables = new ArrayList<>();
-        Class<?> classContext = clazz;
-        while (classContext != Object.class) {
-            closeables.add(injectCloseableMocks(testInstance));
-            classContext = classContext.getSuperclass();
-        }
-        return closeables;
-    }
-
     private List<AutoCloseable> processIndependentAnnotations(
             final Class<?> clazz, final Object testInstance) {
         List<AutoCloseable> closeables = new ArrayList<>();


Test patch:
diff --git a/src/test/java/org/mockito/internal/configuration/InjectingAnnotationEngineTest.java b/src/test/java/org/mockito/internal/configuration/InjectingAnnotationEngineTest.java
deleted file mode 100644
index 274db3beca..0000000000
--- a/src/test/java/org/mockito/internal/configuration/InjectingAnnotationEngineTest.java
+++ /dev/null
@@ -1,55 +0,0 @@
-/*
- * 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 {}
-}
diff --git a/subprojects/junit-jupiter/src/test/java/org/mockitousage/InjectMocksTest.java b/subprojects/junit-jupiter/src/test/java/org/mockitousage/InjectMocksTest.java
new file mode 100644
index 0000000000..0f2523d9bd
--- /dev/null
+++ b/subprojects/junit-jupiter/src/test/java/org/mockitousage/InjectMocksTest.java
@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2018 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockitousage;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class ToBeMockedInTestSuperClass{
+    int identifier;
+
+    public ToBeMockedInTestSuperClass(int identifier) {
+        this.identifier = identifier;
+    }
+}
+
+class ToBeMocked{
+    int identifier;
+
+    public ToBeMocked(int identifier) {
+        this.identifier = identifier;
+    }
+}
+
+class TestClassToBeInitiatedViaConstructorInSuperClass {
+    ToBeMockedInTestSuperClass toBeMockedInTestSuperClass;
+
+    public TestClassToBeInitiatedViaConstructorInSuperClass(ToBeMockedInTestSuperClass toBeMockedInTestSuperClass) {
+        assertThat(toBeMockedInTestSuperClass).isNotNull();
+        this.toBeMockedInTestSuperClass = new ToBeMockedInTestSuperClass(42);
+    }
+}
+
+class TestClassToBeInitiatedViaConstructor{
+    ToBeMockedInTestSuperClass toBeMockedInTestSuperClass;
+    ToBeMocked toBeMocked;
+
+    public TestClassToBeInitiatedViaConstructor(ToBeMocked toBeMocked, ToBeMockedInTestSuperClass toBeMockedInTestSuperClass) {
+        assertThat(toBeMocked).isNotNull();
+        assertThat(toBeMockedInTestSuperClass).isNotNull();
+        this.toBeMocked = new ToBeMocked(42);
+        this.toBeMockedInTestSuperClass = new ToBeMockedInTestSuperClass(42);
+    }
+}
+
+class SuperTestClass {
+    @Mock
+    ToBeMockedInTestSuperClass toBeMockedInTestSuperClass;
+
+    @InjectMocks
+    TestClassToBeInitiatedViaConstructorInSuperClass testClassToBeInitiatedViaConstructorInSuperClass;
+
+}
+
+@ExtendWith(MockitoExtension.class)
+class InjectMocksTest extends SuperTestClass {
+
+    @Mock
+    ToBeMocked toBeMocked;
+
+    @InjectMocks
+    TestClassToBeInitiatedViaConstructor testClassToBeInitiatedViaConstructor;
+
+    /**
+     * Checks that {@link #testClassToBeInitiatedViaConstructor} holds instances that have identifier 42.
+     * It being 42 is proof that constructor injection was used over field injection.
+     */
+    @Test
+    void given_instanceToBeInitializedByMockito_when_mocksRequestedByConstructorAreInTestAndSuperClass_should_useConstructorInjection() {
+        assertThat(testClassToBeInitiatedViaConstructor)
+            .extracting(
+                testInstance-> testInstance.toBeMocked.identifier,
+                testInstance-> testInstance.toBeMockedInTestSuperClass.identifier
+            )
+            .containsExactly(
+                42,
+                42
+            );
+    }
+
+    /**
+     * Checks that {@link #testClassToBeInitiatedViaConstructorInSuperClass} holds instances that have identifier 42.
+     * It being 42 is proof that constructor injection was used over field injection.
+     */
+    @Test
+    public void given_instanceInSuperClassToBeInitializedByMockito_when_mocksRequestedAreInSuperClass_should_useConstructorInjection(){
+        assertThat(testClassToBeInitiatedViaConstructorInSuperClass)
+            .extracting(yetAnotherClas1 -> yetAnotherClas1.toBeMockedInTestSuperClass.identifier)
+            .isEqualTo(42);
+    }
+
+}