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.

Sign up for GitHub

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

Jump to bottom

Fixes for #637, making theory parameters assignment more consistent across sources #651

Merged
merged 8 commits into from
Mar 19, 2013
Merged

Fixes for #637, making theory parameters assignment more consistent across sources #651

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ade5fd
Changed AllMembersSupplier to call single-valued methods at parameter…
pimterry Mar 10, 2013
cc8497f
Parameters are now matched on value, not defined type, for array and …
pimterry Mar 11, 2013
e9d4de8
Single-datapoint methods are now only called if the return types coul…
pimterry Mar 13, 2013
278d76f
Further theory optimisation to avoid calling datapoint methods for mu…
pimterry Mar 13, 2013
b1832b2
Moved instance initialisation for type conversion map into static method
pimterry Mar 14, 2013
3209ce6
Minor tweak to improve symmetric set up of convertable types in Param…
pimterry Mar 14, 2013
ccf0c0a
Changed single-valued datapoints back to deferred execution
pimterry Mar 16, 2013
c60d087
Tiny whitespace fix
pimterry Mar 18, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 41 additions & 6 deletions src/main/java/org/junit/experimental/theories/ParameterSignature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,32 @@
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;

public class ParameterSignature {

@SuppressWarnings("serial")
private static final Map<Class<?>, Class<?>> convertableTypesMap = new HashMap<Class<?>, Class<?>>() {{
Copy link
Member

Choose a reason for hiding this comment

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

I would really prefer that we don't use instance initializers in JUnit. In a constructor like this:

public MyType(...) {
  super(a, b);
  doSomething(c);
}

It is very surprising that there is code magically executed between these two lines.

Could we instead do:

private static final Map<Class<?>, Class<?>> CONVERTABLE_TYPE_MAP = convertableTypeMap();

As a bonus, 1) convertableTypeMap() can return an unmodifiable map and 2) you can remove the @SuppressWarnings("serial")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's nicer, done.

put(boolean.class, Boolean.class);
put(byte.class, Byte.class);
put(short.class, Short.class);
put(char.class, Character.class);
put(int.class, Integer.class);
put(long.class, Long.class);
put(float.class, Float.class);
put(double.class, Double.class);

// Make all type conversions symmetric
Iterable<Entry<Class<?>, Class<?>>> initialEntries = new HashSet<Entry<Class<?>, Class<?>>>(entrySet());
for (Entry<Class<?>, Class<?>> entry : initialEntries) {
this.put(entry.getValue(), entry.getKey());
}
}};

public static ArrayList<ParameterSignature> signatures(Method method) {
return signatures(method.getParameterTypes(), method
.getParameterAnnotations());
Expand Down Expand Up @@ -42,21 +65,33 @@ public boolean canAcceptValue(Object candidate) {
}

public boolean canAcceptType(Class<?> candidate) {
return type.isAssignableFrom(candidate);
return type.isAssignableFrom(candidate) ||
isAssignableViaTypeConversion(type, candidate);
}

public boolean canPotentiallyAcceptType(Class<?> candidate) {
return candidate.isAssignableFrom(type) ||
isAssignableViaTypeConversion(candidate, type) ||
Copy link
Member

Choose a reason for hiding this comment

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

Since CONVERTABLE_TYPES_MAP is symmetric, and canAcceptType already checks one direction, we don't have to also check the other direction here, right?

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I should have re-loaded the code into my wetware memory before commenting. No change needed here.

canAcceptType(candidate);
}

public Class<?> getType() {
private boolean isAssignableViaTypeConversion(Class<?> targetType, Class<?> candidate) {
if (convertableTypesMap.containsKey(candidate)) {
Class<?> wrapperClass = convertableTypesMap.get(candidate);
return targetType.isAssignableFrom(wrapperClass);
} else {
return false;
}
}

public Class<?> getType() {
Copy link
Member

Choose a reason for hiding this comment

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

Indentation went weird here?

return type;
}

public List<Annotation> getAnnotations() {
return Arrays.asList(annotations);
}

public boolean canAcceptArrayType(Class<?> type) {
return type.isArray() && canAcceptType(type.getComponentType());
}

public boolean hasAnnotation(Class<? extends Annotation> type) {
return getAnnotation(type) != null;
}
Expand Down
97 changes: 34 additions & 63 deletions src/main/java/org/junit/experimental/theories/internal/AllMembersSupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,34 +19,6 @@
* Supplies Theory parameters based on all public members of the target class.
*/
public class AllMembersSupplier extends ParameterSupplier {
static class MethodParameterValue extends PotentialAssignment {
private final FrameworkMethod fMethod;

private MethodParameterValue(FrameworkMethod dataPointMethod) {
fMethod = dataPointMethod;
}

@Override
public Object getValue() throws CouldNotGenerateValueException {
try {
return fMethod.invokeExplosively(null);
} catch (IllegalArgumentException e) {
throw new RuntimeException(
"unexpected: argument length is checked");
} catch (IllegalAccessException e) {
throw new RuntimeException(
"unexpected: getMethods returned an inaccessible method");
} catch (Throwable e) {
throw new CouldNotGenerateValueException();
// do nothing, just look for more values
}
}

@Override
public String getDescription() throws CouldNotGenerateValueException {
return fMethod.getName();
}
}

private final TestClass fClass;

Expand All @@ -71,60 +43,59 @@ public List<PotentialAssignment> getValueSources(ParameterSignature sig) {

private void addMultiPointMethods(ParameterSignature sig, List<PotentialAssignment> list) {
for (FrameworkMethod dataPointsMethod : getDataPointsMethods(sig)) {
try {
addMultiPointArrayValues(sig, dataPointsMethod.getName(), list, dataPointsMethod.invokeExplosively(null));
} catch (Throwable e) {
// ignore and move on
Class<?> returnType = dataPointsMethod.getReturnType();

if (returnType.isArray() && sig.canPotentiallyAcceptType(returnType.getComponentType())) {
try {
addArrayValues(sig, dataPointsMethod.getName(), list, dataPointsMethod.invokeExplosively(null));
} catch (Throwable e) {
// ignore and move on
}
}
}
}

private void addSinglePointMethods(ParameterSignature sig,
List<PotentialAssignment> list) {
private void addSinglePointMethods(ParameterSignature sig, List<PotentialAssignment> list) {
for (FrameworkMethod dataPointMethod : getSingleDataPointMethods(sig)) {
if (sig.canAcceptType(dataPointMethod.getType())) {
list.add(new MethodParameterValue(dataPointMethod));
}
}
}

private void addMultiPointFields(ParameterSignature sig,
List<PotentialAssignment> list) {
for (final Field field : getDataPointsFields(sig)) {
Class<?> type = field.getType();
if (sig.canAcceptArrayType(type)) {
if (sig.canPotentiallyAcceptType(dataPointMethod.getReturnType())) {
Object value;

try {
addArrayValues(field.getName(), list, getStaticFieldValue(field));
value = dataPointMethod.invokeExplosively(null);
} catch (Throwable e) {
// ignore and move on
continue;
}

if (sig.canAcceptValue(value)) {
list.add(PotentialAssignment.forValue(dataPointMethod.getName(), value));
}
}
}
}

private void addSinglePointFields(ParameterSignature sig,
List<PotentialAssignment> list) {
for (final Field field : getSingleDataPointFields(sig)) {
Class<?> type = field.getType();
if (sig.canAcceptType(type)) {
list.add(PotentialAssignment.forValue(field.getName(), getStaticFieldValue(field)));
}
}

private void addMultiPointFields(ParameterSignature sig, List<PotentialAssignment> list) {
for (final Field field : getDataPointsFields(sig)) {
addArrayValues(sig, field.getName(), list, getStaticFieldValue(field));
}
}

private void addArrayValues(String name, List<PotentialAssignment> list, Object array) {
for (int i = 0; i < Array.getLength(array); i++) {
list.add(PotentialAssignment.forValue(name + "[" + i + "]", Array.get(array, i)));
private void addSinglePointFields(ParameterSignature sig, List<PotentialAssignment> list) {
for (final Field field : getSingleDataPointFields(sig)) {
Object value = getStaticFieldValue(field);

if (sig.canAcceptValue(value)) {
list.add(PotentialAssignment.forValue(field.getName(), value));
}
}
}

private void addMultiPointArrayValues(ParameterSignature sig, String name, List<PotentialAssignment> list,
Object array) throws Throwable {
private void addArrayValues(ParameterSignature sig, String name, List<PotentialAssignment> list, Object array) {
for (int i = 0; i < Array.getLength(array); i++) {
if (!sig.canAcceptValue(Array.get(array, i))) {
return;
Object value = Array.get(array, i);
if (sig.canAcceptValue(value)) {
list.add(PotentialAssignment.forValue(name + "[" + i + "]", value));
}
list.add(PotentialAssignment.forValue(name + "[" + i + "]", Array.get(array, i)));
}
}

Expand Down
55 changes: 25 additions & 30 deletions src/test/java/org/junit/tests/experimental/theories/AssumingInTheoriesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,42 @@
package org.junit.tests.experimental.theories;

import static org.junit.tests.experimental.theories.TheoryTestUtils.runTheoryClass;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Test;
import org.junit.experimental.theories.DataPoint;
import org.junit.experimental.theories.Theories;
import org.junit.experimental.theories.Theory;
import org.junit.runner.JUnitCore;
import org.junit.runner.Request;
import org.junit.runner.Result;
import org.junit.runner.RunWith;
import org.junit.runner.Runner;
import org.junit.runners.model.InitializationError;

@RunWith(Theories.class)
public class AssumingInTheoriesTest {

@Test
public void noTheoryAnnotationMeansAssumeShouldIgnore() {
Assume.assumeTrue(false);
}

@Test
public void theoryMeansOnlyAssumeShouldFail() throws InitializationError {
JUnitCore junitRunner = new JUnitCore();
Runner theoryRunner = new Theories(TheoryWithNoUnassumedParameters.class);
Request request = Request.runner(theoryRunner);
Result result = junitRunner.run(request);
Assert.assertEquals(1, result.getFailureCount());
}

/**
* Simple class that SHOULD fail because no parameters are met.
*/
public static class TheoryWithNoUnassumedParameters {

@DataPoint
public final static boolean FALSE = false;

@Theory
public void theoryWithNoUnassumedParameters(boolean value) {
Assume.assumeTrue(value);
}
}
@Test
public void noTheoryAnnotationMeansAssumeShouldIgnore() {
Assume.assumeTrue(false);
}

@Test
public void theoryMeansOnlyAssumeShouldFail() throws InitializationError {
Result result = runTheoryClass(TheoryWithNoUnassumedParameters.class);
Assert.assertEquals(1, result.getFailureCount());
}

/**
* Simple class that SHOULD fail because no parameters are met.
*/
public static class TheoryWithNoUnassumedParameters {

@DataPoint
public final static boolean FALSE = false;

@Theory
public void theoryWithNoUnassumedParameters(boolean value) {
Assume.assumeTrue(value);
}
}

}
42 changes: 39 additions & 3 deletions src/test/java/org/junit/tests/experimental/theories/ParameterSignatureTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.hamcrest.CoreMatchers.isA;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assume.assumeTrue;

import java.lang.annotation.Annotation;
Expand Down Expand Up @@ -40,17 +41,52 @@ public void getType(Method method, int index) {
.signatures(method).get(index).getType());
}

public void foo(@TestedOn(ints = {1, 2, 3})
int x) {
public void foo(@TestedOn(ints = {1, 2, 3}) int x) {
}

@Test
public void getAnnotations() throws SecurityException,
NoSuchMethodException {
Method method = ParameterSignatureTest.class.getMethod("foo", int.class);
Method method = getClass().getMethod("foo", int.class);
List<Annotation> annotations = ParameterSignature.signatures(method)
.get(0).getAnnotations();
assertThat(annotations,
CoreMatchers.<TestedOn>hasItem(isA(TestedOn.class)));
}

public void intMethod(int param) {
}

public void integerMethod(Integer param) {
}

public void numberMethod(Number param) {
}

@Test
public void primitiveTypesShouldBeAcceptedAsWrapperTypes() throws Exception {
List<ParameterSignature> signatures = ParameterSignature
.signatures(getClass().getMethod("integerMethod", Integer.class));
ParameterSignature integerSignature = signatures.get(0);

assertTrue(integerSignature.canAcceptType(int.class));
}

@Test
public void primitiveTypesShouldBeAcceptedAsWrapperTypeAssignables() throws Exception {
List<ParameterSignature> signatures = ParameterSignature
.signatures(getClass().getMethod("numberMethod", Number.class));
ParameterSignature numberSignature = signatures.get(0);

assertTrue(numberSignature.canAcceptType(int.class));
}

@Test
public void wrapperTypesShouldBeAcceptedAsPrimitiveTypes() throws Exception {
List<ParameterSignature> signatures = ParameterSignature
.signatures(getClass().getMethod("intMethod", int.class));
ParameterSignature intSignature = signatures.get(0);

assertTrue(intSignature.canAcceptType(Integer.class));
}
}
12 changes: 12 additions & 0 deletions src/test/java/org/junit/tests/experimental/theories/TheoryTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,13 @@
import java.util.List;

import org.junit.experimental.theories.PotentialAssignment;
import org.junit.experimental.theories.Theories;
import org.junit.experimental.theories.internal.Assignments;
import org.junit.runner.JUnitCore;
import org.junit.runner.Request;
import org.junit.runner.Result;
import org.junit.runner.Runner;
import org.junit.runners.model.InitializationError;
import org.junit.runners.model.TestClass;

public final class TheoryTestUtils {
Expand All @@ -17,5 +23,11 @@ public static List<PotentialAssignment> potentialAssignments(Method method)
new TestClass(method.getDeclaringClass()))
.potentialsForNextUnassigned();
}

public static Result runTheoryClass(Class<?> testClass) throws InitializationError {
Runner theoryRunner = new Theories(testClass);
Request request = Request.runner(theoryRunner);
return new JUnitCore().run(request);
}

}
Loading