-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Changes from 4 commits
5c2070d
50e3d1b
38d9130
93d7738
dbe7711
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
|
@@ -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))); | ||
} | ||
} | ||
|
||
throw new CouldNotGenerateValueException(t); | ||
} | ||
} | ||
} | ||
|
||
@Override | ||
public String getDescription() throws CouldNotGenerateValueException { | ||
return fMethod.getName(); | ||
} | ||
} | ||
} | ||
|
||
private final TestClass fClass; | ||
|
||
/** | ||
|
@@ -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); | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} | ||
|
@@ -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)) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever.