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 for #637, making theory parameters assignment more consistent across sources #651

Merged
merged 8 commits into from
Mar 19, 2013

Conversation

pimterry
Copy link
Contributor

There's a few inconsistencies in theory implementation, as discussed in #637. This shouldn't make any differences to reasonable normal usage, just to some edge cases.

Essential changes:

  • Datapoint matches for all field datapoints and multi-valued method datapoints (i.e. not single-valued method datapoints) are done on runtime type, not the field/method return type (previously this was the case for multi-valued array methods only)
  • All datapoint type matches now correctly handle boxing and unboxing for primitive and wrapper types; int values will be considered for theory parameters that are Integer assignable, and Integer values will be considered for int parameters)

(#637 has examples and more details on this)

This conflicts with my other open pull request (#639), but given that we want to make the behaviour in that case more complicated, I think it would be better to fix these inconsistencies first, and then continue that on top. This is also most of the setup work for a fix I have going for #110/#126, as we have to pull types at runtime for iterables there anyway (since the generic type info isn't reliably available).

private boolean canAcceptUnboxed(Class<?> candidate) {
Field primitiveClassField = null;
try {
primitiveClassField = candidate.getDeclaredField("TYPE");
Copy link
Member

Choose a reason for hiding this comment

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

Generating an exception can be pretty expensive. Can we use a map reversing the one in canAcceptBoxed, instead?

@dsaff
Copy link
Member

dsaff commented Mar 11, 2013

One red flag to me: this might make certain types of theory classes much slower. For example, consider a class BigExpensive that is expensive/slow to create:

public class SomeTheories {
@DataPoint public static BigExpensive bigExpensiveOne() { ... }
@DataPoint public static int ZERO = 0;

@test public void foo(int x) { ... }
@test public void fooExpensive(BigExpensive b) { ... }
}

Here, we'll be invoking bigExpensiveOne for foo, which we weren't before.

Perhaps we can optimize by detecting when there's no chance that the runtime type of a method return value could possibly match a parameter's type?

…d be relevant, and theory parameter signatures are compared with types via a prebuilt map rather than reflection.
@pimterry
Copy link
Contributor Author

Updated to fix those two comments. We now make the conversion map symmetric, so we don't need to use reflection (so there's no expensive exception thrown), and we check that types are potentially assignable before calling both single-valued and multi-valued datapoint methods. Multi-valued won't be feasible to work like this for iterables because of type erasure, but we can make it work for arrays, as now, so we might as well do that too.


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.

map.put(double.class, Double.class);

// Make all type conversions symmetric
Iterable<Entry<Class<?>, Class<?>>> initialEntries = new HashSet<Entry<Class<?>, Class<?>>>(map.entrySet());
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps even simpler (and certainly a little more efficient) would be:

addSymmetrically(map, boolean.class, Boolean.class), where

addSymmetrically =
map.put(a, b);
map.put(b, a);

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 don't have any strong feelings on this one way or the other, but I'm happy to change it if you do, so I have.

}

public Class<?> getType() {
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?

@dsaff
Copy link
Member

dsaff commented Mar 18, 2013

We're getting close. Can you edit the pull-request summary to summarize the current state of things?

Moving forward, I wonder if we could harmonize everything by having the ability for datapoints to return either a Foo or a Provider, with Provider as a way to get deferred computation.

@pimterry
Copy link
Contributor Author

Summary updated and whitespace fixed.

Stream of thought on providers (although this is quite out of scope, and we should open an issue/wiki page/something if you want to take this further):

Provider and values is certainly do-able, and committing to that model would nicely clarify the single/multi datapoint method differences.

It does sort-of duplicate the ParameterSuppliers though, from the other direction (specifying a source of generated assignments for all parameters in a class, rather than requesting assignments generation from a source for one parameter). What's the difference between a ParameterSupplier and a Provider here? There's clearly some, but it's not really intuitive to me...

I think if you want to move to that model and carefully break this backward-compatibility, I'd instead separate by annotations to make it clearer: have @DataPoints all be generated once, statically (i.e. put the single-valued datapoints change back in) and then have a @DataPointGenerator (name tbd) annotation for datapoint methods whose values are rebuilt each time. This notably makes for easy migration for people who get bitten by this non-backward compatibility: they just switch to the new annotation, and they don't have to change return types to wrap generation up in a Provider or similar. And splitting behaviour based on the annotated method's return type is going to be a bit messy too (what if you want to actually take Provider parameters to a theory method, to test your providers?)

I think there's maybe a way you can do this even better and pull it closer into the ParameterSupplier model, so you end up with two clear parallel parameter-source hierarchies -- field/method datapoints for static data values and field/method/class ParameterSuppliers for regenerating data values -- but I haven't totally worked out the model for that yet. Ideas very welcome!


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.

@dsaff
Copy link
Member

dsaff commented Mar 19, 2013

Okay, we're in good shape on this pull. I'll merge. Can you make a comment in the release notes?

I agree that further discussion would be interesting outside the scope of this pull request, and think your "separate annotation" idea has merit.

dsaff pushed a commit that referenced this pull request Mar 19, 2013
Fixes for #637, making theory parameters assignment more consistent across sources
@dsaff dsaff merged commit c7b1880 into junit-team:master Mar 19, 2013
@pimterry pimterry deleted the datapoint-result-reuse-#638 branch March 21, 2013 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants