Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes issue #1917 #3154

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@

import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.util.*;

import org.mockito.exceptions.base.MockitoException;
import org.mockito.internal.util.reflection.FieldInitializationReport;
Expand Down Expand Up @@ -43,9 +43,9 @@ public ConstructorInjection() {}
public boolean processInjection(Field field, Object fieldOwner, Set<Object> mockCandidates) {
try {
SimpleArgumentResolver simpleArgumentResolver =
new SimpleArgumentResolver(mockCandidates);
new SimpleArgumentResolver(mockCandidates);
FieldInitializationReport report =
new FieldInitializer(fieldOwner, field, simpleArgumentResolver).initialize();
new FieldInitializer(fieldOwner, field, simpleArgumentResolver).initialize();

return report.fieldWasInitializedUsingContructorArgs();
} catch (MockitoException e) {
Expand All @@ -67,6 +67,21 @@ static class SimpleArgumentResolver implements ConstructorArgumentResolver {
public SimpleArgumentResolver(Set<Object> objects) {
this.objects = objects;
}
/**
* types -> actual constructor types
* argTypes -> fake constructor types
* hashMap -> mocks
*/
@Override
public Object[] resolveTypeInstancesGeneric(HashMap<String, Type> hashMap, Type[] types) {
// list of mocks not constructors
List<Object> argumentInstances = new ArrayList<>(types.length);
for (Type parameterType: types) {
argumentInstances.add(objectThatIsAssignableFromGeneric(hashMap,parameterType));
}
return argumentInstances.toArray();

}

@Override
public Object[] resolveTypeInstances(Class<?>... argTypes) {
Expand All @@ -85,5 +100,26 @@ private Object objectThatIsAssignableFrom(Class<?> argType) {
}
return null;
}

private Object objectThatIsAssignableFromGeneric(HashMap<String, Type> hashMap, Type parameterType) {
Class<?> typeclass = null;
// converts parameter Type into Class
if (parameterType instanceof Class) {
typeclass = (Class<?>) parameterType;
} else if (parameterType instanceof ParameterizedType) {
ParameterizedType parameterizedType = (ParameterizedType) parameterType;
typeclass = (Class<?>) parameterizedType.getRawType();
}
assert typeclass != null;
// for each mock object
for (Object object : objects) {
if (typeclass.isAssignableFrom(object.getClass())) {
// if mock object class(including parameterized classes) matches class of constructor class
if (hashMap.get(object.toString()) == null) continue;
if (hashMap.get(object.toString()).equals(parameterType)) return object;
}
}
return null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,8 @@
import org.mockito.internal.util.MockUtil;
import org.mockito.plugins.MemberAccessor;

import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Modifier;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import java.lang.reflect.*;
import java.util.*;

import static java.lang.reflect.Modifier.isStatic;

Expand Down Expand Up @@ -165,6 +159,7 @@ public interface ConstructorArgumentResolver {
* @return The argument instances to be given to the constructor, should not be null.
*/
Object[] resolveTypeInstances(Class<?>... argTypes);
Object[] resolveTypeInstancesGeneric(HashMap<String, Type> hashMap, Type[] types);
}

private interface ConstructorInstantiator {
Expand Down Expand Up @@ -284,9 +279,17 @@ private int countMockableParams(Constructor<?> constructor) {

@Override
public FieldInitializationReport instantiate() {
Field[] fields = testClass.getClass().getDeclaredFields();
HashMap<String, Type> hashmap = new HashMap<>();

for (Field field : fields) {
hashmap.put(field.getName(), field.getGenericType());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this hashmap? You are comparing objects with strings in the hashmap in the implementation, but that can lead to false-positives. I am also not sure why we would do that lookup there, as we can also pass in all the fields of the testclass into the lookup and perform the injection there.

Copy link
Author

@sectorpre sectorpre Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi thanks for the feedback! Definitely do agree that the hashmap is quite redundant but there's an issue storing mocks as a set of Objects, as this causes type erasure. I'll try to remove this hashmap in the next commit as well as store mocks as a set of Types instead of Objects.

}
final MemberAccessor accessor = Plugins.getMemberAccessor();
Constructor<?> constructor = biggestConstructor(field.getType());
final Object[] args = argResolver.resolveTypeInstances(constructor.getParameterTypes());
final Object[] args = argResolver.resolveTypeInstancesGeneric(
hashmap, constructor.getGenericParameterTypes());
//final Object[] args = argResolver.resolveTypeInstances(constructor.getParameterTypes());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are removing the only usage of resolveTypeInstances afaik, so instead of introducing a new method, let's fix the existing method.

try {
Object newFieldInstance = accessor.newInstance(constructor, args);
accessor.set(field, testClass, newFieldInstance);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package org.mockito.internal.configuration.injection;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import java.util.HashMap;

import static org.junit.Assert.assertEquals;

@RunWith(MockitoJUnitRunner.class)
public class InjectionFourHashmapsWithDifferentTypeParameterTest {
public static class TwoListsTest {
private final HashMap<String, String> hashmap1;
private final HashMap<String, Integer> hashmap2;
private final HashMap<Float, Integer> hashmap3;
private final HashMap<Float, Float> hashmap4;

public TwoListsTest(
HashMap<String, String> hashmap1, HashMap<String, Integer> hashmap2, HashMap<Float, Integer> hashmap3, HashMap<Float, Float> hashmap4) {
this.hashmap1 = hashmap1;
this.hashmap2 = hashmap2;
this.hashmap3 = hashmap3;
this.hashmap4 = hashmap4;
}

public HashMap<String, String> getHashmap1() {
return hashmap1;
}

public HashMap<String, Integer> getHashmap2() {
return hashmap2;
}

public HashMap<Float, Integer> getHashmap3() {
return hashmap3;
}

public HashMap<Float, Float> getHashmap4() {
return hashmap4;
}
}

@Mock(strictness = Mock.Strictness.LENIENT)
HashMap<String, String> mockhashmap1;

@Mock(strictness = Mock.Strictness.LENIENT)
HashMap<String, Integer> mockhashmap2;

@Mock(strictness = Mock.Strictness.LENIENT)
HashMap<Float, Float> mockhashmap4;
@Mock(strictness = Mock.Strictness.LENIENT)
HashMap<Float, Integer> mockhashmap3;

@InjectMocks
TwoListsTest productionClass;
@Test
public void test() {
assertEquals("mockhashmap1", productionClass.getHashmap1().toString());
assertEquals("mockhashmap2", productionClass.getHashmap2().toString());
assertEquals("mockhashmap3", productionClass.getHashmap3().toString());
assertEquals("mockhashmap4", productionClass.getHashmap4().toString());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package org.mockito.internal.configuration.injection;

import static org.junit.Assert.*;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import java.util.List;

@RunWith(MockitoJUnitRunner.class)
public class InjectionTwoListsWithDifferentTypeParameterTest {
public static class TwoListsTest {
private final List<Integer> integerList;
private final List<String> stringList;

TwoListsTest(List<String> argumentStringList, List<Integer> argumentIntegerList) {
this.stringList = argumentStringList;
this.integerList = argumentIntegerList;
}

public List<Integer> getIntegerList() {
return integerList;
}

public List<String> getStringList() {
return stringList;
}
}
@InjectMocks
TwoListsTest productionClass;

@Mock(strictness = Mock.Strictness.LENIENT)
List<Integer> mockIntegerList;

@Mock(strictness = Mock.Strictness.LENIENT)
List<String> mockStringList;
@Test
public void test() {
System.out.println(productionClass.getIntegerList());
System.out.println(productionClass.getStringList());
assertEquals("mockIntegerList", productionClass.getIntegerList().toString());
assertEquals("mockStringList", productionClass.getStringList().toString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Type;
import java.util.HashMap;

import org.junit.Test;
import org.mockito.InjectMocks;
Expand Down Expand Up @@ -198,7 +200,7 @@ public void can_instantiate_class_with_parameterized_constructor() throws Except
ConstructorArgumentResolver resolver =
given(
mock(ConstructorArgumentResolver.class)
.resolveTypeInstances(any(Class.class)))
.resolveTypeInstancesGeneric(any(HashMap.class), any(Type[].class)))
.willReturn(new Object[] {null})
.getMock();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;

import java.io.IOException;
import java.lang.reflect.Field;
import java.lang.reflect.Type;
import java.util.HashMap;
import java.util.Map;
import java.util.Observer;
import java.util.Set;
Expand Down Expand Up @@ -73,7 +76,7 @@ public void should_be_created_with_an_argument_resolver() throws Exception {
public void should_instantiate_type_if_resolver_provide_matching_types() throws Exception {
Observer observer = mock(Observer.class);
Map map = mock(Map.class);
given(resolver.resolveTypeInstances(ArgumentMatchers.any(Class[].class)))
given(resolver.resolveTypeInstancesGeneric(any(HashMap.class), any(Type[].class)))
.willReturn(new Object[] {observer, map});

new ParameterizedConstructorInstantiator(this, field("withMultipleConstructor"), resolver)
Expand Down Expand Up @@ -104,7 +107,7 @@ this, field("withMultipleConstructor"), resolver)

@Test
public void should_report_failure_if_constructor_throws_exception() throws Exception {
given(resolver.resolveTypeInstances(ArgumentMatchers.<Class<?>[]>any()))
given(resolver.resolveTypeInstancesGeneric(any(HashMap.class), any(Type[].class)))
.willReturn(new Object[] {null});

try {
Expand All @@ -120,7 +123,7 @@ this, field("withThrowingConstructor"), resolver)
@Test
public void should_instantiate_type_with_vararg_constructor() throws Exception {
Observer[] vararg = new Observer[] {};
given(resolver.resolveTypeInstances(ArgumentMatchers.any(Class[].class)))
given(resolver.resolveTypeInstancesGeneric(any(HashMap.class), any(Type[].class)))
.willReturn(new Object[] {"", vararg});

new ParameterizedConstructorInstantiator(this, field("withVarargConstructor"), resolver)
Expand Down