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 #449, stopping AllMembersSupplier & Theories hiding DataPoints method exceptions #639

Merged
merged 5 commits into from
Mar 26, 2013
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
3 changes: 2 additions & 1 deletion src/main/java/org/junit/experimental/theories/DataPoint.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,5 @@
@Target({FIELD, METHOD})
public @interface DataPoint {
String[] value() default {};
}
Class<? extends Throwable>[] ignoredExceptions() default {};
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,5 @@
@Target({FIELD, METHOD})
public @interface DataPoints {
String[] value() default {};
Class<? extends Throwable>[] ignoredExceptions() default {};
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@
import java.util.List;

public abstract class ParameterSupplier {
public abstract List<PotentialAssignment> getValueSources(ParameterSignature sig);
public abstract List<PotentialAssignment> getValueSources(ParameterSignature sig) throws Throwable;
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@
public abstract class PotentialAssignment {
public static class CouldNotGenerateValueException extends Exception {
private static final long serialVersionUID = 1L;

public CouldNotGenerateValueException() {
}

public CouldNotGenerateValueException(Throwable t) {
super(t);
}
}

public static PotentialAssignment forValue(final String name, final Object value) {
Expand Down
23 changes: 14 additions & 9 deletions src/main/java/org/junit/experimental/theories/Theories.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import java.util.List;

import org.junit.Assert;
import org.junit.experimental.theories.PotentialAssignment.CouldNotGenerateValueException;
import org.junit.Assume;
import org.junit.experimental.theories.internal.Assignments;
import org.junit.experimental.theories.internal.ParameterizedAssertionError;
import org.junit.internal.AssumptionViolatedException;
Expand Down Expand Up @@ -202,8 +202,13 @@ protected Statement methodInvoker(FrameworkMethod method, Object test) {

@Override
public Object createTest() throws Exception {
return getTestClass().getOnlyConstructor().newInstance(
complete.getConstructorArguments(nullsOk()));
Object[] params = complete.getConstructorArguments();

if (!nullsOk()) {
Assume.assumeNotNull(params);
}

return getTestClass().getOnlyConstructor().newInstance(params);
}
}.methodBlock(fTestMethod).evaluate();
}
Expand All @@ -213,13 +218,13 @@ private Statement methodCompletesWithParameters(
return new Statement() {
@Override
public void evaluate() throws Throwable {
try {
final Object[] values = complete.getMethodArguments(
nullsOk());
method.invokeExplosively(freshInstance, values);
} catch (CouldNotGenerateValueException e) {
// ignore
final Object[] values = complete.getMethodArguments();

if (!nullsOk()) {
Assume.assumeNotNull(values);
}

method.invokeExplosively(freshInstance, values);
}
};
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
package org.junit.experimental.theories.internal;

import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.core.IsInstanceOf.instanceOf;

import java.lang.reflect.Array;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;

import org.junit.Assume;
import org.junit.experimental.theories.DataPoint;
import org.junit.experimental.theories.DataPoints;
import org.junit.experimental.theories.ParameterSignature;
Expand Down Expand Up @@ -36,17 +40,24 @@ public Object getValue() throws CouldNotGenerateValueException {
} 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
} catch (Throwable t) {
DataPoint annotation = fMethod.getAnnotation(DataPoint.class);
if (annotation != null) {
for (Class<? extends Throwable> ignorable : annotation.ignoredExceptions()) {
Assume.assumeThat(t, not(instanceOf(ignorable)));
Copy link
Member

Choose a reason for hiding this comment

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

Clever.

}
}

throw new CouldNotGenerateValueException(t);
}
}
}

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

private final TestClass fClass;

/**
Expand All @@ -57,7 +68,7 @@ public AllMembersSupplier(TestClass type) {
}

@Override
public List<PotentialAssignment> getValueSources(ParameterSignature sig) {
public List<PotentialAssignment> getValueSources(ParameterSignature sig) throws Throwable {
List<PotentialAssignment> list = new ArrayList<PotentialAssignment>();

addSinglePointFields(sig, list);
Expand All @@ -68,15 +79,23 @@ public List<PotentialAssignment> getValueSources(ParameterSignature sig) {
return list;
}

private void addMultiPointMethods(ParameterSignature sig, List<PotentialAssignment> list) {
private void addMultiPointMethods(ParameterSignature sig, List<PotentialAssignment> list) throws Throwable {
for (FrameworkMethod dataPointsMethod : getDataPointsMethods(sig)) {
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
} catch (Throwable t) {
DataPoints annotation = dataPointsMethod.getAnnotation(DataPoints.class);
if (annotation != null) {
Copy link
Member

Choose a reason for hiding this comment

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

This is the second, slightly different, version of this logic. Can we extract it to a shared location? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it feels un-DRY with both of these, but commonising it is much tricker than it looks because it's really checking against the ignoredExceptions property on two subtly different annotations (DataPoint/DataPoints) and there's no heirarchy there, as you can't do any inheritance with annotations, so they're totally independent and the real commonality is fairly low...

Because there's no shared type here you'd have to loop through the lists for both exceptions sequentially or something, and it's all generally all bit messy. It's also currently technically possible to use something as a datapoint AND a datapoint_s_ method (supplying the result to theories looking for int's and int[]'s), and you could then have exceptions ignored in only one of those cases, so you can't just combine the ignoredExceptions properties of both annotations and then run the same logic. Hmm.

We could instead pull this out into a single easily-checkable @IgnoreExceptions annotation which would let us make this DRYer, but then people would probably expect that to work in all sorts of other places, when it clearly won't, and implementing a general purpose exceptions ignorer is a) a big job b) not really something we want to add.

In summary, I'm not sure of a way to actually do that, and I think the best option is just to leave it as is with a little duplication. If you can think of a way to actually do this I'd be very interested to see it though, and it would definitely be an improvement, if you can somehow persuade the Java type system to play ball.

Copy link
Member

Choose a reason for hiding this comment

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

Great point. Maybe just pull out isAssignableToAny(Throwable t, Class<? extends Throwable> potentialSuperClasses), and you're right that it's not worth doing more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, yes, of course. That definitely improves things somewhat even with the type troubles. Now done.

for (Class<? extends Throwable> ignored : annotation.ignoredExceptions()) {
if (ignored.isAssignableFrom(t.getClass())) {
return;
}
}
}
throw t;
}
}
}
Expand All @@ -94,7 +113,7 @@ private void addMultiPointFields(ParameterSignature sig, List<PotentialAssignmen
for (final Field field : getDataPointsFields(sig)) {
addArrayValues(sig, field.getName(), list, getStaticFieldValue(field));
}
}
}

private void addSinglePointFields(ParameterSignature sig, List<PotentialAssignment> list) {
for (final Field field : getSingleDataPointFields(sig)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,21 +63,17 @@ public Assignments assignNext(PotentialAssignment source) {
fUnassigned.size()), fClass);
}

public Object[] getActualValues(int start, int stop, boolean nullsOk)
public Object[] getActualValues(int start, int stop)
throws CouldNotGenerateValueException {
Object[] values = new Object[stop - start];
for (int i = start; i < stop; i++) {
Object value = fAssigned.get(i).getValue();
if (value == null && !nullsOk) {
throw new CouldNotGenerateValueException();
}
values[i - start] = value;
values[i - start] = fAssigned.get(i).getValue();
}
return values;
}

public List<PotentialAssignment> potentialsForNextUnassigned()
throws Exception {
throws Throwable {
ParameterSignature unassigned = nextUnassigned();
List<PotentialAssignment> assignments = getSupplier(unassigned).getValueSources(unassigned);

Expand Down Expand Up @@ -127,20 +123,17 @@ private ParameterSupplier buildParameterSupplierFromClass(
return cls.newInstance();
}

public Object[] getConstructorArguments(boolean nullsOk)
public Object[] getConstructorArguments()
throws CouldNotGenerateValueException {
return getActualValues(0, getConstructorParameterCount(), nullsOk);
return getActualValues(0, getConstructorParameterCount());
}

public Object[] getMethodArguments(boolean nullsOk)
throws CouldNotGenerateValueException {
return getActualValues(getConstructorParameterCount(),
fAssigned.size(), nullsOk);
public Object[] getMethodArguments() throws CouldNotGenerateValueException {
return getActualValues(getConstructorParameterCount(), fAssigned.size());
}

public Object[] getAllArguments(boolean nullsOk)
throws CouldNotGenerateValueException {
return getActualValues(0, fAssigned.size(), nullsOk);
public Object[] getAllArguments() throws CouldNotGenerateValueException {
return getActualValues(0, fAssigned.size());
}

private int getConstructorParameterCount() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ protected Collection<Field> getSingleDataPointFields(ParameterSignature sig) {
}
}

return fieldsWithMatchingNames;
return fieldsWithMatchingNames;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public final class TheoryTestUtils {
private TheoryTestUtils() { }

public static List<PotentialAssignment> potentialAssignments(Method method)
throws Exception {
throws Throwable {
return Assignments.allUnassigned(method,
new TestClass(method.getDeclaringClass()))
.potentialsForNextUnassigned();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ protected void runWithIncompleteAssignment(Assignments incomplete)
}

private GuesserQueue createGuesserQueue(Assignments incomplete)
throws Exception {
throws Throwable {
ParameterSignature nextUnassigned = incomplete.nextUnassigned();

if (nextUnassigned.hasAnnotation(Stub.class)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,21 @@

import java.util.List;

import org.junit.Rule;
import org.junit.Test;
import org.junit.experimental.theories.DataPoint;
import org.junit.experimental.theories.DataPoints;
import org.junit.experimental.theories.ParameterSignature;
import org.junit.experimental.theories.PotentialAssignment;
import org.junit.experimental.theories.Theory;
import org.junit.experimental.theories.internal.AllMembersSupplier;
import org.junit.rules.ExpectedException;
import org.junit.runners.model.TestClass;

public class AllMembersSupplierTest {
@Rule
public ExpectedException expected = ExpectedException.none();


public static class HasDataPointsArrayField {
@DataPoints
Expand All @@ -29,7 +34,7 @@ public void theory(String param) {
}

@Test
public void dataPointsArrayShouldBeRecognized() throws Exception {
public void dataPointsArrayShouldBeRecognized() throws Throwable {
List<PotentialAssignment> assignments = potentialAssignments(
HasDataPointsArrayField.class.getMethod("theory", String.class));

Expand All @@ -46,7 +51,7 @@ public void theory(Integer param) {
}

@Test
public void dataPointsArrayShouldBeRecognizedOnValueTypeNotFieldType() throws Exception {
public void dataPointsArrayShouldBeRecognizedOnValueTypeNotFieldType() throws Throwable {
List<PotentialAssignment> assignments = potentialAssignments(
HasDataPointsArrayWithMatchingButInaccurateTypes.class.getMethod("theory", Integer.class));

Expand All @@ -65,7 +70,7 @@ public void theory(Object param) {
}

@Test
public void dataPointMethodShouldBeRecognizedForOverlyGeneralParameters() throws Exception {
public void dataPointMethodShouldBeRecognizedForOverlyGeneralParameters() throws Throwable {
List<PotentialAssignment> assignments = potentialAssignments(
HasDataPointMethodWithOverlyGeneralTypes.class.getMethod("theory", Object.class));

Expand All @@ -82,7 +87,7 @@ public void theory(Object obj) {
}

@Test
public void dataPointsAnnotationMeansTreatAsArrayOnly() throws Exception {
public void dataPointsAnnotationMeansTreatAsArrayOnly() throws Throwable {
List<PotentialAssignment> assignments = potentialAssignments(
HasDataPointsWithObjectParameter.class.getMethod("theory", Object.class));

Expand All @@ -101,13 +106,9 @@ public HasDataPointsFieldWithNullValue(Object obj) {
}

@Test
public void dataPointsArrayFieldMayContainNullValue()
throws SecurityException, NoSuchMethodException {
List<PotentialAssignment> valueSources = new AllMembersSupplier(
new TestClass(HasDataPointsFieldWithNullValue.class))
.getValueSources(ParameterSignature.signatures(
HasDataPointsFieldWithNullValue.class.getConstructor(Object.class))
.get(0));
public void dataPointsArrayFieldMayContainNullValue() throws Throwable {
List<PotentialAssignment> valueSources = allMemberValuesFor(
HasDataPointsFieldWithNullValue.class, Object.class);
assertThat(valueSources.size(), is(2));
}

Expand All @@ -122,13 +123,34 @@ public HasDataPointsMethodWithNullValue(Integer i) {
}

@Test
public void dataPointsArrayMethodMayContainNullValue()
throws SecurityException, NoSuchMethodException {
List<PotentialAssignment> valueSources = new AllMembersSupplier(
new TestClass(HasDataPointsMethodWithNullValue.class))
public void dataPointsArrayMethodMayContainNullValue() throws Throwable {
List<PotentialAssignment> valueSources = allMemberValuesFor(
HasDataPointsMethodWithNullValue.class, Integer.class);
assertThat(valueSources.size(), is(2));
}

public static class HasFailingDataPointsArrayMethod {
@DataPoints
public static Object[] objects() {
throw new RuntimeException("failing method");
}

public HasFailingDataPointsArrayMethod(Object obj) {
}
}

@Test
public void allMembersFailsOnFailingDataPointsArrayMethod() throws Throwable {
expected.expect(RuntimeException.class);
expected.expectMessage("failing method");
allMemberValuesFor(HasFailingDataPointsArrayMethod.class, Object.class);
}

private List<PotentialAssignment> allMemberValuesFor(Class<?> testClass,
Class<?>... constructorParameterTypes) throws Throwable {
return new AllMembersSupplier(new TestClass(testClass))
.getValueSources(ParameterSignature.signatures(
HasDataPointsMethodWithNullValue.class.getConstructor(Integer.class))
testClass.getConstructor(constructorParameterTypes))
.get(0));
assertThat(valueSources.size(), is(2));
}
}
Loading