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

[1-alex98]

Short, Self Contained, Correct (Compilable), Example:

Have a look here .
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

mockito/src/main/java/org/mockito/internal/configuration/InjectingAnnotationEngine.java

Line 56 in caf35b2

private List<AutoCloseable> processInjectMocks(

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.

[TimvdLippe]

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!

[1-alex98]

The issues seem to come from this PR long ago #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.

[1-alex98]

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 Fixes #1587 : Remove unnecessary loop from InjectingAnnotationEngine #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). Revert "Fixes #1587 : Remove unnecessary loop from InjectingAnnotationEngine" #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.

[1-alex98]

@TimvdLippe what do you think?

[Sheikah45]

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.