JUnit 4.12 issue when using theory + enum type + DataPoint that contains a null

[willix]

I have a Theory with a parameter list that includes a String, a boolean, and an
enum. The latter 2 should be automatically assigned and should not need any
explicit @DataPoint declaration (per the release note description below. When I
run the test, the enum param is assigned the null value instead of being
assigned each value of the enum, and I'm not sure why this is not working for
me. I would appreciate any help you can offer, please.

I cloned master from here:

https://github.com/junit-team/junit

...and looked at the JUnit 4.12 release notes:

https://github.com/junit-team/junit/wiki/4.12-release-notes

...and scrolled-down to pull request #654 which says:

"Any theory method parameters with boolean or enum types that can't be supplied
with values by any other sources will be automatically supplied with default
values: true and false, or every value of the given enum. If other explicitly
defined values are available (e.g. from a specified ParameterSupplier or some
DataPoints method in the theory class), only those explicitly defined values
will be used."

On that same release notes page, I also see the comment under pull request #549,
which says that a @DataPoint-annotated array field can contain null values.

So here's my test:

import org.junit.experimental.theories.DataPoints;
import org.junit.experimental.theories.Theories;
import org.junit.experimental.theories.Theory;
import org.junit.runner.RunWith;

@RunWith(Theories.class)
public class QuickTest {

@DataPoints
public static String[] platforms = new String[]{"windows", "linux", null};

public static enum JDKS {
JDK6,
JDK7,
}

@theory
public void testFoo(String platform, boolean truth, JDKS j) throws Exception {
System.out.println(platform + ":" + truth + ":" + j);
}

}

...and here's the output:
windows:true:null
windows:false:null
linux:true:null
linux:false:null
null:true:null
null:false:null

...which is not what I expected... I expected the 3rd value to be JDK6 or JDK7.
I run a 2nd test, but this time I remove the null array element from the
'platforms' field, and the results are expected:

import org.junit.experimental.theories.DataPoints;
import org.junit.experimental.theories.Theories;
import org.junit.experimental.theories.Theory;
import org.junit.runner.RunWith;

@RunWith(Theories.class)
public class QuickTest {

@DataPoints
public static String[] platforms = new String[]{"windows", "linux"};

public static enum JDKS {
JDK6,
JDK7,
}

@theory
public void testFoo(String platform, boolean truth, JDKS j) throws Exception {
System.out.println(platform + ":" + truth + ":" + j);
}

}

...which results in:
windows:true:JDK6
windows:true:JDK7
windows:false:JDK6
windows:false:JDK7
linux:true:JDK6
linux:true:JDK7
linux:false:JDK6
linux:false:JDK7

Cool. But now just for fun, I remove the 'platform' param from testFoo and I
restore the null array element to 'platforms' field. Since there is no longer
any String param in the param list (and since String is a final class so there's
no possibility of an assignable subclass), I expect that the 'platforms'
DataPoint will just be silently ignored and I would get the expected output. But
I actually see the unexpected output, as if a null array element that exists in
a field (that's not even used in the Theory's param list) is affecting the
values of a totally different type, the enum type (causing the enum param to
receive only a null value):

import org.junit.experimental.theories.DataPoints;
import org.junit.experimental.theories.Theories;
import org.junit.experimental.theories.Theory;
import org.junit.runner.RunWith;

@RunWith(Theories.class)
public class QuickTest {

@DataPoints
public static String[] platforms = new String[]{"windows", "linux", null};

public static enum JDKS {
JDK6,
JDK7,
}

@theory
public void testFoo(boolean truth, JDKS j) throws Exception {
System.out.println(truth + ":" + j);
}

}

...which results in:
true:null
false:null

My understanding is that this part of JUnit is relevant to recent work on theories by @pimterry.

[pimterry]

Hmm. This is because when we look for datapoints that are valid to use for a parameter, we first search all datapoint-annotated fields for values that we can use, and then autogenerate values if none can be found.

Also, we now do this on runtime-type of values in arrays, not the declared types of the fields (in part because we often can't get at it, e.g. generic types on iterables), so this includes looking through your platforms array for valid values. Null is then actually a valid value for the JDKS parameter for your theory, so that gets used, and then no autogeneration happens (since we've already found a value).

I'm not immediately sure what the solution is to this, from a behavioural point of view... If you had an Object array that contained an actual JDKS value, we certainly should use that value, rather than autogenerating JDKS values. Arguably we shouldn't ever pull values from arrays that don't match the types at all (e.g. String/JDKS), but that's never normally a problem, because the types have to be related for their to be any possible relevant values except for null values...

Which I think then suggests this should be a special case fix to limit when we automatically find null values from datapoint arrays, so AllMembersSupplier doesn't use the null value unless there's something indicating it's relevant. I'll look into that now; shout if anybody else has thoughts on this.

[pimterry]

Wow, yes, this is quite tricky 😕. The best solution (I can think of) is to restrict use of null values for parameters so they're only allowed if the parameter types are explicitly assignable to the component of field types (so only consider String[]s for String params), since otherwise nulls just have to always be valid for any type, which will cause things like this bug.

This still doesn't work for iterable datapoints though, so we should either: disallow nulls in iterable datapoints, or only have this restriction on arrays themselves (so nulls in iterables still act like this). I'd like to avoid having any unexpected differences between using arrays and iterables, so I suggest the former.

A real magical solution would be to actually have definitive types, and you can get closer to this information than we currently are (at the moment we only look at runtime types), but there are some cases we currently allow where it's impossible to get at the generic type, e.g.

@DataPoints
public static Object values = new ArrayList<String>() {{ [...add values...] }};

Could require a relevant type signature on the field in the case of iterable datapoints? I.e. refuse the above, and fail the theories requesting that values has a type of the form Iterable.

I think that basically gives three potential solutions:

• Only use nulls from datapoints in theories when the array component types are appropriate, for arrays only (ignore this case for iterables)
• Only use nulls from datapoints in theories when the array component types are appropriate, and ban nulls from iterables.
• Only use nulls from datapoints in theories when the array/generic component types are appropriate, and require all iterable datapoint fields and methods to have Iterable return/field types, so we can always derive the actual component type.

I actually quite like the last option. It potentially breaks for anybody who's been depending on 4.12 while it's in development, but I think that's fine, and you couldn't use iterables in 4.11 at all anyway. It also adds some restrictions that feel slightly arbitrary, but with sensible error messages I think it'll be clear enough and it's always an easy change for users to make.

With this you're essentially declaring with your return and field types the upper limits on which parameters datapoints should be considered for (so Object fields will be considered for every parameter, Number fields will be considered for all number subclass parameters, etc), which is actually quite a nice model.

@dsaff thoughts?

[dsaff]

@pimterry, can one get the type signature from an Iterable at runtime?

[pimterry]

From a field or return type yes, but not from a given instance, which is the problem.

Fields and methods in Java have a [getGenericType()](http://docs.oracle.com/javase/6/docs/api/java/lang/reflect/Field.html#getGenericType(%29) method which gives you both the actual class of the field (Iterable), and any declared generic type parameters within the field, and instances don't have this. You can then traverse up this tree, but depending on how exactly the generic type is resolved in the tree full generic type information is then sometimes not accessible, I believe, I think perhaps if it's a generic interface implemented by a superclass, for example? Not sure, but this stackoverflow answer has a few examples for this where it does and doesn't work.

In practice if you wanted to extract generic type information from there you'd need to require it be explicitly specified in the given field/return type, so the datapoints field/method had to specify an Iterable return type, with no ArrayList or similar allowed. This also eliminates the problem where people just use too general types (e.g. Object), to still hold the same data, but with useless field types.

Otherwise you can try and derive it from instances, but some types are never accessible, specifically ones that are only specified on the constructor, or as a generic argument to a method, for example. I think this is just because they're not really defined statically, as the objects are built at runtime, so the information isn't kept. Thus if you have a StringList class which extends List, you can look at instances of that, and then get the static type information that says that StringLists are Lists with a String generic parameter, since that's static information, but you can't look at a vanilla instance of ArrayList and do that same, since the generic parameter isn't specified until runtime (unless the variable, field or return type it's stored will provide that information). Does that clarify things?